login: Add message about using PATs by rumpl · Pull Request #4478 · docker/cli

rumpl · 2023-08-02T10:30:20Z

- What I did

Added a hint about using PATs

$ docker login
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username:

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2023-08-02T10:33:27Z

Codecov Report

Merging #4478 (9947f4b) into master (06de1f8) will decrease coverage by 0.43%.
The diff coverage is 0.00%.

❗ Current head 9947f4b differs from pull request most recent head 8d51f36. Consider uploading reports for the commit 8d51f36 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4478      +/-   ##
==========================================
- Coverage   59.39%   58.97%   -0.43%     
==========================================
  Files         288      286       -2     
  Lines       24782    24779       -3     
==========================================
- Hits        14720    14614     -106     
- Misses       9175     9278     +103     
  Partials      887      887

vvoland · 2023-08-02T11:39:10Z

cli/command/registry.go

 			// if this is a default registry (docker hub), then display the following message.
-			fmt.Fprintln(cli.Out(), "Login with your Docker ID to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com to create one.")
+			fmt.Fprintln(cli.Out(), "Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.")
+			if os.Getenv("DOCKER_CLI_HINTS") != "false" {


0 should also be supported. It feels more natural for an env variable.

How about using strconv.ParseBool withh fallback to true (hints enabled by default) if err != nil?

Maybe we should also handle "no", like yaml does? I'm joking, this check comes from compose-cli.

I kinda like only one possible value here

Another nice possibility is to flip the check, have a DOCKER_NO_CLI_HINTS and only check that the variable is defined.

Examples

cli/cli/streams/in.go

Line 33 in 06de1f8

if !i.isTerminal || os.Getenv("NORAW") != "" {

cli/cli/debug/debug.go

Line 25 in 06de1f8

return os.Getenv("DEBUG") != ""

cli/cli-plugins/plugin/plugin.go

Line 36 in 06de1f8

if os.Getenv("DOCKER_CLI_PLUGIN_USE_DIAL_STDIO") != "" {

Personally I'd prefer DOCKER_NO_CLI_HINTS.

DOCKER_DISABLE_CLI_HINTS would be an option.

I think we came with DOCKER_CLI_HINTS in a similar fashion as DOCKER_BUILDKIT; initially that was "opt-in", but the intent was to make that "opt-out" after incubation.

DOCKER_CLI_HINTS (and DOCKER_BUILDKIT) is more agnostic w.r.t. it being enable/disable. And can start with "opt-in" (DOCKER_CLI_HINTS=1) and when it's the default, "opt-out" (DOCKER_CLI_HINTS=0), or for situations where the config would disable it and you wanted to enable it for a specific call (through env var), as DOCKER_DISABLE_CLI_HINTS=false is kinda awkward because of the double negative.

w.r.t. accepted values, if we do look at value (beyond "non-empty"), I agree that we should use ParseBool if the value is non-empty, which would match other env-vars, such as DOCKER_BUILDKIT;

cli/cli/command/cli.go

Lines 164 to 171 in 06de1f8

// use DOCKER_BUILDKIT env var value if set and not empty

if v := os.Getenv("DOCKER_BUILDKIT"); v != "" {

enabled, err := strconv.ParseBool(v)

if err != nil {

return false, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")

}

return enabled, nil

}

+1 to ParseBool and DOCKER_NO_CLI_HINTS

We already have DOCKER_CLI_HINTS in the wrapper (see docker/for-mac#6904 for evidence of people building on it); we might have to continue with that out of inertia, or at least propagate any changes we make here back to the Scout hints implementation (cc @eunomie)

cli/command/registry.go

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

laurazard

LGTM

vvoland

LGTM; it's beautiful now!

thaJeztah · 2023-08-10T18:43:45Z

cli/command/registry.go

+				fmt.Fprintln(cli.Out(), patSuggest)
+				fmt.Fprintln(cli.Out())


We probably can't / shouldn't change the existing (old) hint, but perhaps we should use cli.Err() here (so that it can be redirected)

thaJeztah · 2023-08-10T18:46:42Z

cli/command/registry.go

+const patSuggest = "You can log in with your password or a Personal Access " +
+	"Token (PAT). Using a limited-scope PAT grants better security and is required " +
+	"for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/"


Perhaps use a string-literal for this (back-tick)

Didn’t use them because we don’t want newlines

neersighted · 2023-08-16T13:23:05Z

I'm going to merge this as-is, given no further discussion around the env var, and the desire to stay consistent (for now) with what was already done in the CLI wrapper for Scout.

rumpl added kind/feature status/2-code-review area/ux area/authentication labels Aug 2, 2023

rumpl mentioned this pull request Aug 2, 2023

login: Remove the PAT message docker-archive/compose-cli#2261

Merged

rumpl force-pushed the feat-pat-suggest branch from 5037bd0 to 9947f4b Compare August 2, 2023 10:37

vvoland reviewed Aug 2, 2023

View reviewed changes

rumpl requested a review from thaJeztah August 2, 2023 12:11

sam-thibault approved these changes Aug 2, 2023

View reviewed changes

thaJeztah reviewed Aug 2, 2023

View reviewed changes

cli/command/registry.go Show resolved Hide resolved

dvdksn approved these changes Aug 2, 2023

View reviewed changes

rumpl force-pushed the feat-pat-suggest branch 2 times, most recently from 78df3b5 to d73fc86 Compare August 3, 2023 08:31

login: Add message about using PATs

8d51f36

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

rumpl force-pushed the feat-pat-suggest branch from d73fc86 to 8d51f36 Compare August 3, 2023 08:35

rumpl requested review from thaJeztah and vvoland August 3, 2023 08:36

laurazard approved these changes Aug 3, 2023

View reviewed changes

vvoland approved these changes Aug 3, 2023

View reviewed changes

thaJeztah reviewed Aug 10, 2023

View reviewed changes

neersighted self-assigned this Aug 10, 2023

This comment was marked as spam.

Sign in to view

neersighted merged commit 3dbf1af into docker:master Aug 16, 2023

neersighted added this to the 25.0.0 milestone Aug 17, 2023

neersighted mentioned this pull request Aug 17, 2023

[24.0 backport] login: Add message about using PATs #4500

Merged

neersighted added the process/cherry-picked label Aug 17, 2023

thaJeztah mentioned this pull request Aug 22, 2023

scout hint: tell user logging in is required docker-archive/compose-cli#2262

Merged

	// use DOCKER_BUILDKIT env var value if set and not empty
	if v := os.Getenv("DOCKER_BUILDKIT"); v != "" {
	enabled, err := strconv.ParseBool(v)
	if err != nil {
	return false, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")
	}
	return enabled, nil
	}

Conversation

rumpl commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rumpl Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neersighted Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laurazard left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

neersighted commented Aug 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

rumpl commented Aug 2, 2023 •

edited

Loading

codecov-commenter commented Aug 2, 2023 •

edited

Loading

rumpl Aug 2, 2023 •

edited

Loading

neersighted Aug 10, 2023 •

edited

Loading