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: index referrers for image manifests #41

Merged
merged 5 commits into from
Nov 25, 2022
Merged

feat: index referrers for image manifests #41

merged 5 commits into from
Nov 25, 2022

Conversation

wangxiaoxuan273
Copy link

@wangxiaoxuan273 wangxiaoxuan273 commented Nov 23, 2022

Part 8 of issue #21
Part 1 of issue #39

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 with suggestions.

@@ -51,6 +51,10 @@ type Manifest struct {
// configuration.
Layers []distribution.Descriptor `json:"layers"`

// Subject specifies the descriptor of another manifest. This value is
// used by the referrers API.
Subject *distribution.Descriptor `json:"subject,omitempty"`

Choose a reason for hiding this comment

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

Do you want to also update the Reference() function?

Copy link
Author

Choose a reason for hiding this comment

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

added subject in Reference() and changed the verifyManifest function accordingly.

@@ -141,3 +164,20 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc

return nil
}

// indexReferrers indexes the subject of the given revision in its referrers index store.
func (ms *ocischemaManifestHandler) indexReferrers(ctx context.Context, dm *ocischema.DeserializedManifest, revision digest.Digest) error {

Choose a reason for hiding this comment

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

This is probably OK since oras/distribution is for testing purpose. It will be better if it is refactored since it duplicated with this function.

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

codecov-commenter commented Nov 24, 2022

Codecov Report

Merging #41 (1c15e35) into main (7c54a46) will decrease coverage by 0.01%.
The diff coverage is 60.71%.

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
- Coverage   57.18%   57.16%   -0.02%     
==========================================
  Files         107      107              
  Lines       11021    11041      +20     
==========================================
+ Hits         6302     6312      +10     
- Misses       4011     4019       +8     
- Partials      708      710       +2     
Impacted Files Coverage Δ
manifest/ocischema/manifest.go 70.76% <0.00%> (-3.43%) ⬇️
registry/storage/ocimanifesthandler.go 64.00% <38.46%> (-3.05%) ⬇️
manifest/ocischema/builder.go 67.27% <100.00%> (+1.23%) ⬆️
registry/storage/ociartifactmanifesthandler.go 58.82% <100.00%> (+0.99%) ⬆️
registry/storage/registry.go 89.04% <100.00%> (+0.05%) ⬆️

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

@@ -109,7 +118,7 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc
}
}

case v1.MediaTypeImageManifest:
case v1.MediaTypeImageManifest, v1.MediaTypeArtifactManifest, v1.MediaTypeImageIndex, schema2.MediaTypeManifest:

Choose a reason for hiding this comment

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

how about docker manifest list?

Copy link
Author

Choose a reason for hiding this comment

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

Added manifest list and schema1 manifest.

Signed-off-by: wangxiaoxuan273 <[email protected]>
@@ -118,7 +120,7 @@ func (ms *ocischemaManifestHandler) verifyManifest(ctx context.Context, mnfst oc
}
}

case v1.MediaTypeImageManifest, v1.MediaTypeArtifactManifest, v1.MediaTypeImageIndex, schema2.MediaTypeManifest:
case v1.MediaTypeImageManifest, v1.MediaTypeArtifactManifest, v1.MediaTypeImageIndex, schema1.MediaTypeManifest, schema2.MediaTypeManifest, manifestlist.MediaTypeManifestList:

Choose a reason for hiding this comment

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

We should remove schema1 as it is deprecated.

Copy link
Author

Choose a reason for hiding this comment

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

removed.

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 ad70be1 into oras-project:main Nov 25, 2022
@wangxiaoxuan273 wangxiaoxuan273 deleted the image-referrers branch November 25, 2022 06:02
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.

3 participants