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

crane push: Support OCI layout #1208

Merged
merged 7 commits into from
Dec 21, 2021
Merged

crane push: Support OCI layout #1208

merged 7 commits into from
Dec 21, 2021

Conversation

jonjohnsonjr
Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr commented Dec 16, 2021

Detect if the first argument is a directory. If so, attempt to read it
as an OCI image layout and push to the destination if it contains a
single image.

Fixes #726
Fixes #1116

Detect if the first argument is a directory. If so, attempt to read it
as an OCI image layout and push to the destination if it contains a
single image.
@mattmoor
Copy link
Collaborator

nice! Will TAL.

cc @dlorenc @priyawadhwa

Comment on lines 71 to 74
if len(m.Manifests) != 1 {
return nil, errors.New("pushing multiple images from layout not yet supported (PRs welcome)")
}
return l.Image(m.Manifests[0].Digest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Haven't you marked this as Fixes: #{the issue covering pushing layouts}?

Maybe we should leave one open

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are ~3 ways I can imagine people wanting to use this, let's chat.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we resolve the next steps here? I'm fine with landing this part as-is, assuming it's good enough for kaniko's --oci-layout. However, I'd like a clear path for how we support the same for ko.

cmd/crane/cmd/push.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Can you add a little roundtrip e2e test where we crane pull -> crane push -> crane pull and check for diffs?

For extra credit, I'd love to check that it works with https://github.com/GoogleContainerTools/kaniko/blob/master/README.md#--oci-layout-path

@mattmoor
Copy link
Collaborator

I think the main thing I'd want to see to land this is a roundtrip test, but otherwise it LGTM. I'd even be fine if we plan to change the roundtrip test as the semantics of how we handle single images evolves later, I mostly want the confidence that things WAI.

@mattmoor
Copy link
Collaborator

Alternately (or additionally), a test that runs kaniko --oci-layout -> crane push would be 🤩

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2021

Codecov Report

Merging #1208 (52f7261) into main (ab77ea6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1208   +/-   ##
=======================================
  Coverage   74.55%   74.55%           
=======================================
  Files         111      111           
  Lines        8048     8048           
=======================================
  Hits         6000     6000           
  Misses       1464     1464           
  Partials      584      584           

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 ab77ea6...52f7261. Read the comment docs.

Make sure we can roundtrip images through an OCI layout.
cmd/crane/cmd/push.go Outdated Show resolved Hide resolved
},
}
cmd.Flags().BoolVar(&index, "index", false, "Push the collection of images as a single index")
Copy link
Collaborator

Choose a reason for hiding this comment

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

My interpretation of this text is that the fallback behavior is to push N images instead, which is not what the code does, so perhaps some clarity is called for there?

Use type switch.

Elaborate in description of --index flag.
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.

Add support for OCI layouts in crane pull/push crane push doesn't support OCI image tarballs
4 participants