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.

Sign up for GitHub

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

Use the full container version for build_version #1982

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

Use the full container version for build_version #1982

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions src/bci_build/registry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,20 +49,32 @@ def registry_prefix(*, is_application: bool) -> str:
@staticmethod
def build_version(base_version, container: "BaseContainerImage") -> str:
"""Return the build version to set for this container build."""
container_version: str = container.tag_version
# if container_version is a numeric version and not a macro, then
# version.parse() returns a `Version` object => then we concatenate
# it with the existing build_version
# for non PEP440 versions, we'll get an exception and just return
# the parent's classes build_version
try:
packaging.version.parse(str(container_version))
stability_suffix: str = ""
if container._stability_suffix:
stability_suffix = "." + container._stability_suffix
return f"{base_version}.{container_version}{stability_suffix}"
except packaging.version.InvalidVersion:
return base_version

def _build_version_from_container_version(
container_version: int | str | None,
) -> str | None:
# if container_version is a numeric version and not a macro, then
# version.parse() returns a `Version` object => then we concatenate
# it with the existing build_version
# for non PEP440 versions, we'll get an exception and just return
# None
try:
packaging.version.parse(str(container_version))
stability_suffix: str = ""
if container._stability_suffix:
stability_suffix = "." + container._stability_suffix
return f"{base_version}.{container_version}{stability_suffix}"
except packaging.version.InvalidVersion:
return None

# first try to use the containers full version, if that fails, pick
# tag_version and if that also fails, fall back to the parent's classes
# version
Comment on lines +70 to +72
Copy link
Member

Choose a reason for hiding this comment

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

so that means switching from a %%..%%` based version to another version will change the behavior. is that really a good idea? seems very dangerous to me.

I think the current setup of "version" being the %%placeholder and "tag_version" being a placeholder-free "stable" version is good enough and more predictable in behavior. I know we fscked it up for kiwi but that has been fixed and there is now a check that tag_version needs to be set if there is a placeholder in version. we could enforce it even more to have tag_version always set which I think would make the behavior more predicatble.

return (
_build_version_from_container_version(container.version)
or _build_version_from_container_version(container.tag_version)
or base_version
)


class ApplicationCollectionRegistry(Registry):
Expand Down