Conversation
No change in behavior. These functions are guaranteed-cached versions of the same method in types.ImageSource. Both will be needed for signature policy evaluation, and the symmetry with ImageSource is nice. Also replaces the equivalent RawManifest method, preferring to keep the same naming convention as types.ImageSource.
This will be necessary for signature verification and related policy evaluation in the future.
| // GetManifest is like ImageSource.GetManifest, but the result is cached; it is OK to call this however often you need. | ||
| GetManifest() ([]byte, error) | ||
| // GetSignatures is like ImageSource.GetSignatures, but the result is cached; it is OK to call this however often you need. | ||
| GetSignatures() ([][]byte, error) |
There was a problem hiding this comment.
nit, but I don't like Get(er) or setter in Go - calling image.Signatures is pretty straightforward and clean WDYT?
There was a problem hiding this comment.
alright, I've seen the comment in commit message - yet I prefer not to use Get|Set - is there anything specific that needs this Get|Set prefix?
There was a problem hiding this comment.
This is valid also for Put (which recall me of HTTP for instance) - is it ok to use something like UploadManifest? SendManifest? I like it much more
There was a problem hiding this comment.
For now the immediate barrier is that there already is a Manifest method. But #58 proposes renaming it to Inspect anyway…
Dropping the Get prefix is perfectly fine with me.
As for Put, Upload/Send are a bit weird with dirImageDestination; and generally ImageDestination may have ordering dependencies, e.g. see how openshiftImageDestination in https://github.com/mtrmac/skopeo/tree/openshift-annotation-sigs only stores signatures locally in PutSignatures and uploads them only in PutManifest. Upload/Send could be misleading in such a case — but honestly the whole ImageSource/ImageDestination will need to be rethought if we ever support non-seekable tarball streams.
There was a problem hiding this comment.
For now the immediate barrier is that there already is a Manifest method. But #58 proposes renaming it to Inspect anyway…
this is indeed awesome and what I like
There was a problem hiding this comment.
I'm fine merging this as-is given we could remove at least Get (#57 (comment))
There was a problem hiding this comment.
but honestly the whole ImageSource/ImageDestination will need to be rethought if we ever support non-seekable tarball streams.
agree, maybe they could be embedded directly in images and are available only as a mean of developing new internal source|destination exposed by types.Image directy
There was a problem hiding this comment.
I do think that the source/destination distinction is useful; but for sources, this PR already goes a long way towards most users using a types.Image and not caring about ImageSource at all (basically unless you need layer contents, in which case tarball streams and ordering etc. are really important, Image gives you the same information and the caching and parsing makes it the preferred interface.).
|
apart from my naming conventions nit - this LGTM |
|
See #58 for more context. |
|
Merging, will defer #57 (comment) to #58 |
2 participants
This adds methods useful for validating signatures to
types.Image, namely the ability to retrieve signatures, and the supposedly signed identity, and renamesRawManifestfor consistency.The implementation remains Docker-specific (in particular, not usable for OpenShift), but having the type and API will allow the policy evaluation code to compile, which is on my critical path now.
See comments in the individual commits for details.