Fix #31386: Remove work-arounds once oras-go creates patch release#32017
Fix #31386: Remove work-arounds once oras-go creates patch release#32017
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes work-arounds that were implemented for bugs in the oras-go library (version 2.6.0). After updating to a patched version of oras-go (v2.6.1-0.20251010150221-ba36eb50d8f7), these work-arounds are no longer necessary and are being cleaned up to simplify the codebase.
Changes:
- Simplifies HTTP transport cloning logic by directly using
http.DefaultTransport.(*http.Transport).Clone()instead of defensive type assertion checks - Removes the
ForceAttemptOAuth2manipulation and retry logic from theLoginfunction, which was a work-around for OAuth2 handling issues in oras-go - Removes test functions that specifically verified the work-around behavior (
TestLogin_ResetsForceAttemptOAuth2_OnSuccessandTestLogin_ResetsForceAttemptOAuth2_OnFailure) - Cleans up unused imports from client_test.go that were only needed by the removed test functions
- Updates oras-go dependency from v2.6.0 to v2.6.1-0.20251010150221-ba36eb50d8f7
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/transport.go | Simplifies transport cloning logic by removing defensive type checks |
| pkg/registry/client.go | Removes ForceAttemptOAuth2 workaround and retry logic from Login function |
| pkg/registry/client_test.go | Removes workaround-specific tests and cleans up unused imports |
| go.mod | Updates oras-go to patched version |
| go.sum | Updates oras-go checksums |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| c.authorizer.ForceAttemptOAuth2 = true | ||
| reg.Client = c.authorizer | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
The removal of error recovery retry logic in the Login function (line 259) simplifies the code significantly, relying on the assumption that the underlying oras-go library bug has been fixed. While this is the intended purpose of the PR, consider adding a comment explaining why the retry logic with ForceAttemptOAuth2 is no longer necessary for maintainability. This helps future developers understand that this wasn't just a cleanup but a fix for a dependency issue.
| ctx := context.Background() | |
| ctx := context.Background() | |
| // A previous implementation retried authentication with ForceAttemptOAuth2 | |
| // to work around an oras-go login bug. That workaround is intentionally no | |
| // longer needed now that the upstream issue has been fixed, so a single Ping | |
| // is sufficient here. |
|
It is going to be a while before oras-go is ready. |
|
Got it - should I close this then and wait for oras-go to be ready? |
|
Yeh, that would probably be best |
Fixes #31386