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

Set container local names explicitly #3398

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Apr 19, 2023

The name field in the blueprint customizations for containers was ignored. We now use it to set the LocalName and fall back to the source value when it's not.

This is a behaviour change for both cases:

  • When name set, the full string is added to the LocalName field in the spec.
  • When name is not set, the full source string is added to the LocalName field in the spec. Previously, the source.Name() was added instead, which corresponds to the source container name without any tag or digest.

The LocalName is then set in the manifest as the name option for the input image in the skopeo source. This is used when importing the image in the skopeo stage: https://github.com/osbuild/osbuild/blob/7cd36f9797a8de740afdd4f9e22cfa004bf04531/stages/org.osbuild.skopeo#L176-L178

As a result, the names array in the images.json file is populated with the name (or source) from the blueprint (or compose request), including any tags or digests the user has specified.

In the specific case where a user specifies a name, it gives the user the ability to rename an image or add a tag of their own.
In the case where a user does not specify a name, it preserves any tag that was used to define the image.

The most important behaviour change is that local images are no longer tagged as latest when the name (or source if empty) is specified with a digest. Previously, all images were tagged as latest in the image, which is the standard tag given when no tag is specified during the skopeo copy. Now, the latest tag is automatically added if the name or source contains no other tag or digest.

Fixes COMPOSER-1952


This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change such as

The PR which added manifest lists and the format-request-map was changed
to include two containers for the `with-container` compose requests [1]
was rebased after the PR which added RHEL 8.9 and RHEL 9.3 was merged
[2].  The test manifests were not updated after the rebase, so they were
never created with the new request.

Updating them now.

[1] osbuild#3336
[2] osbuild#3350
Set the LocalName for the spec using a separate argument in the
NewSpec() constructor instead of reusing the `source` arg.
The name is already available in the calling scope in the client's
Resolve() method.

If the LocalName is an empty string, default to the remote (source)
reference.  This is a change from the previous behaviour which only used
the base source.Name().  The full source corresponds to the
user-provided source value, which includes any specified tag or digest.

The `name` argument which is used in the `Resolve()` function should
always correspond to the user-provided container name.
@thozza
Copy link
Member

thozza commented Apr 19, 2023

Didn't review yet, but I wonder if #3395 is attempting to solve a similar problem and should be closed as duplicate? (although there is literally zero information in that PR and commit messages also don't help much)

Add name field to the manifest-list-test container in the test
request.  The value is the same as the source but with a `v1` tag
added.

In the manifests, the name field for the manifest-list-test is added to
the skopeo stage.  The `name` option of the fedora-minimal container in
the skopeo stage is also changed to reflect the full source reference
including the `latest` tag.
Add a local name for the fedora minimal image which includes the tag
`v1`.

Check the image info for the expected names:
1. For the fedora-minimal image, the name as it appears in the blueprint
   should be included in the names list.
2. For the manifest-list-test image, the full source reference should be
   included in the names list since no name was specified in the
   blueprint.
@achilleas-k
Copy link
Member Author

Didn't review yet, but I wonder if #3395 is attempting to solve a similar problem and should be closed as duplicate? (although there is literally zero information in that PR and commit messages also don't help much)

Ah yes. I forgot about that one. I'll close it in favour of this.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Amazing work 👍

Copy link
Collaborator

@schutzbot schutzbot left a comment

Choose a reason for hiding this comment

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

⚠️ This PR introduces changes in at least one manifest (when comparing PR HEAD eeff366 with the main merge-base d834286). Please review the changes. The changes can be found in the artifacts of the Manifest-diff job [0] as manifests.diff.

[0] https://gitlab.com/redhat/services/products/image-builder/ci/osbuild-composer/-/jobs/4140093149/artifacts/browse

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

Looks good! :)

@ondrejbudai ondrejbudai merged commit 03b093a into osbuild:main Apr 19, 2023
37 checks passed
@achilleas-k achilleas-k deleted the container-localname branch April 20, 2023 08:46
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.

None yet

4 participants