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.

Sign up for GitHub

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

Jump to bottom

feat: support per-host scope hints #604

Merged
merged 29 commits into from
Sep 27, 2023
Merged

feat: support per-host scope hints #604

merged 29 commits into from
Sep 27, 2023

Conversation

Wwwsylvia
Copy link
Member

@Wwwsylvia Wwwsylvia commented Sep 21, 2023 •
edited
Loading

The purpose of this PR is to fix the bug, but new APIs are needed to avoid breaking changes.

  1. Introduce auth.WithScopesForHost
  2. Introduce auth.AppendScopesForHost
  3. Introduce auth.GetScopesForHost and auth.GetAllScopesForHost
  4. Introduce auth.AppendRepositoryScope

Resolves: #581

@Wwwsylvia Wwwsylvia marked this pull request as draft September 21, 2023 08:06
Wwwsylvia
Wwwsylvia commented Sep 21, 2023
View reviewed changes
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Show resolved Hide resolved
@Wwwsylvia Wwwsylvia marked this pull request as ready for review September 21, 2023 09:07
shizhMSFT
shizhMSFT reviewed Sep 22, 2023
View reviewed changes
internal/maps/maps.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Show resolved Hide resolved
registry/remote/auth/client.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023 •
edited
Loading

Codecov Report

Merging #604 (e6fa97b) into main (2d371a0) will increase coverage by 0.32%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #604      +/-   ##
==========================================
+ Coverage   74.46%   74.79%   +0.32%     
==========================================
  Files          59       58       -1     
  Lines        5275     5304      +29     
==========================================
+ Hits         3928     3967      +39     
+ Misses        993      985       -8     
+ Partials      354      352       -2
Files Coverage Δ
content.go 72.85% <100.00%> (ø)
registry/remote/auth/client.go 73.12% <100.00%> (ø)
registry/remote/auth/scope.go 100.00% <100.00%> (ø)
registry/remote/registry.go 67.67% <100.00%> (ø)
registry/remote/repository.go 71.19% <100.00%> (ø)

... and 1 file with indirect coverage changes

@Wwwsylvia Wwwsylvia force-pushed the fix_scope branch 2 times, most recently from 79a6a97 to 8719d29 Compare September 26, 2023 03:29
shizhMSFT
shizhMSFT reviewed Sep 26, 2023
View reviewed changes
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
registry/remote/auth/scope.go Outdated Show resolved Hide resolved
@Wwwsylvia
draft
a88b850 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
fix + test
65c4acb 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
unit tests
1657b05 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
update internal
e260ef9 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
update client
6737ba2 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
TODOs
4f80680 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
rename
14547a7 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
rename
57d0181 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
move + test
fb139e5 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
WithRepositoryScopes
bced651 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
2 client tests
744e55b 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
test oauth password
0d2c1d3 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
test refresh token
3325e26 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
test expire
6a98c3c 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
client tests
f7180dc 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
add docs
448ae21 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
update doc
66f3841 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
minor reword
c517954 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
rename
799ba84 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
fix
5a766a8 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
optimize
29f3497 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
fix race?
78c68b8 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
refactor
60eba30 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
rename
783cffc 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
update doc
32e4866 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
add a private function
fd3da4a 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
test new function
56cc8e7 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
address comments
e6fa97b 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia
expose GetAllScopesForHost
db4eaa8 
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
shizhMSFT
shizhMSFT approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@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

@Wwwsylvia Wwwsylvia merged commit a428ca6 into oras-project:main Sep 27, 2023
7 checks passed
@Wwwsylvia Wwwsylvia deleted the fix_scope branch September 27, 2023 02:58
@Wwwsylvia Wwwsylvia mentioned this pull request Oct 20, 2023
Merged
Wwwsylvia added a commit that referenced this pull request Oct 20, 2023
@Wwwsylvia
perf: reduce auth request count for manifest delete (#623)
f296072 
Backporting 0f1dc30 to the release
branch.
This change does the same thing as the original commit (#618) did but
using the old `registryutil.WithScopeHint` instead of
`auth.AppendRepositoryScope`, which is introduced by #604 and is not
available in `v2.3.x`.

Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
@Wwwsylvia Wwwsylvia mentioned this pull request Jan 26, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangxiaoxuan273 wangxiaoxuan273 wangxiaoxuan273 left review comments

@qweeah qweeah qweeah left review comments

@shizhMSFT shizhMSFT shizhMSFT approved these changes

@sajayantony sajayantony Awaiting requested review from sajayantony sajayantony is a code owner

@SteveLasker SteveLasker Awaiting requested review from SteveLasker SteveLasker is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: auth.AppendScopes does not distinguish between registries
5 participants
@Wwwsylvia @codecov-commenter @qweeah @shizhMSFT @wangxiaoxuan273