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

Don't annotate refs by default, switch to OCI key #1401

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

jonjohnsonjr
Copy link
Collaborator

A bunch of breaking changes that might be contentious, but I think this
is probably the right behavior.

  1. Stick the full image ref in the image (not just e.g. ubuntu) so it's
    clear that it's the image and not just a tag.
  2. Only do this when --annotate-ref is passed, so that these things are
    location-independent by default.
  3. Switch from a crane annotation key to the standard OCI key.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #1401 (34d2434) into main (2b1087a) will increase coverage by 0.04%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1401      +/-   ##
==========================================
+ Coverage   74.09%   74.14%   +0.04%     
==========================================
  Files         113      113              
  Lines        8481     8450      -31     
==========================================
- Hits         6284     6265      -19     
+ Misses       1587     1579       -8     
+ Partials      610      606       -4     
Impacted Files Coverage Δ
pkg/crane/pull.go 68.11% <50.00%> (-1.33%) ⬇️
pkg/v1/layout/write.go 48.08% <0.00%> (-0.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b1087a...34d2434. Read the comment docs.

return err
}
opts = append(opts, layout.WithAnnotations(map[string]string{
"org.opencontainers.image.ref.name": parsed.Name(),
Copy link
Collaborator

@imjasonh imjasonh Jun 29, 2022

Choose a reason for hiding this comment

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

https://github.com/opencontainers/image-spec/blob/main/image-layout.md says

No semantic restriction is given for the "org.opencontainers.image.ref.name" annotation of descriptors.

but also

Implementor's Note: A common use case of descriptors with a "org.opencontainers.image.ref.name" annotation is representing a "tag" for a container image. For example, an image may have a tag for different versions or builds of the software. In the wild you often see "tags" like "v1.0.0-vendor.0", "2.0.0-debug", etc. Those tags will often be represented in an image-layout repository with matching "org.opencontainers.image.ref.name" annotations like "v1.0.0-vendor.0", "2.0.0-debug", etc.

Despite the spec asserting otherwise, do tools that interact with layouts tend to interpret these as tags, and not full image refs?

If so this would have been a problem before too, just curious if we're moving even further from accepted behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's straightforward to distinguish a tag from a full ref (strings.Contains(v, "/")), so I feel like it should be okay to do this, but it could possibly break some stuff (which is why I don't want to do it by default anymore).

The grammar defined in https://github.com/opencontainers/image-spec/blob/main/annotations.md allows for a full ref, so I don't feel like we'd be doing anything particularly egregious, but @sudo-bmitch might disagree.

A bunch of breaking changes that might be contentious, but I think this
is probably the right behavior.

1. Stick the full image ref in the image (not just e.g. ubuntu) so it's
   clear that it's the image and not just a tag.
2. Only do this when --annotate-ref is passed, so that these things are
   location-independent by default.
3. Switch from a crane annotation key to the standard OCI key.
@jonjohnsonjr jonjohnsonjr merged commit 59b5c06 into google:main Jun 29, 2022
@jonjohnsonjr jonjohnsonjr deleted the keep-ref branch June 29, 2022 19:50
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

3 participants