feat!: implement artifact manifest handler in the storage package #30
Conversation
wangxiaoxuan273
changed the title
wangxiaoxuan273
changed the title
Signed-off-by: wangxiaoxuan273 <[email protected]>
Signed-off-by: wangxiaoxuan273 <[email protected]>
registry/storage/manifeststore.go
Outdated
| if versioned.MediaType == v1.MediaTypeArtifactManifest { | ||
| return ms.ociartifactHandler.Unmarshal(ctx, dgst, content) | ||
| } |
There was a problem hiding this comment.
Added mediaType check before the schema number check, according to our offline discussion.
There was a problem hiding this comment.
Should only check MediaType before switching and comment the spec link(https://github.com/opencontainers/image-spec/blob/v1.1.0-rc2/artifact.md) in the if block.
| err := descriptor.Digest.Validate() | ||
| if err != nil { | ||
| errs = append(errs, err, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest}) | ||
| continue | ||
| } | ||
| exists, err := manifestService.Exists(ctx, descriptor.Digest) | ||
| if err != nil || !exists { | ||
| errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest}) | ||
| } |
There was a problem hiding this comment.
This part does not look right. Descriptor of Subject refers to manifest but descriptors of Blobs refers to blobs.
There was a problem hiding this comment.
Corrected this part with blobService.
Signed-off-by: wangxiaoxuan273 <[email protected]>
Signed-off-by: wangxiaoxuan273 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 57.17% 57.21% +0.03%
==========================================
Files 106 107 +1
Lines 10892 10979 +87
==========================================
+ Hits 6228 6282 +54
- Misses 3972 3995 +23
- Partials 692 702 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
registry/storage/manifeststore.go
Outdated
| @@ -94,19 +96,25 @@ func (ms *manifestStore) Get(ctx context.Context, dgst digest.Digest, options .. | |||
| return nil, err | |||
| } | |||
|
|
|||
| switch versioned.MediaType { | |||
There was a problem hiding this comment.
To maintain the behavior of all previous implementation. I suggest
switch versioned.MediaType {
case v1.MediaTypeArtifactManifest:
return ms.ociartifactHandler.Unmarshal(ctx, dgst, content)
}
switch versioned.SchemaVersion {
// ... all code below should not be modified.If we do want to improve, it should be done in a separated PR after upstream merge.
| // TODO: we should be able to handle the case in which 'descriptor' only has Digest and no MediaType | ||
| // currently, if Digest is a manifest digest and MediaType is "", there will be an error. | ||
| switch descriptor.MediaType { | ||
| case v1.MediaTypeArtifactManifest, v1.MediaTypeImageManifest, v1.MediaTypeImageIndex, | ||
| schema1.MediaTypeManifest, schema2.MediaTypeManifest, manifestlist.MediaTypeManifestList: | ||
| exists, err := manifestService.Exists(ctx, descriptor.Digest) | ||
| if err != nil || !exists { | ||
| errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest}) | ||
| } | ||
| default: | ||
| _, err = blobsService.Stat(ctx, descriptor.Digest) | ||
| if err != nil { | ||
| errs = append(errs, distribution.ErrManifestBlobUnknown{Digest: descriptor.Digest}) | ||
| } | ||
| } |
There was a problem hiding this comment.
Media Type is a MUST field. Meanwhile, all descriptors in the blobs field are blobs. Even if they are manifests, they are still considered as blobs.
There was a problem hiding this comment.
Changed the unit test accordingly.
Signed-off-by: wangxiaoxuan273 <[email protected]>
Signed-off-by: wangxiaoxuan273 <[email protected]>
Implemented artifact manifest handler related changes for the storage package, unit tests included.
Part 3 of #21
Signed-off-by: wangxiaoxuan273 [email protected]