add go specs for artifact manifest - #17
Conversation
Signed-off-by: Aviral Takkar <aviral26@users.noreply.github.com>
SteveLasker
left a comment
There was a problem hiding this comment.
LGTM with the minor, but important change on the MediaTypeArtifactManifest
Signed-off-by: Aviral Takkar <aviral26@users.noreply.github.com>
SteveLasker
left a comment
There was a problem hiding this comment.
Some additional questions on why we need dependencies to the image-spec
|
|
||
| package v1 | ||
|
|
||
| import v1 "github.com/opencontainers/image-spec/specs-go/v1" |
There was a problem hiding this comment.
This seems correct to me - Artifact-spec doesn’t need to redefine the standard descriptor.
see line 32.
SubjectManifest v1.Descriptor json:"subjectManifest"
There was a problem hiding this comment.
Actually, this is the purpose fo the artifact-descriptor as it adds the artifactType property, and is a superset of the image-spec-descriptor.
There was a problem hiding this comment.
I would ideally not like to redefine structures that have already been well defined in other locations in projects we typically are comfortable taking dependency on. After discussions with @SteveLasker I think its important to call out this specific case which is that these are only spec files and image-spec repo is a subset of OCI which is more about container images (or process containers) and there is no reason for client library which wants to deal with signatures need to bring in image-spec into its modules as an indirect dependency.
The descriptor if at all there was should come from distribution-spec but since that is also not solidified and is under a different body (OCI) it is ok to just use the artifactDescriptor for both blobs and subjectManifest since the annotation fields are optional and anyone building clients don't have indirect deps on image-spec
There was a problem hiding this comment.
and is a superset of the image-spec-descriptor.
If it's intended to be a superset of the image-spec descriptor, you have a logical dependency on image-spec. If you're intending to fork the image-spec descriptor, that's fine, but it's not really a superset anymore if they can diverge.
There was a problem hiding this comment.
but it's not really a superset anymore if they can diverge.
That's fair. It's a superset of what's defined in the 1.0 of the image-spec. Having it defined in the artifacts-spec, or preferably in the distribution-spec would be to allow the image-spec maintainers to evolve the image-spec as they want without destabilizing artifacts-manifest consumers.
There was a problem hiding this comment.
So as artifact descriptors, blobs and subjectManifest properties will not support annotations?
There was a problem hiding this comment.
Removed the dependency on image-spec.
remove dependency on opencontainers/image-spec by defining a descriptor Signed-off-by: Aviral Takkar <aviral26@users.noreply.github.com>
This PR adds Go specs for ORAS Artifact manifest. Addresses #15.