-
Notifications
You must be signed in to change notification settings - Fork 2k
cli/command/registry: remove all uses of response message #6436
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
The message returned by the API is a hardcoded message; the only real information currently returned by the API is whether or not the auth was successul; https://github.com/moby/moby/blob/v2.0.0-beta.0/daemon/server/router/system/system_routes.go#L408-L421 Signed-off-by: Sebastiaan van Stijn <[email protected]>
86de089
to
3904477
Compare
Flaky test, or did break something?
|
if err := storeCredentials(dockerCLI.ConfigFile(), authConfig); err != nil { | ||
return "", err | ||
} | ||
|
||
return response.Status, err | ||
return storeCredentials(dockerCLI.ConfigFile(), authConfig) |
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.
Ah! The existing code shadowed the error; so this function would return when failing to store the credentials, but the error returned is the error from earlier?? return response.Status, err
returns the err
from response, err := dockerCLI.Client().RegistryLogin(ctx, authConfig)
, but even on failure, it would still try to store the credentials 🤔
I suspect that was a bug!
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.
I think this may have been introduced in 6e4818e
Before that patch, loginWithCredStoreCreds would only try to login, but did not handle saving credentials.
cli/cli/command/registry/login.go
Lines 145 to 157 in fcfdd7b
func loginWithCredStoreCreds(ctx context.Context, dockerCli command.Cli, authConfig *registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) { | |
fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n") | |
cliClient := dockerCli.Client() | |
response, err := cliClient.RegistryLogin(ctx, *authConfig) | |
if err != nil { | |
if errdefs.IsUnauthorized(err) { | |
fmt.Fprintf(dockerCli.Err(), "Stored credentials invalid or expired\n") | |
} else { | |
fmt.Fprintf(dockerCli.Err(), "Login did not succeed, error: %s\n", err) | |
} | |
} | |
return response, err | |
} |
So question is; was it intentional to save credentials even if they were invalid or expired? Or was the intent perhaps to remove / reset those credentials so that they wouldn't be used again?
cli/cli/command/registry/login.go
Lines 179 to 181 in be97096
if err := storeCredentials(dockerCLI.ConfigFile(), authConfig); err != nil { | |
return "", err | |
} |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
The message returned by the API is a hardcoded message; the only real information currently returned by the API is whether or not the auth was successul;
https://github.com/moby/moby/blob/v2.0.0-beta.0/daemon/server/router/system/system_routes.go#L408-L421
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)