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: support oras discover pass pagination args #1525

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

njucjc
Copy link
Contributor

@njucjc njucjc commented Nov 6, 2024

What this PR does / why we need it:
support oras discover pass pagination args

Please check the following list:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
  • Does this change require a documentation update?
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have an appropriate license header?

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.83%. Comparing base (0013d4c) to head (5660be4).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
cmd/oras/root/discover.go 42.85% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1525      +/-   ##
==========================================
- Coverage   85.59%   83.83%   -1.76%     
==========================================
  Files         113      118       +5     
  Lines        3936     5173    +1237     
==========================================
+ Hits         3369     4337     +968     
- Misses        340      594     +254     
- Partials      227      242      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@qweeah qweeah requested a review from FeynmanZhou November 6, 2024 09:31
@qweeah
Copy link
Contributor

qweeah commented Nov 6, 2024

Add @FeynmanZhou for UX-related discussion.

@shizhMSFT
Copy link
Contributor

@njucjc Could you create an issue for the motivation of this PR?

Adding the page size option won't change the UX but may change the number of request send to the server as well as the expected server load.

Note

The pagination is not defined in the OCI distribution spec for the Referrers API. oras-go has the ReferrerListPageSize field because the code is adapted from the original ORAS Artifact Spec.

/cc @Wwwsylvia

@shizhMSFT
Copy link
Contributor

shizhMSFT commented Nov 7, 2024

The pagination is not defined in the OCI distribution spec for the Referrers API.

In other words, it depends entirely on the remote repository whether it implements the pagination defined by the ORAS artifact specification rather than the OCI distribution specification.

@njucjc
Copy link
Contributor Author

njucjc commented Nov 7, 2024

The pagination is not defined in the OCI distribution spec for the Referrers API.

In other words, it depends entirely on the remote repository whether it implements the pagination defined by the ORAS artifact specification rather than the OCI distribution specification.

@shizhMSFT OCI distribution specification already defined the the pagination
image

@shizhMSFT
Copy link
Contributor

shizhMSFT commented Nov 7, 2024

@shizhMSFT OCI distribution specification already defined the the pagination

@njucjc Not quite right. The OCI distribution spec defines how Link header works but it does not define the n query parameter.

Note that the OCI distribution spec does define n for listing tags but not for listing referrers.

@Wwwsylvia
Copy link
Member

@njucjc Could you create an issue for the motivation of this PR?

Adding the page size option won't change the UX but may change the number of request send to the server as well as the expected server load.

Note

The pagination is not defined in the OCI distribution spec for the Referrers API. oras-go has the ReferrerListPageSize field because the code is adapted from the original ORAS Artifact Spec.

/cc @Wwwsylvia

I've opened an issue on oras-go to track this issue: oras-project/oras-go#841

@FeynmanZhou
Copy link
Member

I've opened an issue on oras-go to track this issue: oras-project/oras-go#841

We also need to create an issue in the oras repo to elaborate on the feature request and why this is necessary for oras.

@qweeah
Copy link
Contributor

qweeah commented Nov 19, 2024

We also need to create an issue in the oras repo to elaborate on the feature request and why this is necessary for oras.

Hi @njucjc can you kindly create an issue explaining the use case of the suggested pagination feature and why it is necessary for oras?

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.

5 participants