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

Add --prefer-index flag forimagetools create on a single source #2482

Merged
merged 6 commits into from
May 31, 2024

Conversation

rvoh-tismith
Copy link
Contributor

@rvoh-tismith rvoh-tismith commented May 29, 2024

…to the list of mediatypes we return the original bytes for when calling *Resolver.Combine rather than adding it to a newly created manifest list

Signed-off-by: Tim Smith <tismith@rvohealth.com>
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

While I tend to agree that this makes more sense as a default behavior, this is removing the capability to convert image manifest into manifest list. Should we add a new flag that keeps previous behavior?

@@ -79,7 +79,7 @@ func (r *Resolver) Combine(ctx context.Context, srcs []*Source, ann []string) ([

// on single source, return original bytes
if len(srcs) == 1 && len(ann) == 0 {
if mt := srcs[0].Desc.MediaType; mt == images.MediaTypeDockerSchema2ManifestList || mt == ocispec.MediaTypeImageIndex {
if mt := srcs[0].Desc.MediaType; mt == images.MediaTypeDockerSchema2ManifestList || mt == images.MediaTypeDockerSchema2Manifest || mt == ocispec.MediaTypeImageIndex {
Copy link
Member

Choose a reason for hiding this comment

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

If that is the desired behavior then why is there a mediatype check at all, or at least why is ocispec.MediaTypeImageManifest missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhhh great point! Yeah, it actually seems reasonable that there doesn't even need to be a check. Thinking through the logic, I guess I can't really think of when the logic would need to be different here. Seems like if there is one source and no annotations (and possibly if the --preserve-single-source flag is set to true, based on my response to your other comment), we should just return the original bytes. I almost feel like I'm missing something here though. It feels like this is here for a reason, but maybe it is just an oversight.

@rvoh-tismith
Copy link
Contributor Author

While I tend to agree that this makes more sense as a default behavior, this is removing the capability to convert image manifest into manifest list. Should we add a new flag that keeps previous behavior?

@tonistiigi great point. I was too focused on my own use case, but yeah, I definitely agree that some people are probably using this explicitly hoping to convert to a manifest list, or at the very least they've probably built workflows that are expecting this behavior, so changing the default behavior is probably too risky.

So, 2 things:

  1. I think a flag for this behavior is the right way to go. I'm not married to the name, but maybe something like --preserve-single-source, so docker buildx imagetools create --preserve-single-source=true -t foo/bar:newimage foo/bar:oldimage.
  2. Like I mentioned above, it feels like the behavior here should somewhat align with the behavior of docker buildx build which only outputs a manifest list if you specify multiple platforms. What are your thoughts there? Do you think there needs to be some alignment, or maybe the same flag for build so that you can control this behavior in both places? (that doesn't necessarily have to be included in this pr)

Thoughts?

@tonistiigi
Copy link
Member

Maybe smth. like --prefer-manifest-index --create-index etc. for the behavior of always creating the index result and without the flag, it is the original manifest on a single source.

@rvoh-tismith
Copy link
Contributor Author

Maybe smth. like --prefer-manifest-index --create-index etc. for the behavior of always creating the index result and without the flag, it is the original manifest on a single source.

Nice. I'll make some changes and push them up.

…havior when deciding on how to create an image/manifest from a single source.

Signed-off-by: Tim Smith <tismith@rvohealth.com>
…we return original bytes without filtering on mediaType, based on the preferIndex preference.

Signed-off-by: Tim Smith <tismith@rvohealth.com>
Signed-off-by: Tim Smith <tismith@rvohealth.com>
…n index regardless of what value you specify.

Signed-off-by: Tim Smith <tismith@rvohealth.com>
@rvoh-tismith
Copy link
Contributor Author

I split the difference and landed on --prefer-index lol. I also removed the mediaType checks.

Signed-off-by: Tim Smith <tismith@rvohealth.com>
@rvoh-tismith rvoh-tismith changed the title Added application/vnd.docker.distribution.manifest.v2+json mediatype to the list of mediatypes we return the original bytes for when calling *Resolver.Combine rather than adding it to a newly created manifest list Added --prefer-index flag which allows you to preserve the source manifest format instead of converting to a manifest list when calling imagetools create on a single source May 30, 2024
@tonistiigi tonistiigi changed the title Added --prefer-index flag which allows you to preserve the source manifest format instead of converting to a manifest list when calling imagetools create on a single source Ad --prefer-index flag forimagetools create on a single source May 31, 2024
@tonistiigi tonistiigi changed the title Ad --prefer-index flag forimagetools create on a single source Add --prefer-index flag forimagetools create on a single source May 31, 2024
@tonistiigi tonistiigi merged commit 4549283 into docker:master May 31, 2024
104 checks passed
@crazy-max crazy-max added this to the v0.15.0 milestone May 31, 2024
@thompson-shaun thompson-shaun added the kind/bug Something isn't working label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working status/code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants