-
Notifications
You must be signed in to change notification settings - Fork 384
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
docker: drop use of external distribution challenge pkg #2629
docker: drop use of external distribution challenge pkg #2629
Conversation
3cfd7bb
to
f0b1ae1
Compare
% go mod why github.com/docker/distribution/registry/client
# github.com/docker/distribution/registry/client
github.com/containers/image/v5/pkg/docker/config
github.com/containers/image/v5/pkg/docker/config.test
github.com/docker/docker/registry
github.com/docker/distribution/registry/client/auth
github.com/docker/distribution/registry/client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! distribution/distribution/v3
would be a separate Go module from docker/distribution v2.…+incompatible
, so this is not immediately urgent, but removing duplicated code is always nice.
Could you restructure the code to make the handling of StatusCode
more explicit, please? We don’t really need the separate responseChallenges
function, and it seems somewhat non-obvious to me that a function with that name does not just parse challenges, but conditions that on a status (“A server MAY generate a WWW-Authenticate header field in other response messages” per RFC 7235)
Something like
switch {
case resp.StatusCode == http.StatusUnauthorized:
// …
for _, c := range parseAuthHeader(…) {
}
fallthrough
case resp.StatusCode >= 400 && resp.StatusCode < 500:
…
default:
…
}
maybe
Signed-off-by: Flavian Missi <[email protected]>
f0b1ae1
to
3a8a059
Compare
I did as you suggested and also added some more unit tests for |
in distribution v3, the registry/client package became internal. this change drops usage of dockerChallenge.ResponseChallenges in favor of parseAuthHeader directly, merging some of ResponseChallenges functionality into handleErrorResponse (where it was being called). Signed-off-by: Flavian Missi <[email protected]>
3a8a059
to
2aee541
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
in distribution v3, the registry/client package became internal. this change copies
responseChallenges
from upstream, removing the last reference to registry/client in this repo, making it ready for the distribution v3 bump, whenever that comes.NOTE: I expected this change to cause
go mod tidy && go mod vendor
to remove thechallenge
package from the vendor dir, but that didn't happen - not sure why.