cli/command: remove interactive login prompt from docker push/pull #6174
Conversation
There was a problem hiding this comment.
Pull Request Overview
Deprecate RegistryAuthenticationPrivilegedFunc and remove its usage from various CLI commands, replacing interactive fallback prompts with a nil default.
- Mark
RegistryAuthenticationPrivilegedFuncas deprecated. - Remove
requestPrivilegelogic and setPrivilegeFunctonilin commands: sign, search, plugin install, image pull, and image push. - Ensure no interactive credential prompt fallback is invoked by default.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/command/trust/sign.go | Remove requestPrivilege logic and set PrivilegeFunc to nil |
| cli/command/registry/search.go | Remove requestPrivilege logic and set PrivilegeFunc to nil |
| cli/command/registry.go | Add deprecation comment to RegistryAuthenticationPrivilegedFunc |
| cli/command/plugin/install.go | Remove requestPrivilege logic and set PrivilegeFunc to nil |
| cli/command/image/trust.go | Remove requestPrivilege logic and set PrivilegeFunc to nil |
| cli/command/image/push.go | Remove requestPrivilege logic and set PrivilegeFunc to nil |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
Benehiko
force-pushed
the
remove-prompt-privilege-func
branch
2 times, most recently
from
99e60c5 to
ae3e756
Compare
cli/command/registry.go
Outdated
| // | ||
| // Deprecated: this function is no longer used and will be removed in the next release. | ||
| func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInfo, cmdName string) registrytypes.RequestAuthConfig { |
There was a problem hiding this comment.
I should probably dust-off #5925, where I was playing with some options to get some of the nasty "IndexInfo" uses out. In that branch I added a NewAuthRequester function, that's probably more convenient to use for most;
But I was still contemplating if it should also accept a reference.Named as alternative to a string (for the image ref);
// and the given cmdName (used as informational message to the user).
//
// The returned function is a [registrytypes.RequestAuthConfig] to prompt the user
// for credentials if needed. It is called as fallback if the credentials (if any)
// used for the initial operation did not work.
//
// TODO(thaJeztah): cli Cli could be a Streams if it was not for cli.SetIn to be needed?
// TODO(thaJeztah): ideally, this would accept reposName / imageRef as a regular string (we can parse it if needed!), or .. maybe generics and accept either?
func NewAuthRequester(cli Cli, indexServer string, promptMsg string) registrytypes.RequestAuthConfig {
| var requestPrivilege registrytypes.RequestAuthConfig | ||
| if dockerCli.In().IsTerminal() { | ||
| requestPrivilege = command.RegistryAuthenticationPrivilegedFunc(dockerCli, repoInfo.Index, "push") | ||
| } |
There was a problem hiding this comment.
If you have time; perhaps cleaner would be to;
- Put these changes in a separate commit / PR (change-log would be about
docker push/docker pullno longer triggers an interactive login prompt) - The "deprecation" in a separate commit/PR (change-log would be relevant for users of the go module, not impacting end-users)
Separate commits, but same PR is probably fine, but would be good to update the PR title (and change-log to have separate items for each, because different audience).
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
change itself LGTM, but left a comment about splitting the change in behavior from the deprecation.
This patch removes the interactive prompts from `docker push/pull`. The prompt would only execute on a response status code 403 from the registry after trying the value set in `RegistryAuth`. Docker Hub could return 404 instead or 429, which would never execute the prompt. The UX regarding the prompt is also questionable since the user might not actually want to authenticate with a registry and the CLI could fail fast instead. The user can always run `docker login` or set the `DOCKER_AUTH_CONFIG` environment variable to get authenticated. Signed-off-by: Alano Terblanche <[email protected]>
Benehiko
force-pushed
the
remove-prompt-privilege-func
branch
from
ae3e756 to
7c8c267
Compare
Benehiko
force-pushed
the
remove-prompt-privilege-func
branch
from
7c8c267 to
2b56b66
Compare
Benehiko
changed the title
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
I expanded the change-log description a bit; let me know if that looks good to you (also hinted at "returning the error from the registry, as I'm in process of surfacing the actual error from the registry, instead of hiding it behind our generic message).
|
The changelog looks good, we can merge |
4 participants
Relates to:
- What I did
This patch removes the interactive prompts from
docker push/pull.The prompt would only execute on a response status code 403 from the registry
after trying the value set in
RegistryAuth. Docker Hub could return 404instead or 429, which would never execute the prompt.
The UX regarding the prompt is also questionable since the user might
not actually want to authenticate with a registry and the CLI could fail fast
instead. The user can always run
docker loginor set theDOCKER_AUTH_CONFIGenvironment variable to get authenticated.
- 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)