Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: implement artifact manifest type #29

Merged
merged 5 commits into from
Oct 27, 2022
Merged

feat!: implement artifact manifest type #29

merged 5 commits into from
Oct 27, 2022

Conversation

wangxiaoxuan273
Copy link

@wangxiaoxuan273 wangxiaoxuan273 commented Oct 20, 2022

Implemented ArtifactManifest and related structs and functions. Unit tests included.

Part 2 of #21

Signed-off-by: wangxiaoxuan273 [email protected]

Signed-off-by: wangxiaoxuan273 <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #29 (2f5ef79) into main (f71877f) will decrease coverage by 0.02%.
The diff coverage is 54.05%.

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
- Coverage   57.18%   57.15%   -0.03%     
==========================================
  Files         105      106       +1     
  Lines       10842    10914      +72     
==========================================
+ Hits         6200     6238      +38     
- Misses       3954     3981      +27     
- Partials      688      695       +7     
Impacted Files Coverage Δ
manifest/manifestlist/manifestlist.go 69.56% <25.00%> (-1.87%) ⬇️
manifest/ocischema/manifest.go 70.76% <25.00%> (-3.43%) ⬇️
manifest/ociartifact/artifact_manifest.go 57.57% <57.57%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about create a new package named manifest/ociartifact since each type of manifest should have its own package? See https://github.com/oras-project/distribution/tree/main/manifest

@wangxiaoxuan273
Copy link
Author

How about create a new package named manifest/ociartifact since each type of manifest should have its own package? See https://github.com/oras-project/distribution/tree/main/manifest

Done.

Signed-off-by: wangxiaoxuan273 <[email protected]>
Signed-off-by: wangxiaoxuan273 <[email protected]>
@wangxiaoxuan273 wangxiaoxuan273 changed the title feat!: implemented artifact manifest type feat!: implement artifact manifest type Oct 24, 2022
manifest/manifestlist/manifestlist.go Outdated Show resolved Hide resolved
manifest/ocischema/manifest.go Outdated Show resolved Hide resolved
}

// ArtifactManifest defines an ocischema artifact manifest.
type ArtifactManifest struct {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why don't we use v1.Artifact?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this struct needs to satisfy the manifest interface, also there are some compatibility issues as v1.Artifact uses oci.Descriptor but distribution uses distribution.Descriptor.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fair. Then there're a few disparities between the 2 structs:

  • artifactType is a required field, so we don't need omitempty
  • Better use *distribution.Descriptor for the optional subject

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I read the latest oci artifact manifest spec, artifactType is actually SHOULD.

Copy link
Author

@wangxiaoxuan273 wangxiaoxuan273 Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed subject to pointer after offline discussion. This gives func (m Manifest) References() a better design since it's much easier to decide whether it is nil. Thanks Wei!

manifest/ociartifact/artifact_manifest.go Outdated Show resolved Hide resolved
@wangxiaoxuan273
Copy link
Author

wangxiaoxuan273 commented Oct 27, 2022

I think a builder struct (present for ocischema manifest) will be needed for artifact manifest, so that we can enforce some rules on artifact manifest creation. I'll add it in future when necessary, for now I'll just create artifact manifest by hand.

Signed-off-by: wangxiaoxuan273 <[email protected]>
Copy link

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shizhMSFT shizhMSFT merged commit 0fce2c9 into oras-project:main Oct 27, 2022
@wangxiaoxuan273 wangxiaoxuan273 deleted the artifact-manifest-type branch October 28, 2022 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants