Skip to content
This repository was 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
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,5 +100,6 @@ func configFilePath(dir string) string {

// File contains the current context from the docker configuration file
type File struct {
CurrentContext string `json:"currentContext,omitempty"`
CurrentContext string `json:"currentContext,omitempty"`
Plugins map[string]map[string]string `json:"plugins,omitempty"`
}
68 changes: 68 additions & 0 deletions cli/mobycli/cli_hints.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
Copyright 2023 Docker Compose CLI authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package mobycli

import (
"fmt"
"os"

"github.com/docker/compose-cli/api/config"
)

const (
cliHintsEnvVarName = "DOCKER_CLI_HINTS"
cliHintsDefaultBehaviour = true

cliHintsPluginName = "-x-cli-hints"
cliHintsEnabledName = "enabled"
cliHintsEnabled = "true"
cliHintsDisabled = "false"
)

func CliHintsEnabled() bool {
if envValue, ok := os.LookupEnv(cliHintsEnvVarName); ok {
if enabled, err := parseCliHintFlag(envValue); err == nil {
return enabled
}
}

conf, err := config.LoadFile(config.Dir())
if err != nil {
// can't read the config file, use the default behaviour
return cliHintsDefaultBehaviour
}
if cliHintsPluginConfig, ok := conf.Plugins[cliHintsPluginName]; ok {
if cliHintsValue, ok := cliHintsPluginConfig[cliHintsEnabledName]; ok {
if cliHints, err := parseCliHintFlag(cliHintsValue); err == nil {
return cliHints
}
}
}

return cliHintsDefaultBehaviour
}

func parseCliHintFlag(value string) (bool, error) {
switch value {
case cliHintsEnabled:
return true, nil
case cliHintsDisabled:
return false, nil
default:
return cliHintsDefaultBehaviour, fmt.Errorf("could not parse CLI hints enabled flag")
}
}
168 changes: 168 additions & 0 deletions cli/mobycli/cli_hints_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/*
Copyright 2023 Docker Compose CLI authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package mobycli

import (
"os"
"path/filepath"
"testing"

"github.com/docker/compose-cli/api/config"

"gotest.tools/v3/assert"
)

func TestCliHintsEnabled(t *testing.T) {
testCases := []struct {
name string
setup func()
expected bool
}{
{
"enabled by default",
func() {},
true,
},
{
"enabled from environment variable",
func() {
t.Setenv(cliHintsEnvVarName, "true")
},
true,
},
{
"disabled from environment variable",
func() {
t.Setenv(cliHintsEnvVarName, "false")
},
false,
},
{
"unsupported value",
func() {
t.Setenv(cliHintsEnvVarName, "maybe")
},
true,
},
{
"enabled in config file",
func() {
d := testConfigDir(t)
writeSampleConfig(t, d, configEnabled)
},
true,
},
{
"plugin defined in config file but no enabled entry",
func() {
d := testConfigDir(t)
writeSampleConfig(t, d, configPartial)
},
true,
},

{
"unsupported value",
func() {
d := testConfigDir(t)
writeSampleConfig(t, d, configOnce)
},
true,
},
{
"disabled in config file",
func() {
d := testConfigDir(t)
writeSampleConfig(t, d, configDisabled)
},
false,
},
{
"enabled in config file but disabled by env var",
func() {
d := testConfigDir(t)
writeSampleConfig(t, d, configEnabled)
t.Setenv(cliHintsEnvVarName, "false")
},
false,
},
{
"disabled in config file but enabled by env var",
func() {
d := testConfigDir(t)
writeSampleConfig(t, d, configDisabled)
t.Setenv(cliHintsEnvVarName, "true")
},
true,
},
}

for _, testCase := range testCases {
tc := testCase
t.Run(tc.name, func(t *testing.T) {
tc.setup()
assert.Equal(t, CliHintsEnabled(), tc.expected)
})
}
}

func testConfigDir(t *testing.T) string {
dir := config.Dir()
d, _ := os.MkdirTemp("", "")
config.WithDir(d)
t.Cleanup(func() {
_ = os.RemoveAll(d)
config.WithDir(dir)
})
return d
}

func writeSampleConfig(t *testing.T, d string, conf []byte) {
err := os.WriteFile(filepath.Join(d, config.ConfigFileName), conf, 0644)
assert.NilError(t, err)
}

var configEnabled = []byte(`{
"plugins": {
"-x-cli-hints": {
"enabled": "true"
}
}
}`)

var configDisabled = []byte(`{
"plugins": {
"-x-cli-hints": {
"enabled": "false"
}
}
}`)

var configPartial = []byte(`{
"plugins": {
"-x-cli-hints": {
}
}
}`)

var configOnce = []byte(`{
"plugins": {
"-x-cli-hints": {
"enabled": "maybe"
}
}
}`)
13 changes: 11 additions & 2 deletions cli/mobycli/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,18 @@ func Exec(_ *cobra.Command) {
}
commandArgs := os.Args[1:]
command := metrics.GetCommand(commandArgs)
if command == "login" && !metrics.HasQuietFlag(commandArgs) {
displayPATSuggestMsg(commandArgs)
if !metrics.HasQuietFlag(commandArgs) {
switch command {
case "build": // only on regular build, not on buildx build
Copy link
Contributor

@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.

displayScoutQuickViewSuggestMsgOnBuild(commandArgs)
case "pull":
displayScoutQuickViewSuggestMsgOnPull(commandArgs)
case "login":
displayPATSuggestMsg(commandArgs)
default:
}
}

metricsClient.Track(
metrics.CmdResult{
ContextType: store.DefaultContextType,
Expand Down
73 changes: 73 additions & 0 deletions cli/mobycli/scout_suggest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
Copyright 2023 Docker Compose CLI authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package mobycli

import (
"fmt"
"os"
"strings"

"github.com/docker/compose/v2/pkg/utils"

"github.com/fatih/color"
)

func displayScoutQuickViewSuggestMsgOnPull(args []string) {
image := pulledImageFromArgs(args)
displayScoutQuickViewSuggestMsg(image)
}

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
Contributor

@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
Contributor 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?

if utils.StringContains(args, "--output") || utils.StringContains(args, "-o") ||
utils.StringContains(args, "--progress") ||
Copy link
Contributor

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
Contributor 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
Contributor Author

Choose a reason for hiding this comment

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

added

utils.StringContains(args, "--push") {
return
}
displayScoutQuickViewSuggestMsg("")
}

func displayScoutQuickViewSuggestMsg(image string) {
if !CliHintsEnabled() {
return
}
if len(image) > 0 {
image = " " + image
}
out := os.Stderr
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
Contributor 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
Contributor 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.

_, _ = fmt.Fprintln(out)
}

func pulledImageFromArgs(args []string) string {
var image string
var pull bool
for _, a := range args {
if a == "pull" {
pull = true
continue
}
if pull && !strings.HasPrefix(a, "-") {
image = a
break
}
}
return image
}
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ require (
github.com/docker/docker v20.10.7+incompatible
github.com/docker/go-connections v0.4.0
github.com/docker/go-units v0.5.0
github.com/fatih/color v1.7.0
github.com/gobwas/ws v1.1.0
github.com/golang/mock v1.6.0
github.com/golang/protobuf v1.5.2
Expand Down Expand Up @@ -100,7 +101,6 @@ require (
github.com/docker/go-metrics v0.0.1 // indirect
github.com/evanphx/json-patch v4.9.0+incompatible // indirect
github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d // indirect
github.com/fatih/color v1.7.0 // indirect
github.com/form3tech-oss/jwt-go v3.2.2+incompatible // indirect
github.com/fvbommel/sortorder v1.0.1 // indirect
github.com/go-errors/errors v1.0.1 // indirect
Expand Down