Add remote.Descriptor #408
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This resolves a mismatch between how registries actually work and our Image/ImageIndex interfaces. Since we don't know what type of artifact a registry stores, we need some kind of union type to represent either of them. This allows clients to reason about what type of artifact they got back from the registry, and efficiently "cast" it to the right implementation. This removes the need to do fallback. remote.Descriptor embeds a v1.Descriptor, so it contains MediaType, Size, and Digest, but it also has a Manifest field so we can access the raw response from the registry. This is useful for crane digest and crane manifest, where we dont' want to interpret the response, just show what's there. The remote.Image and remote.Index functions have been rewritten to use remote.Descriptor under the hood. One observable side-effect of this is that these functions will fail earlier than before. Whereas we used to just perform the token handshake in e.g. `remote.Image(ref)`, now we fetch the manifest (potentially recursively). In my opinion, this is a good thing. It's better to fail early than later. One thing this fixes is that e.g. crane push will fail with a message about pulling instead of pushing, since we'll fail before we try to call remote.Write. This is better for debugging. Doing this eager manifest fetching also allows us to return a better error message in the case of schema 1 images. There are downsides to this, e.g. passing around collection of v1.Image (before calling (Image.RawManifest)) will take up more memory (the size of the manifest, just a few KB). That really only affects tarball.MultiWriteToFile, but the difference should be negligble.
remote/image.go -> remote/descriptor.go
This consolidates the remoteImage and remoteIndex logic such that it
goes through a remote.Descriptor, which does appropriate type checking
and error handling in one place.
Doing so required a bit of shuffling around, which made the ImageIndex
-> Image resolution a lot cleaner, and enabled code reuse for the
v1.ImageIndex.{Image,ImageIndex} methods for accessing child artifacts.
Skip digest checks if we get back a signed schema 1 manifest, since we'd
have to remove the signatures to get the digest to match.
Cover new remote.Get paths and refactored index paths.
This uses remote.Descriptor, so it's now more accurate, in particular, both commands will now do the expected thing for manifests, manifests lists, and even schema 1 images, since we just return what we got back from the registry without trying to parse it as a v1.Image or v1.ImageIndex. Potentially this could break "crane digest" if users expect it to do image resolution first, but downstream tools should know how to resolve manifest lists anyway.
Remove the fallback logic in favor of remote.Get.
Codecov Report
@@ Coverage Diff @@
## master #408 +/- ##
==========================================
+ Coverage 53.6% 54.27% +0.66%
==========================================
Files 85 86 +1
Lines 4022 4074 +52
==========================================
+ Hits 2156 2211 +55
+ Misses 1531 1525 -6
- Partials 335 338 +3
Continue to review full report at Codecov.
|
imjasonh
approved these changes
Mar 15, 2019
This was referenced Mar 18, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This resolves a mismatch between how registries actually work and our
Image/ImageIndex interfaces. Since we don't know what type of artifact a
registry stores, we need some kind of union type to represent either
of them. This allows clients to reason about what type of artifact they
got back from the registry, and efficiently "cast" it to the right
implementation. This removes the need to do fallback.
remote.Descriptor embeds a v1.Descriptor, so it contains MediaType,
Size, and Digest, but it also has a Manifest field so we can access
the raw response from the registry. This is useful for crane digest and
crane manifest, where we dont' want to interpret the response, just show
what's there.
The remote.Image and remote.Index functions have been rewritten to use
remote.Descriptor under the hood. One observable side-effect of this is
that these functions will fail earlier than before. Whereas we used to
just perform the token handshake in e.g.
remote.Image(ref), now wefetch the manifest (potentially recursively). In my opinion, this is a
good thing. It's better to fail early than later. One thing this fixes
is that e.g. crane push will fail with a message about pulling instead
of pushing, since we'll fail before we try to call remote.Write. This is
better for debugging.
Doing this eager manifest fetching also allows us to return a better
error message in the case of schema 1 images.
There are downsides to this, e.g. passing around collection of v1.Image
(before calling (Image.RawManifest)) will take up more memory (the size
of the manifest, just a few KB). That really only affects
tarball.MultiWriteToFile, but the difference should be negligble.
Fixes #388
Closes #377
I tacked on changes to relevant consumers in crane and added some
schema 1 checks (since it's easier to do now). I can split those out into
separate PRs if needed.