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 <[email protected]>
dmcgowan
force-pushed
the
fix-manifest-list-size
branch
from
916dafc to
1fd2d66
Compare
clnperez
left a comment
clnperez
left a comment
There was a problem hiding this comment.
everything looks good and seem to work fine
| } | ||
| 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
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
left some questions / notes, and saw there were some outstanding nits; I'm ok with doing those in a follow up though
LGTM
| 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.
7 participants
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