-
Notifications
You must be signed in to change notification settings - Fork 5
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: support referrers for oci image manifests #43
Conversation
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Prettified result:
|
registry/handlers/referrers.go
Outdated
func getArtifactReferrers(ctx context.Context, | ||
blobStatter distribution.BlobStatter, | ||
referrerDigest digest.Digest, | ||
man distribution.Manifest, | ||
referrers *[]v1.Descriptor, | ||
artifactType string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the code pattern
func doSomething(result *[]any)
Try golang-style
func doSomething(result []any) []any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, you don't even need to pass the referrers
. All you want to return is a referrer descriptor.
Think about
func generateReferrerFromArtifact(ctx context.Context, blobStatter distribution.BlobStatter, referrerDigest digest.Digest, man *ociartifact.DeserializedManifest, artifactType string) (v1.Descriptor, error)
You could apply the same thing to getImageReferrers
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Changed accordingly.
registry/handlers/referrers.go
Outdated
func getArtifactReferrers(ctx context.Context, | ||
blobStatter distribution.BlobStatter, | ||
referrerDigest digest.Digest, | ||
man distribution.Manifest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
man distribution.Manifest, | |
man *ociartifact.DeserializedManifest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed accordingly.
registry/handlers/referrers.go
Outdated
case *ociartifact.DeserializedManifest: | ||
return getArtifactReferrers(ctx, blobStatter, referrerDigest, man, &referrers, artifactType) | ||
default: | ||
return fmt.Errorf("referrers not supported for this manifest type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it return nil
to keep the previous behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed accordingly.
registry/handlers/referrers.go
Outdated
return nil | ||
} | ||
|
||
func getImageReferrers(ctx context.Context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Image Manifest, the media type of the config is used as the artifact type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added filtering with config media type.
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #43 +/- ##
==========================================
- Coverage 56.48% 56.31% -0.18%
==========================================
Files 108 108
Lines 11176 11211 +35
==========================================
Hits 6313 6313
- Misses 4153 4188 +35
Partials 710 710
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Resolves #39
Part 9 of #21
Signed-off-by: wangxiaoxuan273 wangxiaoxuan119@gmail.com