Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring container crate into container family #2199

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rcmadhankumar
Copy link
Contributor

No description provided.

dcermak pushed a commit that referenced this pull request Jan 15, 2025
@rcmadhankumar rcmadhankumar changed the title Refactoring container crate into container family to make it more readable Refactoring container crate into container family for better readability Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
@rcmadhankumar rcmadhankumar changed the title Refactoring container crate into container family for better readability Refactoring container crate into container family Jan 15, 2025
Copy link

github-actions bot commented Jan 15, 2025

Created a staging project on OBS for Tumbleweed: home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-2199
Changes pushed to branch Tumbleweed-2199 as commit 87bfcd4d7ab36066ccdabe468072c9d7eeb8cfba
Build succeeded ✅

Build Results

Repository images in home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-2199 for x86_64: current state: published
Build results:

package name status build log
apache-tomcat-10-image ⛔ excluded live log
apache-tomcat-9-image ⛔ excluded live log
git-image ✅ succeeded live log

Repository images in home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-2199 for aarch64: current state: published
Build results:

package name status build log
apache-tomcat-10-image ⛔ excluded live log
apache-tomcat-9-image ⛔ excluded live log
git-image ✅ succeeded live log

Repository containerfile in home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-2199 for x86_64: current state: published
Build results:

package name status build log
apache-tomcat-10-image ⛔ excluded live log
apache-tomcat-9-image ⛔ excluded live log
git-image ⛔ excluded live log

Repository containerfile in home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-2199 for aarch64: current state: published
Build results:

package name status build log
apache-tomcat-10-image ⛔ excluded live log
apache-tomcat-9-image ⛔ excluded live log
git-image ⛔ excluded live log

Build succeeded ✅

To run BCI-tests against this PR, use the following command:

OS_VERSION=tumbleweed TARGET=custom BASEURL=registry.opensuse.org/home/defolos/bci/staging/tumbleweed/tumbleweed-2199/ tox -- -n auto
The following images can be pulled from the staging project:
  • registry.opensuse.org/home/defolos/bci/staging/tumbleweed/tumbleweed-2199/images/opensuse/git:latest

Copy link

github-actions bot commented Jan 15, 2025

Created a staging project on OBS for 7: home:defolos:BCI:Staging:SLE-15-SP7:7-2199
Changes pushed to branch 7-2199 as commit b27a300fd8dd20a39375037c51da740dd7fb74fb
Build succeeded ✅

Build Results

Repository images in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for x86_64: current state: published
Build results:

package name status build log
spack-image ⛔ excluded live log

Repository images in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for aarch64: current state: published
Build results:

package name status build log
spack-image ⛔ excluded live log

Repository images in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for s390x: current state: published
Build results:

package name status build log
spack-image ⛔ excluded live log

Repository images in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for ppc64le: current state: published
Build results:

package name status build log
spack-image ⛔ excluded live log

Repository containerfile in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for x86_64: current state: published
Build results:

package name status build log
spack-image ✅ succeeded live log

Repository containerfile in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for aarch64: current state: published
Build results:

package name status build log
spack-image ✅ succeeded live log

Repository containerfile in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for s390x: current state: published
Build results:

package name status build log
spack-image ⛔ excluded live log

Repository containerfile in home:defolos:BCI:Staging:SLE-15-SP7:7-2199 for ppc64le: current state: published
Build results:

package name status build log
spack-image ⛔ excluded live log

Build succeeded ✅

To run BCI-tests against this PR, use the following command:

OS_VERSION=15.7 TARGET=custom BASEURL=registry.opensuse.org/home/defolos/bci/staging/sle-15-sp7/7-2199/ tox -- -n auto
The following images can be pulled from the staging project:
  • registry.opensuse.org/home/defolos/bci/staging/sle-15-sp7/7-2199/containerfile/bci/spack:0.23

Copy link

github-actions bot commented Jan 15, 2025

Created a staging project on OBS for 6: home:defolos:BCI:Staging:SLE-15-SP6:6-2199
Changes pushed to branch 6-2199 as commit 01d8dccce18b67976973443ff997d1679323f9b0

@rcmadhankumar rcmadhankumar force-pushed the refactor-container-crate branch from c08a628 to 48bed4b Compare January 15, 2025 05:46
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
@rcmadhankumar rcmadhankumar force-pushed the refactor-container-crate branch from 48bed4b to 18a6af7 Compare January 15, 2025 05:56
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
@rcmadhankumar rcmadhankumar force-pushed the refactor-container-crate branch from 18a6af7 to cf3898a Compare January 15, 2025 06:00
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not yet what I was anticipating, currently this is just a bit more a rename of the ContainerCrate class.

What I'd like to see (eventually, we don't have to get there in a single PR):

  • crate/family shouldn't start mutating containers passed to it
  • the bot & package file writing code should switch to use ContainerFamily directly

Also, this currently is breaking multibuild generation: deb6b4a#diff-25c1eeff6e17b5c4eaac44d821f42827a798b8c8d36f6ef005edb1ae7771f041R84

src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
src/bci_build/package/__init__.py Outdated Show resolved Hide resolved
@rcmadhankumar rcmadhankumar force-pushed the refactor-container-crate branch from cf3898a to 70eb2a9 Compare January 15, 2025 09:55
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
@rcmadhankumar rcmadhankumar force-pushed the refactor-container-crate branch from 70eb2a9 to dd424ad Compare January 15, 2025 09:57
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
@rcmadhankumar rcmadhankumar force-pushed the refactor-container-crate branch from dd424ad to c3d28ef Compare January 15, 2025 10:00
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
dcermak pushed a commit that referenced this pull request Jan 15, 2025
@@ -1435,6 +1441,85 @@ def prepare_template(self) -> None:
pass


class ContainerFamily:
Copy link
Member

@dirkmueller dirkmueller Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rcmadhankumar . I do like the new name suggestion "Family" instead of "Crate". However I was going into a slightly different route in #2085 - perhaps it would make sense to revive that with the updated name and rebase it instead?

I've spent many hours trying to split the dreaded "package" module into smaller, more modular and more consumable chunks, so can we please keep this Family class separate and independent of a several thousand line long module already? I understand there are recursive imports with the type annotations. we either have to omit them or use the string form to avoid them though rather than piling it together into one module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, there were some recursive import issues, i will try moving it out.

@dirkmueller
Copy link
Member

What I'd like to see (eventually, we don't have to get there in a single PR):

  • the bot & package file writing code should switch to use ContainerFamily directly

This is what #2085 is doing..

@rcmadhankumar
Copy link
Contributor Author

rcmadhankumar commented Jan 17, 2025

This is not yet what I was anticipating, currently this is just a bit more a rename of the ContainerCrate class.

What I'd like to see (eventually, we don't have to get there in a single PR):

* crate/family shouldn't start mutating containers passed to it

* the bot & package file writing code should switch to use ContainerFamily directly

Also, this currently is breaking multibuild generation: deb6b4a#diff-25c1eeff6e17b5c4eaac44d821f42827a798b8c8d36f6ef005edb1ae7771f041R84

As per the implementation in container family, if container has more than one flavors, then we need set version_in_uid to false. Only then multibuild content will be generated. Tome cat containers doesn't have this attribute set, do we need to set for tomcat containers or any changes needed in check version in uid function?

def get_multibuild_file_content(
        self,
        container: BaseContainerImage,
    ) -> str:
        """Return the _multibuild file string to write for this container based on its family."""
        if not self.check_version_in_uid(container):
            return ""

        flavors: str = "\n".join(
            " " * 4 + f"<package>{pkg}</package>"
            for pkg in self.get_all_build_flavors(container)
        )
        return f"<multibuild>\n{flavors}\n</multibuild>"
        
def check_version_in_uid(
        self,
        container: BaseContainerImage,
    ) -> bool:
        """check if version_in_uid is set to False if a container has more than one flavour"""
        if len(self.get_all_build_flavors(container)) > 1 and getattr(
            container, "version_in_uid", False
        ):
            return False
        return True
        
        

Are we expecting something like this?

       ALL_CONTAINER_IMAGE_NAMES: dict[str, BaseContainerImage] = {
    f"{bci.uid}-{bci.os_version.pretty_print.lower()}": bci
    for bci in (
        *BASE_CONTAINERS,
        ....
        ....
        # *TOMCAT_CONTAINERS,
        *TOMCAT_FAMILIES

there were looping imports and that's why i moved it inside a single file, but i think i will work on moving it out as if there is a way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants