Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

feat: display docker scout hints on build and pull #2252

Merged
merged 8 commits into from
Jun 21, 2023

Conversation

eunomie
Copy link
Member

@eunomie eunomie commented Jun 6, 2023

What I did

On docker build and docker pull commands, display a hint to docker scout quickview.

Hints are enabled by default.
To disable them:

  • use the --quiet/-q quiet flag
  • set the DOCKER_CLI_HINTS environment variable with a false (according to Go) value
  • add a -x-cli-hints configuration in ~/.docker/config.json file under {plugins: {-x-cli-hints: {enabled}}}

In case of build, if --push, --progress or --output flag are used, hints are not displayed to keep it working on main (and less advanced) case.

There's no hint for buildx build.

In case of a docker build the docker scout quickview command doesn't need an argument as it will take the most recently built image by default.
In case of a docker pull the pulled image is extracted from the command arguments.

The output already handles plain/tty display:
image

Related issue

https://github.com/docker/team-ssc-dev-features/issues/306

(not mandatory) A picture of a cute animal, if possible in relation with what you did
bela-bako-CT0FXtDtYII-unsplash

@github-actions github-actions bot added the cli cli label Jun 6, 2023
On `docker build` and `docker pull` commands, display a hint to
`docker scout quickview`.
Hints are disabled if quiet flag is used or if `DOCKER_SCOUT_HINTS`
environment variable is set to a false (according to Go) value:
  `0`, `f`, `F`, `false`, `FALSE`, `False`

In case of a `docker build` the `docker scout quickview` command doesn't
need an argument as it will take the most recently built image by
default.
In case of a `docker pull` the pulled image is extracted from the
command arguments.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
if command == "pull" {
for _, a := range commandArgs[1:] {
if !strings.HasPrefix(a, "--") {
image = a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably won't work for docker build as it takes the build context as positional argument, not an image.

Also note that;

  • build may not build an image (in case --output is used)
  • build allows controlling the output format (plain/tty), ik which case we may have to adjust the output accordingly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this part is only for pull, not build. In case of a build we don't care that much as we already have a shortcut to get the most recently built image.

But I'll check:

  • the output so it's only displayed when we build an image
  • I'll check the output format

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hint already handle plain/tty, I'll just check if there's other way to enable/disable it for build for instance
image

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
only display the hints in main case, without progress, output or push
flags

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
- cliHints entry in config.json
- if not present, enabled by default
- can be overridden by the DOCKER_CLI_HINTS environment variable

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
@github-actions github-actions bot added the api api label Jun 12, 2023
@eunomie
Copy link
Member Author

eunomie commented Jun 12, 2023

I updated some things:

  • to disable them from the ~/.docker/config.json file
  • to disable them with a more generic DOCKER_CLI_HINTS env variable
  • to disable them in more advanced/unknown build scenario (use of progress, output. push flags)

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
@eunomie
Copy link
Member Author

eunomie commented Jun 12, 2023

Also added a CliHintsEnabled() that can be used if we introduce more hints

To avoid any conflict with the CLI, use the `plugins` section of the
config. This section is read and saved by the CLI without the risk to
remove the value in there. So it's a safe way to deal with this feature.

As it's a cross plugin configuration (now for scout but goal is wider
than that) then put it under a generic `-x-cli-hints` name so that other
plugins might use it if needed.

Value can be true/false, but is parsed against these exact two values
instead of using the ParseBool.

Both the environment variable and the config value are parsed the same
way.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
b := color.New(color.Bold)
_, _ = fmt.Fprintln(out)
_, _ = b.Fprintln(out, "What's Next?")
_, _ = fmt.Fprintf(out, " View summary of image vulnerabilities and recommendations → %s", color.CyanString("docker scout quickview%s", image))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a while we've talked about using "analysis" vs "vulnerabilities", I feel like in this case, this might still be a better choice of word?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I don't know, analysis is really more vague than "vulnerabilities and recommendations". Especially that's what will be displayed by the proposed command.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for sure, I just know there's been a lot of discussion around trying to stop using "vulnerabilities", but if others have already seen this, then maybe it's fine as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, I think I'm not aware of those discussions and it's still what we are displaying in the docker scout help so I'd more in favour to keep it consistent with the CLI.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
displayPATSuggestMsg(commandArgs)
if !metrics.HasQuietFlag(commandArgs) {
switch command {
case "build": // only on regular build, not on buildx build
Copy link
Member

@crazy-max crazy-max Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build command redirects to buildx build since Docker 23 and I don't see in the metrics.HasQuietFlag anything checking the BuildKit progress quiet mode.

func displayScoutQuickViewSuggestMsgOnBuild(args []string) {
// only display the hint in the main case, build command and not buildx build, no output flag, no progress flag, no push flag
if utils.StringContains(args, "--output") || utils.StringContains(args, "-o") ||
utils.StringContains(args, "--progress") ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see --progress but not BUILDKIT_PROGRESS env var being taken into account though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add the env var 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

}

func displayScoutQuickViewSuggestMsgOnBuild(args []string) {
// only display the hint in the main case, build command and not buildx build, no output flag, no progress flag, no push flag
Copy link
Member

@crazy-max crazy-max Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment, build redirects to buildx build since Docker 23 and therefore buildx build flags are being applied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand.
Here I check the command flags the user is passing, not what it's done internally. So if it's a buildx build command hint will not be displayed, as well as if it's a build command with --progress or --output flags.
Then the build command can redirect to buildx build but it's more an implementation detail here, no?

the same way we check --progress flag, if it exists, disable

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
@glours glours merged commit 6b231d6 into docker:main Jun 21, 2023
13 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api cli cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants