Introduce a ObsPackage class for grouping Container Images #1815
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Created a staging project on OBS for 16.0: home:defolos:BCI:Staging:16.0:16.0-1815 Build ResultsRepository
Repository
Repository
Repository
Repository
Repository
Repository
Repository
Build succeeded ✅ To run BCI-tests against this PR, use the following command: OS_VERSION=16.0 TARGET=custom BASEURL=registry.opensuse.org/home/defolos/bci/staging/16.0/16.0-1815/ tox -- -n autoThe following images can be pulled from the staging project:
|
|
Created a staging project on OBS for 5: home:defolos:BCI:Staging:SLE-15-SP5:5-1815 Build ResultsRepository
Repository
Repository
Repository
Repository
Repository
Repository
Repository
Build succeeded ✅ To run BCI-tests against this PR, use the following command: OS_VERSION=15.5 TARGET=custom BASEURL=registry.opensuse.org/home/defolos/bci/staging/sle-15-sp5/5-1815/ tox -- -n autoThe following images can be pulled from the staging project:
|
|
Created a staging project on OBS for 7: home:defolos:BCI:Staging:SLE-15-SP7:7-1815 Build ResultsRepository
Repository
Repository
Repository
Repository
Repository
Repository
Repository
|
|
Created a staging project on OBS for 6: home:defolos:BCI:Staging:SLE-15-SP6:6-1815 Build ResultsRepository
Repository
Repository
Repository
Repository
Repository
Repository
|
|
Created a staging project on OBS for Tumbleweed: home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-1815 Build ResultsRepository
Repository
Repository
Repository
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-1815/ tox -- -n autoThe following images can be pulled from the staging project:
|
dirkmueller
reviewed
Oct 8, 2024
| @@ -1479,7 +1484,7 @@ def generate_disk_size_constraints(size_gb: int) -> str: | |||
|
|
|||
| SORTED_CONTAINER_IMAGE_NAMES = sorted( | |||
| ALL_CONTAINER_IMAGE_NAMES, | |||
| key=lambda bci: f"{ALL_CONTAINER_IMAGE_NAMES[bci].os_version}-{ALL_CONTAINER_IMAGE_NAMES[bci].name}", | |||
| key=lambda bci: f"{ALL_CONTAINER_IMAGE_NAMES[bci].os_version}-{ALL_CONTAINER_IMAGE_NAMES[bci].uid}", | |||
There was a problem hiding this comment.
that change seems unrelated?
There was a problem hiding this comment.
It is required, because ObsPackageBase doesn't have a name property
dirkmueller
reviewed
Oct 8, 2024
|
|
||
| async def _write_service_file(self, dest: str) -> list[str]: | ||
| await write_to_file(os.path.join(dest, "_service"), self._service_file_contents) | ||
| return ["_service"] |
There was a problem hiding this comment.
that return value seems unused? since it should end up in the files list anyway..
There was a problem hiding this comment.
It is used here:
|
There's at least 3 refactorings in this PR which makes it really hard to review. the osversion -> os_version rename, the service jinja to class refactoring and the change of the containercrate to ObsPackage refactoring. any chance to split the first two out so that it is easier to see what the rest is about? this is a 1000 lines diff over 11 commits. personally I don't really like the Base in the name also, why is there "magic" to collate up the services, but no magic to collate up the build flavors? instead they have to be set explicitly in multiple list entries (so whenever we add or remove a os_version that has different build flavors, the whole shebang needs to be duplicated)? Its such an implementation detail, it should be hidden the same way like services imho. |
This was referenced Oct 9, 2024
Merged
I've renamed the classes as you suggested,
Hm, I don't fully understand what pain point that this should solve though? |
|
Overall LGTM, seems to clean up the code a bit. I am not a big fan of the name |
dirkmueller
reviewed
Oct 22, 2024
| ], | ||
| package_name="apache-tomcat-10-image", | ||
| ), | ||
| _create_tomcat_container(OsVersion.TUMBLEWEED, "9", 17), |
There was a problem hiding this comment.
this should also be a multibuildobspackage, imho.
dirkmueller
reviewed
Oct 22, 2024
|
|
||
| @dataclass(kw_only=True) | ||
| class ObsPackage(abc.ABC): | ||
| """Abstract base class of the ObsPackage and the BaseContainerImage.""" |
dirkmueller
reviewed
Oct 22, 2024
Comment on lines
+127
to
+128
| TOMCAT_CONTAINERS: list[MultiBuildObsPackage | ApplicationStackContainer] = [ | ||
| MultiBuildObsPackage.from_bcis( |
There was a problem hiding this comment.
I guess this comes down to a matter of personal taste, but looking at this, this makes the _multibuild handling explicit over hiding the complexity in an abstraction. Code wise, this is larger and maintenance effort is higher (you need to manually group bcis into multibuilds accordingly). the "package name is unclear" issue seems a non issue for me because in the previous implementation, the package name is determined by the container, the containercrate just generates an extra _multibuild file for the cases where it's needed, in the places where it's needed, and that's all. How often is it needed to dig into the implementation of multibuild generation to fix a bug? I'd argue, almost never. how often do we need to update the multibuildobspackage mapping whenever a flavor or a variant is added? each and every time. Leaky abstractions are imho worse than no abstractions at all, and we could simply have an "extra_files" that containers the _multibuild file and be done with this overall. It's even more explicit than this construct.
for me, keeping the custom, repetitive boilerplate in the container definitions at a minimum should be a goal of the dockerfile generator. this implementation turns the inner guts outside, so you constantly have to deal with the can of worms for every container that you want to convert to multibuild (and I can see that we have several candidates for that, e.g. the openjdks, potentially the pythons and other language containers).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is pretty much a re-implementation of #1755, with more code but an API that I personally like a bit more, because it is more explicit:
This required the introduction of another base class
ObsPackageBasefrom which bothObsPackageandBaseContainerImageinherit. The API is not yet great, as there's now a bit of duplication going on, if you stuff a BCI into aObsPackage, but I wanted to get some feedback first.