Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
imagetools: Allow annotations for OCI image index #1965
imagetools: Allow annotations for OCI image index #1965
Changes from all commits
18894a8
3ef93e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a regexp here, wdyt about using https://github.com/moby/buildkit/blob/dd0053cdce470b1355fdb0bd5a8f2b0fc506d842/exporter/containerimage/exptypes/annotations.go#L85 instead?
Obviously, it's not perfect, since we'd need to add the
annotation-
prefix here, which might make the error message a bit odd, but we could always upstream a buildkit fix later that would rework the function to have the caller remove the prefix there (so we wouldn't have to do that).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I had the same idea but we are using
:
as a separator fortype
and annotation key compared to.
expected here. So it needs more work than just appendingannotation-
prefix.Does it make sense to make buildkit parser to also handle
:
separator. It sounded specific to buildx so I feel we should keep it here. wdyt?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'd prefer the conversion to buildkit's form for now, so that we only have one source of truth for parsing these keys - I'm happy to follow up in buildkit to rework the logic to be a bit more reusable for this case.
I also just noticed (sorry), that we aren't handling platforms?
manifest-descriptor
supports a platform, and should only be attached to the descriptor for those platforms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. That should be the way moving forward. I added a comment regrading using buildkit once it supports our use-case here.
Great Catch. Done.