Fix manifest lists to always use correct size #1156
Conversation
dmcgowan
force-pushed
the
fix-manifest-list-size
branch
from
903562f
to
be04d8d
Compare
| blobReqs = append(blobReqs, manifestBlob{canonical: canonical, os: imageManifest.Platform.OS}) | ||
| var os string | ||
| if imageManifest.Descriptor.Platform != nil { | ||
| os = imageManifest.Descriptor.Platform.OS |
There was a problem hiding this comment.
nit: this would be better as requireMount bool
cli/manifest/types/types.go
Outdated
| img := &Image{} | ||
| if err := json.Unmarshal(src, img); err != nil { | ||
| // PlatformFromJSON returns an OCI platform from formatted JSON bytes | ||
| func PlatformFromJSON(src []byte) (*ocispec.Platform, error) { |
There was a problem hiding this comment.
nit: no need for this function if called only once
dmcgowan
force-pushed
the
fix-manifest-list-size
branch
from
be04d8d
to
916dafc
Compare
Stores complete OCI descriptor instead of digest and platform fields. This includes the size which was getting lost by not storing the original manifest bytes. Attempt to support existing cached files, if not output the filename with the incorrect content. Signed-off-by: Derek McGowan <derek@mcgstyle.net>
dmcgowan
force-pushed
the
fix-manifest-list-size
branch
from
916dafc
to
1fd2d66
Compare
| @@ -195,7 +199,11 @@ func buildBlobRequestList(imageManifest types.ImageManifest, repoName reference. | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| blobReqs = append(blobReqs, manifestBlob{canonical: canonical, os: imageManifest.Platform.OS}) | |||
| var os string | |||
| if imageManifest.Descriptor.Platform != nil { | |||
There was a problem hiding this comment.
since this is called after buildManifestList, this should never be nil
| if imageManifest.Platform.Architecture == "" || imageManifest.Platform.OS == "" { | ||
| if imageManifest.Descriptor.Platform == nil || | ||
| imageManifest.Descriptor.Platform.Architecture == "" || | ||
| imageManifest.Descriptor.Platform.OS == "" { |
There was a problem hiding this comment.
Wonder if this validation should be inside buildManifestDescriptor(), but I assume we can't move it there because that's also used by manifest inspect?
| } | ||
|
|
||
| // Compatibility with image manifests created before | ||
| // descriptor, newer versions omit Digest and Platform |
There was a problem hiding this comment.
Compatibility here is only for the manifests created by earlier versions of docker manifest ("experimental") correct?
If so, we should probably add a TODO to remove this code at some point
| } | ||
| } | ||
|
|
||
| // PlatformSpecFromOCI creates a platform spec from OCI platform |
There was a problem hiding this comment.
Wondering if we somehow can get rid of having both types (given that they're effectively the same)
There was a problem hiding this comment.
Using the containerd code instead of the distribution code would effectively get rid of this.
Stores complete OCI descriptor instead of digest and platform fields. This includes the size which was getting lost by not storing the original manifest bytes.
Attempt to support existing cached files, if the original byte content cannot be retrieved output the filename with the incorrect content. This allows the user to remove the file and re-attempt to create their manifest list.
Note: I think before taking this out of experimental we should just update this tool to either use containerd or the content/image store from containerd.
Fixes #1135
Replaces #1144
ping @flx42 @clnperez