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 --tarball to crane digest #896

Merged
merged 1 commit into from
Apr 16, 2021
Merged

Conversation

jonjohnsonjr
Copy link
Collaborator

Fixes #895

@codecov-io
Copy link

Codecov Report

Merging #896 (c838fd6) into master (ec3c7a5) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
- Coverage   72.53%   72.46%   -0.08%     
==========================================
  Files         108      108              
  Lines        4672     4677       +5     
==========================================
  Hits         3389     3389              
- Misses        773      778       +5     
  Partials      510      510              
Impacted Files Coverage Δ
pkg/crane/push.go 54.54% <0.00%> (-45.46%) ⬇️

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 ec3c7a5...c838fd6. Read the comment docs.

@aelij
Copy link

aelij commented Jan 3, 2021

Can you make the required tag argument optional? Our tarballs have "RepoTags": null in their manifest.

I've made this change and tested it - it works as expected (crane push and crane digest produce the same digest):

img, err := crane.Load(tarball)

Base automatically changed from master to main January 27, 2021 18:57
@aelij
Copy link

aelij commented Jan 28, 2021

@jonjohnsonjr Do you want me to finish this? I can create a sub-command as you suggested.

@jonjohnsonjr
Copy link
Collaborator Author

jonjohnsonjr commented Jan 28, 2021

Ah I just forgot about it completely. I have some plans to add another crane layout subcommand for doing layout-y things, so adding a crane tarball command makes some sense to me, but I haven't thought it through.

At the same time, it's a bit weird to duplicate all the top-level commands a bunch of times instead of just having flags.

The main thing pushing me towards subcommands here is that the cobra stuff for parsing flags and args will be different depending on if you're interacting with a tarball, layout, or registry.

The other option would be overloading the reference string with schemes, e.g. tarball:// or layout://... it doesn't make sense to have both --tarball and --layout flags, this is kind of nice... also, anything that crane can do remotely still makes sense to do against a tarball or a layout, so I'm leaning this way.

It makes a lot of sense to support append, blob, config, copy, delete, digest, export, ls, manifest, rebase, tag, and validate for a tarball or layout. We could make catalog make a little bit of sense, but it would be a little weird...

@imjasonh what do you think about this?

# Roughly, `$ tar -Oxf ubuntu.tar manifest.json | jq .[].RepoTags | jq .[] -r`
$ crane ls tarball://ubuntu.tar
ubuntu

# Works for len(tarball) == 1
$ crane [digest|config|manifest] tarball://ubuntu.tar

# For len(tarball) > 1
# Not sure about the separator? How do we handle tarballs with `:` in the filename?
$ crane [digest|config|manifest] tarball://ubuntu.tar:latest

Maybe we need a -t flag for these? That makes me thing we should do a separate subcommand, but it's weird...

Similarly, for layout, we'd need some selector -- we could use -t and assume the OCI name annotation?

For layout, there are other commands that are interesting, like GC (see https://github.com/opencontainers/umoci). This could also apply to tarball, I guess... gcrane has a gc command, but it's a bit different (sorry).

tl;dr three options:

  1. Flags, manual arg parsing
  2. Schemes
  3. Subcommands

Not sure which we want to go with. If we go the scheme route, we might want to adopt skopeo's conventions.

@aelij
Copy link

aelij commented Apr 11, 2021

Ping :)
My vote is for the schemes approach. Makes all the commands simple to use.
Meanwhile, could merge the current PR with this minor fix to unblock us?

img, err := crane.Load(tarball)

@jonjohnsonjr
Copy link
Collaborator Author

Ended up just finessing this with some manual flag logic. If we want to do something fancier in the future, we can revisit this.

I should probably provide a big warning about how this doesn't do what most people would expect, but we can do that if it becomes an issue.

cmd/crane/cmd/digest.go Outdated Show resolved Hide resolved
If specified, tag can be omitted for single-image tarballs.
@codecov-commenter
Copy link

Codecov Report

Merging #896 (d2c9843) into main (7e30746) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head d2c9843 differs from pull request most recent head cb59bb1. Consider uploading reports for the commit cb59bb1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   74.52%   74.61%   +0.09%     
==========================================
  Files         107      107              
  Lines        5020     5027       +7     
==========================================
+ Hits         3741     3751      +10     
+ Misses        723      721       -2     
+ Partials      556      555       -1     
Impacted Files Coverage Δ
pkg/crane/push.go 100.00% <100.00%> (ø)
pkg/v1/daemon/write.go 46.15% <0.00%> (+11.53%) ⬆️

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 7e30746...cb59bb1. Read the comment docs.

@imjasonh imjasonh merged commit 549ee62 into google:main Apr 16, 2021
@aelij
Copy link

aelij commented Apr 21, 2021

Thanks all

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.

Calculate digest from tarball
5 participants