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

Implement crane index subcommand #1561

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

jonjohnsonjr
Copy link
Collaborator

crane index filter allows platform-based filtering of manifests in an index. This is primarily useful for folks who want to use only some entries in a base image without having to copy irrelevant platforms.

crane index append allows appending manifests to an existing index or creating a new index from scratch.

Fixes #1144
Fixes #1143
Fixes #1557

See also:

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Merging #1561 (9b77df4) into main (e04520b) will increase coverage by 0.15%.
The diff coverage is 82.14%.

@@            Coverage Diff             @@
##             main    #1561      +/-   ##
==========================================
+ Coverage   72.75%   72.91%   +0.15%     
==========================================
  Files         118      118              
  Lines        9236     9408     +172     
==========================================
+ Hits         6720     6860     +140     
- Misses       1831     1856      +25     
- Partials      685      692       +7     
Impacted Files Coverage Δ
pkg/v1/config.go 31.57% <0.00%> (-43.43%) ⬇️
pkg/v1/remote/write.go 62.84% <62.50%> (+0.15%) ⬆️
pkg/v1/remote/descriptor.go 72.34% <67.56%> (-1.51%) ⬇️
internal/cmd/edit.go 54.12% <85.00%> (+0.64%) ⬆️
pkg/registry/manifest.go 94.04% <100.00%> (+1.83%) ⬆️
pkg/registry/registry.go 100.00% <100.00%> (ø)
pkg/v1/platform.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tianon
Copy link
Contributor

tianon commented Feb 8, 2023

I gave Jon badly worded feedback that made him split the one command he'd created into two separate commands 😭

"filter" for something that mutates seems like an odd word choice - his original choice was crane amend, and I thought that was potentially a large source of user confusion when it'll appear right next to crane append (and "append" vs "amend" is not great - they're very related words but the two commands will do very different things and even operate on entirely different sets of objects)

IMO something like crane index amend would be totally reasonable, but it feels a little weird next to crane manifest, crane config, and crane blob that are all single-object-type getters and it feels like overkill to crane index just for a single subcommand so I don't know what to recommend 😞

I'm sorry I made this harder Jon 😭 ❤️ 🙇

@imjasonh
Copy link
Collaborator

imjasonh commented Feb 8, 2023

Don't beat yourself up @tianon 😄, I recommended index filter and index append because they feel like two separate operations to me. One removes manifests from an index, one adds them.

I think I like having crane index as a place to do index-specific operations. A lot of crane implicitly deals with images (pull, append, export), but I don't think it's worth having a crane image sub-tree. Agnostic stuff like manifest, digest, blob also make sense at the root and should stay there.

Since index append is different than (image) append, I think that's a good place to start filling out an index-specific sub-command.

cmd/crane/cmd/util.go Show resolved Hide resolved
cmd/crane/cmd/util.go Show resolved Hide resolved
pkg/v1/platform.go Show resolved Hide resolved
crane index filter allows platform-based filtering of manifests in an
index. This is primarily useful for folks who want to use only some
entries in a base image without having to copy irrelevant platforms.

crane index append allows appending manifests to an existing index or
creating a new index from scratch.
There is some special handling around "all" that I left in place, but at
least we can drop some of this code.
@jonjohnsonjr
Copy link
Collaborator Author

@thesayyn FYI, would be interested in your feedback.

I'm not sure if I'm ready to go full-on CEL integration :P

@jonjohnsonjr jonjohnsonjr merged commit 2ceebaa into google:main Feb 8, 2023
@jonjohnsonjr jonjohnsonjr deleted the copy-platforms branch February 8, 2023 22:03
@thesayyn
Copy link
Collaborator

sorry, I was off last week due to the earthquake. this looks as good as it could get without having a DSL or using CEL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants