Build the apache-tomcat containers as build_flavors #1755
Conversation
|
Created a staging project on OBS for 6: home:defolos:BCI:Staging:SLE-15-SP6:6-1755 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.6 TARGET=custom BASEURL=registry.opensuse.org/home/defolos/bci/staging/sle-15-sp6/6-1755/ tox -- -n autoThe following images can be pulled from the staging project:
|
dirkmueller
force-pushed
the
multibuild_tomcat
branch
from
479ce6a
to
f844a40
Compare
src/bci_build/package/__init__.py
Outdated
| @@ -1320,9 +1344,22 @@ async def write_file_to_dest(fname: str, contents: str | bytes) -> None: | |||
| False | |||
| ), f"got an unexpected build_recipe_type: '{self.build_recipe_type}'" | |||
|
|
|||
| if self.build_flavor: | |||
| flavors: str = "\n".join( | |||
| " " * 4 + f"<package>{pkg}</package>" | |||
There was a problem hiding this comment.
This looks really odd, just to produce what this does:
| " " * 4 + f"<package>{pkg}</package>" | |
| f" <package>{pkg}</package>" |
There was a problem hiding this comment.
I guess this comes down to a personal preference and DRY, don't care either way.
| build_flavor=f"openjdk{jre_version}", | ||
| all_build_flavors=[f"openjdk{ver}" for ver in all_jre_versions], |
There was a problem hiding this comment.
Taking my idea about one single item, I think something like this could be done here.
build_flavors=[Flavor(name=f"openjdk{ver}", current=ver==jre_version) for ver in all_jre_versions],There was a problem hiding this comment.
I think this is way too complicated for a simple container definition, and would have to repeated for each one of them (lets think about the python containers becoming multibuild, for example).
There was a problem hiding this comment.
Yes, it was just an idea, because I wasn't that happy with the solution proposed.
dirkmueller
force-pushed
the
multibuild_tomcat
branch
from
f844a40
to
f7c2d46
Compare
|
Created a staging project on OBS for Tumbleweed: home:defolos:BCI:Staging:Tumbleweed:Tumbleweed-1755 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-1755/ tox -- -n autoThe following images can be pulled from the staging project:
|
dirkmueller
force-pushed
the
multibuild_tomcat
branch
from
f7c2d46
to
3eb1b22
Compare
| {%- set all_build_flavors = [""] %} | ||
| {%- if image.crate and image.build_flavor %} | ||
| {%- set all_build_flavors = image.crate.all_build_flavors(image) %} | ||
| {%- endif %} | ||
| {%- for flavor in all_build_flavors %} |
There was a problem hiding this comment.
We should have a BaseContainerImage.all_build_flavors to avoid this. In theory, you have this attribute, but only if there's a crate, make it a function/property that always exists and perform this check.
There was a problem hiding this comment.
yeah, I was trying to avoid storing the (essentially duplicated) information in the ContainerImage because it is only needed for this one case. Polluting yet another method into that ContainerImage seems rather messy to me.
my intention was to move the generation of _multibuild and _service into the Crate entirely, but that was a bit too much for one commit so it'll come later on.
|
|
||
| def __init__(self, containers: list): | ||
| """Assign the crate for every container.""" | ||
| self._all_build_flavors: dict[tuple, set] = {} |
There was a problem hiding this comment.
I don't quite understand why we need this dict[tuple, set]. All calls are done with a container object, that in theory is a BaseContainerImage.
If this is the case, can't we just use something like dict[BaseContainerImage, str] and use self._all_build_flavors[container] = container.build_flavor?
Not even sure if we need a dict, or if it could be just a list.
There was a problem hiding this comment.
The crate is to unify over (os_version, container.package_name). if I understand correctly, you're unifying over all given containers (which could be many different versions, which have different build flavors depending on version and also have different package names, like in the tomcat case). However, we do not want to collect flavors that are not built for this version or not build for this package name. hence, we need to group them by this, which is not working as far as I can see in your proposal. Maybe I'm missing something.
There was a problem hiding this comment.
As an example, with tomcat, there are two flavors on SP6 (17, 21) and 3 on tumbleweed (17, 21, 22). the generated _multibuild file needs to be specific to the os-version. in theory it could even be specific to the package name.
dict/set are the most efficient ways to aggregate it.
|
|
||
| def all_build_flavors(self, container): | ||
| """Return all build flavors for this container in the crate""" | ||
| return sorted( |
There was a problem hiding this comment.
If you are sorting here and in multibuild, I would rather have it sorted in __init__, it would be called once and always be sorted, which might be a small tidy tiny faster each future call.
There was a problem hiding this comment.
in __init__ it is still a set, which is unsorted. I agree we move away from storing it as a set and switch to a sorted list. I don't think you'll be able to measure any difference but I agree it is more efficient.
dirkmueller
force-pushed
the
multibuild_tomcat
branch
from
3eb1b22
to
f6b7084
Compare
|
Created a staging project on OBS for 7: home:defolos:BCI:Staging:SLE-15-SP7:7-1755 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.7 TARGET=custom BASEURL=registry.opensuse.org/home/defolos/bci/staging/sle-15-sp7/7-1755/ tox -- -n autoThe following images can be pulled from the staging project:
|
This allows to sync the -RELEASE tag between multiple flavors, which is a requirement for appcollection. For that, BaseContainerImages can be optionally grouped into a ContainerCrate, which takes care of collecting build_flavors for proper _multibuild and _service generation.
dirkmueller
force-pushed
the
multibuild_tomcat
branch
from
f6b7084
to
6785a96
Compare
There was a problem hiding this comment.
To be frankly honest, I don't particularly like the current abstraction2 both in naming and behavior: it adds additional behind the scenes mutation and doesn't make it very obvious that we're actually combining multiple containers in one package.
I'd like to re-design the API. Merge this as is, if it is time critical though.
well, names can be changed. so "PackageContainer" then instead of Crate?
I don't see how .. yeah, it is kinda urgent for the SAC prototype, but I can adjust it if there is a concrete feedback on what to improve. |
|
as discussed, will be solved in a followup. |
This allows to sync the -RELEASE tag between multiple flavors, which is a requirement for appcollection.
For now all the build_flavors need to be manually passed in for _multibuild and _service generation. This could be eventually replaced by more automagic later on.