cli/command/registry: remove all uses of response message#6436
cli/command/registry: remove all uses of response message#6436thaJeztah wants to merge 2 commits intodocker:masterfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
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.
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
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
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 <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
1e99b25 to
0920f1b
Compare
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)