Conversation
Does not change behavior. This will allow us to move collecting some of the data to the (skopeo inspect) code and to have a more focused types.Image API, where types.Image.Manifest() does not return a grab bag of manifest-unrelated data, eventually. For how it actually makes the coupling more explicit by having types.Image.Manifest() return a types.DockerImageManifest instead of the too generic types.ImageManifest. We will need to think about which parts of DockerImageManifest are truly generic, later.
Compute the digest ourselves, the registry is in general untrusted and computing it ourserlves is easy enough. The stop passing the unverifiedCanonicalDigest value around, simplifying ImageSource.GetManifest and related code. In particular, remove retrieveRawManifest and have internal users just call Manifest() now that we don't need the digest.
…ge.GetRepositoryTags This does not change behavior. Splits listing of repository tags, which is not a property of an image, from the image.Manifest gathering of information about an image.
This does not change behavior. Rename types.DockerImageManifest to types.ImageInspectInfo. This naming more accurately reflects what the function does and how it is expected to be used. (The only outstanding non-inspection piece is the Name field, which is kind of a subset of GetIntendedDockerReference() right now. Not sure whether that is intentional.) Also fold makeImageManifest into its only user.
|
Rebased on top of the removal of |
| if err != nil { | ||
| logrus.Fatalf("Error computing manifest digest: %s", err.Error()) | ||
| } | ||
| repoTags, err := img.GetRepositoryTags() |
There was a problem hiding this comment.
I agree with you this does smell here... (I mean this tag method on the generic image interface)
There was a problem hiding this comment.
This improves the interface at least a little. How to do it truly cleanly and in a generic way, without breaking existing users (if any?) is not clear to me yet; I was thinking
if di, ok := img.(DockerImage); ok {
tags = di.GetDockerRepositoryTags()
} else {
tags = nil;
}as another small step towards kicking this out of types.Image. Eventually; this is not really hurting me :)
There was a problem hiding this comment.
That type assertion sounds fine to me and it's clean (tags can be omitemtpy when/if marshaling also)
There was a problem hiding this comment.
We will eventually need to generalize dockerImage to work with openshiftImageSource, at that point the type assertion will make sense, because we could test the tags = nil path.
Or do you want it done immediately so that the GetRepositoryTags method is never added to types.Image?
There was a problem hiding this comment.
I'm fine doing it later (hopefully we would freeze the API in the coming weeks so we are still more time to think more about this :)
We can leave a comment though just in case we forgot about doing this larer
There was a problem hiding this comment.
Added a comment pointing to this thread, within the “Add comments on the use of the API and the general direction” commit.
|
LGTM (given #58 (comment)) |
2 participants
This is a follow-up to #57, I will rebase as needed.
This somewhat cleans up
types.Imagetowards becoming the primary inspection endpoint for images:skopeo inspectoutput formatImage.Manifestturns intoImage.Inspect, which parses the manifest, config and layers only; its other former functions are split into new methods.This does not yet completely clean up the API (e.g. moving parts of
Layerstocmd/skopeo).Nor does this make
Imageindependent of the underlying Docker implementation yet: (InspectInfo.Namereturns the parsed Docker ref, andGetRepositoryTagsneeds a Docker client). Non-Docker replacements for this mostly depend on what values we defineskopeo inspectto return for non-Docker sources.See comments in the individual commits for details.