Skip to content

feat: index referrers when pushing manifests#35

Merged
shizhMSFT merged 5 commits intooras-project:mainfrom
wangxiaoxuan273:index-referrers
Nov 17, 2022
Merged

feat: index referrers when pushing manifests#35
shizhMSFT merged 5 commits intooras-project:mainfrom
wangxiaoxuan273:index-referrers

Conversation

@wangxiaoxuan273
Copy link
Copy Markdown

@wangxiaoxuan273 wangxiaoxuan273 commented Nov 11, 2022

Part 6 of #21

This pr implements indexing referrers during manifest push. This is needed to enable the referrers API. It changes the registry/storage package. Based on the existing implementation on the rc2 branch.

Signed-off-by: wangxiaoxuan273 wangxiaoxuan119@gmail.com

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

// TODO: Should be removed and defined instead in paths package
// Requires paths package to be exported
func referrersLinkPath(name string) string {
Copy link
Copy Markdown
Author

@wangxiaoxuan273 wangxiaoxuan273 Nov 11, 2022

Choose a reason for hiding this comment

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

Need advice on how to handle these. Move them to the path package?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@wangxiaoxuan273 Yes, please move them to the path package and update the related documentation.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 11, 2022

Codecov Report

Merging #35 (9400f77) into main (6f0e3ea) will decrease coverage by 0.00%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   57.18%   57.18%   -0.01%     
==========================================
  Files         107      107              
  Lines       10990    11021      +31     
==========================================
+ Hits         6285     6302      +17     
- Misses       4001     4011      +10     
- Partials      704      708       +4     
Impacted Files Coverage Δ
registry/storage/paths.go 70.43% <50.00%> (-1.67%) ⬇️
registry/storage/ociartifactmanifesthandler.go 57.83% <56.25%> (-0.38%) ⬇️
registry/storage/registry.go 88.99% <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.

Comment on lines +136 to +137
rootPath := path.Join(referrersLinkPath(ms.repository.Named().Name()), subjectRevision.Algorithm().String(), subjectRevision.Hex())
referenceLinkPath := path.Join(rootPath, revision.Algorithm().String(), revision.Hex(), "link")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What would be proper path considering referrers is a first-class item like manifests, blobs?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How would it be impacted if we delete a repository?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also think about how artifact type can be supported.

Hint: Hashing will be useful for user input projection.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Currently, only PUT path is considered. How about the DELETE path in terms of indexing referrers?

Copy link
Copy Markdown
Author

@wangxiaoxuan273 wangxiaoxuan273 Nov 15, 2022

Choose a reason for hiding this comment

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

After offline discussion, I will address these issues in later prs. Issues #36, #37, #38 are created to keep track of them.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
//
// The path layout in the storage backend is roughly as follows:
//
// <root>/v2
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

All these formatting changes are auto-generated by the go formatter when saving the file.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Comment on lines +98 to +100
// Referrers:
//
// blobsPathSpec: <root>/v2/blobs/
// blobPathSpec: <root>/v2/blobs/<algorithm>/<first two hex bytes of digest>/<hex digest>
// blobDataPathSpec: <root>/v2/blobs/<algorithm>/<first two hex bytes of digest>/<hex digest>/data
// blobMediaTypePathSpec: <root>/v2/blobs/<algorithm>/<first two hex bytes of digest>/<hex digest>/data
// referrersLinkPathSpec: <root>/v2/repositories/<name>/_referrers/<algorithm>/<hex digest>/<subject algorithm>/<subject hex digest>/link
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

indentation issue.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed the indentation.

// hashstates/<algorithm>/<offset>
// -> blob/<algorithm>
// <split directory content addressable storage>
// <root>/v2
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

referrer related paths should also be here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Please check if line 43-45 are correct.

if err != nil {
return "", err
}
return path.Join(append(append(append(append(repoPrefix, v.name, "_referrers", "subjects"), revisionComponents...), subjectComponents...), "link")...), nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

4 x append do not seem right. Could you rewrite it to multiple lines?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Rewrote it.

// We cover the path formats implemented by this path mapper below.
//
// Manifests:
// Manifests:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should it just one tab?

Suggested change
// Manifests:
// Manifests:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed with one tab.

Comment on lines +137 to +140
if err := ms.storageDriver.PutContent(ctx, referrersLinkPath, []byte(revision.String())); err != nil {
return err
}
return nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit:

Suggested change
if err := ms.storageDriver.PutContent(ctx, referrersLinkPath, []byte(revision.String())); err != nil {
return err
}
return nil
return ms.storageDriver.PutContent(ctx, referrersLinkPath, []byte(revision.String()))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed accordingly.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>
Copy link
Copy Markdown

@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 7c54a46 into oras-project:main Nov 17, 2022
@wangxiaoxuan273 wangxiaoxuan273 deleted the index-referrers branch November 17, 2022 11:22
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