From 073d8e5545d09bbc0ad2038b945f5398dad3e895 Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Tue, 6 Apr 2021 23:19:55 +0200 Subject: [PATCH] Categorize failure metrics, set specific metrics for compose file parse error, file not found, cmdline syntax errors, build error, pull error. Signed-off-by: Guillaume Tardif --- cli/cmd/compose/compose.go | 11 ++- cli/main.go | 20 ++++- cli/metrics/client.go | 11 --- cli/metrics/compose_errors.go | 72 ++++++++++++++++ cli/metrics/definitions.go | 57 ++++++++++++ local/compose/build.go | 18 ++-- local/compose/pull.go | 7 +- local/e2e/cli-only/e2e_test.go | 3 +- local/e2e/compose/compose_run_test.go | 4 +- .../wrong-composefile/build-error.yml | 3 + .../fixtures/wrong-composefile/compose.yml | 4 + .../wrong-composefile/service1/Dockerfile | 17 ++++ .../wrong-composefile/unknown-image.yml | 3 + local/e2e/compose/metrics_test.go | 86 +++++++++++++++++++ .../e2e/cli-only => utils/e2e}/mockmetrics.go | 2 +- 15 files changed, 287 insertions(+), 31 deletions(-) create mode 100644 cli/metrics/compose_errors.go create mode 100644 cli/metrics/definitions.go create mode 100644 local/e2e/compose/fixtures/wrong-composefile/build-error.yml create mode 100644 local/e2e/compose/fixtures/wrong-composefile/compose.yml create mode 100644 local/e2e/compose/fixtures/wrong-composefile/service1/Dockerfile create mode 100644 local/e2e/compose/fixtures/wrong-composefile/unknown-image.yml create mode 100644 local/e2e/compose/metrics_test.go rename {local/e2e/cli-only => utils/e2e}/mockmetrics.go (99%) diff --git a/cli/cmd/compose/compose.go b/cli/cmd/compose/compose.go index e15911180..60e51f065 100644 --- a/cli/cmd/compose/compose.go +++ b/cli/cmd/compose/compose.go @@ -30,6 +30,7 @@ import ( "github.com/docker/compose-cli/api/context/store" "github.com/docker/compose-cli/cli/formatter" + "github.com/docker/compose-cli/cli/metrics" ) // Warning is a global warning to be displayed to user on command failure @@ -74,7 +75,7 @@ func (o *projectOptions) toProject(services []string, po ...cli.ProjectOptionsFn project, err := cli.ProjectFromOptions(options) if err != nil { - return nil, err + return nil, metrics.WrapComposeError(err) } if len(services) != 0 { @@ -114,6 +115,14 @@ func Command(contextType string) *cobra.Command { Short: "Docker Compose", Use: "compose", TraverseChildren: true, + // By default (no Run/RunE in parent command) for typos in subcommands, cobra displays the help of parent command but exit(0) ! + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return cmd.Help() + } + _ = cmd.Help() + return fmt.Errorf("unknown docker command: %q", "compose "+args[0]) + }, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { if noAnsi { if ansi != "auto" { diff --git a/cli/main.go b/cli/main.go index 61a23076a..d5aba3938 100644 --- a/cli/main.go +++ b/cli/main.go @@ -70,7 +70,7 @@ var ( "version": {}, "backend-metadata": {}, } - unknownCommandRegexp = regexp.MustCompile(`unknown command "([^"]*)"`) + unknownCommandRegexp = regexp.MustCompile(`unknown docker command: "([^"]*)"`) ) func init() { @@ -122,7 +122,7 @@ func main() { if len(args) == 0 { return cmd.Help() } - return fmt.Errorf("unknown command %q", args[0]) + return fmt.Errorf("unknown docker command: %q", args[0]) }, } @@ -292,7 +292,18 @@ func exit(ctx string, err error, ctype string) { os.Exit(exit.StatusCode) } - metrics.Track(ctype, os.Args[1:], metrics.FailureStatus) + var composeErr metrics.ComposeError + metricsStatus := metrics.FailureStatus + exitCode := 1 + if errors.As(err, &composeErr) { + metricsStatus = composeErr.GetMetricsFailureCategory().MetricsStatus + exitCode = composeErr.GetMetricsFailureCategory().ExitCode + } + if strings.HasPrefix(err.Error(), "unknown shorthand flag:") || strings.HasPrefix(err.Error(), "unknown flag:") || strings.HasPrefix(err.Error(), "unknown docker command:") { + metricsStatus = metrics.CommandSyntaxFailure.MetricsStatus + exitCode = metrics.CommandSyntaxFailure.ExitCode + } + metrics.Track(ctype, os.Args[1:], metricsStatus) if errors.Is(err, errdefs.ErrLoginRequired) { fmt.Fprintln(os.Stderr, err) @@ -310,7 +321,8 @@ func exit(ctx string, err error, ctype string) { os.Exit(1) } - fatal(err) + fmt.Fprintln(os.Stderr, err) + os.Exit(exitCode) } func fatal(err error) { diff --git a/cli/metrics/client.go b/cli/metrics/client.go index 44954c790..570a135eb 100644 --- a/cli/metrics/client.go +++ b/cli/metrics/client.go @@ -47,17 +47,6 @@ func init() { } } -const ( - // APISource is sent for API metrics - APISource = "api" - // SuccessStatus is sent for API metrics - SuccessStatus = "success" - // FailureStatus is sent for API metrics - FailureStatus = "failure" - // CanceledStatus is sent for API metrics - CanceledStatus = "canceled" -) - // Client sends metrics to Docker Desktopn type Client interface { // Send sends the command to Docker Desktop. Note that the function doesn't diff --git a/cli/metrics/compose_errors.go b/cli/metrics/compose_errors.go new file mode 100644 index 000000000..7a1b06435 --- /dev/null +++ b/cli/metrics/compose_errors.go @@ -0,0 +1,72 @@ +/* + Copyright 2020 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 metrics + +import ( + "io/fs" + + "github.com/pkg/errors" + + composeerrdefs "github.com/compose-spec/compose-go/errdefs" +) + +// ComposeError error to categorize failures and extract metrics info +type ComposeError struct { + Err error + Category *FailureCategory +} + +// WrapComposeError wraps the error if not nil, otherwise returns nil +func WrapComposeError(err error) error { + if err == nil { + return nil + } + return ComposeError{ + Err: err, + } +} + +// WrapCategorisedComposeError wraps the error if not nil, otherwise returns nil +func WrapCategorisedComposeError(err error, failure FailureCategory) error { + if err == nil { + return nil + } + return ComposeError{ + Err: err, + Category: &failure, + } +} + +// Unwrap get underlying error +func (e ComposeError) Unwrap() error { return e.Err } + +func (e ComposeError) Error() string { return e.Err.Error() } + +// GetMetricsFailureCategory get metrics status and error code corresponding to this error +func (e ComposeError) GetMetricsFailureCategory() FailureCategory { + if e.Category != nil { + return *e.Category + } + var pathError *fs.PathError + if errors.As(e.Err, &pathError) { + return FileNotFoundFailure + } + if composeerrdefs.IsNotFoundError(e.Err) { + return FileNotFoundFailure + } + return ComposeParseFailure +} diff --git a/cli/metrics/definitions.go b/cli/metrics/definitions.go new file mode 100644 index 000000000..707af1af0 --- /dev/null +++ b/cli/metrics/definitions.go @@ -0,0 +1,57 @@ +/* + Copyright 2020 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 metrics + +// FailureCategory sruct regrouping metrics failure status and specific exit code +type FailureCategory struct { + MetricsStatus string + ExitCode int +} + +const ( + // APISource is sent for API metrics + APISource = "api" + // SuccessStatus command success + SuccessStatus = "success" + // FailureStatus command failure + FailureStatus = "failure" + // ComposeParseFailureStatus failure while parsing compose file + ComposeParseFailureStatus = "failure-compose-parse" + // FileNotFoundFailureStatus failure getting compose file + FileNotFoundFailureStatus = "failure-file-not-found" + // CommandSyntaxFailureStatus failure reading command + CommandSyntaxFailureStatus = "failure-cmd-syntax" + // BuildFailureStatus failure building imge + BuildFailureStatus = "failure-build" + // PullFailureStatus failure pulling imge + PullFailureStatus = "failure-pull" + // CanceledStatus command canceled + CanceledStatus = "canceled" +) + +var ( + // FileNotFoundFailure failure for compose file not found + FileNotFoundFailure = FailureCategory{MetricsStatus: FileNotFoundFailureStatus, ExitCode: 14} + // ComposeParseFailure failure for composefile parse error + ComposeParseFailure = FailureCategory{MetricsStatus: ComposeParseFailureStatus, ExitCode: 15} + // CommandSyntaxFailure failure for command line syntax + CommandSyntaxFailure = FailureCategory{MetricsStatus: CommandSyntaxFailureStatus, ExitCode: 16} + //BuildFailure failure while building images. + BuildFailure = FailureCategory{MetricsStatus: BuildFailureStatus, ExitCode: 17} + // PullFailure failure while pulling image + PullFailure = FailureCategory{MetricsStatus: PullFailureStatus, ExitCode: 18} +) diff --git a/local/compose/build.go b/local/compose/build.go index 6d2714955..d2f3d138a 100644 --- a/local/compose/build.go +++ b/local/compose/build.go @@ -24,21 +24,21 @@ import ( "path" "strings" - moby "github.com/docker/docker/api/types" - - "github.com/docker/compose-cli/api/compose" - composeprogress "github.com/docker/compose-cli/api/progress" - "github.com/docker/compose-cli/utils" - "github.com/compose-spec/compose-go/types" "github.com/containerd/containerd/platforms" "github.com/docker/buildx/build" "github.com/docker/buildx/driver" _ "github.com/docker/buildx/driver/docker" // required to get default driver registered "github.com/docker/buildx/util/progress" + moby "github.com/docker/docker/api/types" "github.com/docker/docker/errdefs" bclient "github.com/moby/buildkit/client" specs "github.com/opencontainers/image-spec/specs-go/v1" + + "github.com/docker/compose-cli/api/compose" + composeprogress "github.com/docker/compose-cli/api/progress" + "github.com/docker/compose-cli/cli/metrics" + "github.com/docker/compose-cli/utils" ) func (s *composeService) Build(ctx context.Context, project *types.Project, options compose.BuildOptions) error { @@ -160,7 +160,8 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opts if info.OSType == "windows" { // no support yet for Windows container builds in Buildkit // https://docs.docker.com/develop/develop-images/build_enhancements/#limitations - return s.windowsBuild(opts, mode) + err := s.windowsBuild(opts, mode) + return metrics.WrapCategorisedComposeError(err, metrics.BuildFailure) } if len(opts) == 0 { return nil @@ -190,6 +191,9 @@ func (s *composeService) build(ctx context.Context, project *types.Project, opts if err == nil { err = errW } + if err != nil { + return metrics.WrapCategorisedComposeError(err, metrics.BuildFailure) + } cw := composeprogress.ContextWriter(ctx) for _, c := range observedState { diff --git a/local/compose/pull.go b/local/compose/pull.go index 1be445fc5..7e114b487 100644 --- a/local/compose/pull.go +++ b/local/compose/pull.go @@ -36,6 +36,7 @@ import ( "github.com/docker/compose-cli/api/compose" "github.com/docker/compose-cli/api/config" "github.com/docker/compose-cli/api/progress" + "github.com/docker/compose-cli/cli/metrics" ) func (s *composeService) Pull(ctx context.Context, project *types.Project, opts compose.PullOptions) error { @@ -121,7 +122,7 @@ func (s *composeService) pullServiceImage(ctx context.Context, service types.Ser Status: progress.Error, Text: "Error", }) - return err + return metrics.WrapCategorisedComposeError(err, metrics.PullFailure) } dec := json.NewDecoder(stream) @@ -131,10 +132,10 @@ func (s *composeService) pullServiceImage(ctx context.Context, service types.Ser if err == io.EOF { break } - return err + return metrics.WrapCategorisedComposeError(err, metrics.PullFailure) } if jm.Error != nil { - return errors.New(jm.Error.Message) + return metrics.WrapCategorisedComposeError(errors.New(jm.Error.Message), metrics.PullFailure) } toPullProgressEvent(service.Name, jm, w) } diff --git a/local/e2e/cli-only/e2e_test.go b/local/e2e/cli-only/e2e_test.go index 21136ab1e..1e5305344 100644 --- a/local/e2e/cli-only/e2e_test.go +++ b/local/e2e/cli-only/e2e_test.go @@ -182,7 +182,7 @@ func TestContextMetrics(t *testing.T) { assert.DeepEqual(t, []string{ `{"command":"ps","context":"moby","source":"cli","status":"success"}`, `{"command":"version","context":"moby","source":"cli","status":"success"}`, - `{"command":"version","context":"moby","source":"cli","status":"failure"}`, + `{"command":"version","context":"moby","source":"cli","status":"failure-cmd-syntax"}`, }, usage) }) @@ -224,7 +224,6 @@ func TestContextDuplicateACI(t *testing.T) { } func TestContextRemove(t *testing.T) { - t.Run("remove current", func(t *testing.T) { c := NewParallelE2eCLI(t, binDir) diff --git a/local/e2e/compose/compose_run_test.go b/local/e2e/compose/compose_run_test.go index e46e9dd16..ff8397b83 100644 --- a/local/e2e/compose/compose_run_test.go +++ b/local/e2e/compose/compose_run_test.go @@ -95,9 +95,9 @@ func TestLocalComposeRun(t *testing.T) { }) t.Run("compose run --publish", func(t *testing.T) { - c.RunDockerCmd("compose", "-f", "./fixtures/run-test/compose.yml", "run", "--publish", "8080:80", "-d", "back", "/bin/sh", "-c", "sleep 1") + c.RunDockerCmd("compose", "-f", "./fixtures/run-test/compose.yml", "run", "--publish", "8081:80", "-d", "back", "/bin/sh", "-c", "sleep 1") res := c.RunDockerCmd("ps") - assert.Assert(t, strings.Contains(res.Stdout(), "8080->80/tcp"), res.Stdout()) + assert.Assert(t, strings.Contains(res.Stdout(), "8081->80/tcp"), res.Stdout()) }) t.Run("down", func(t *testing.T) { diff --git a/local/e2e/compose/fixtures/wrong-composefile/build-error.yml b/local/e2e/compose/fixtures/wrong-composefile/build-error.yml new file mode 100644 index 000000000..ea13a847b --- /dev/null +++ b/local/e2e/compose/fixtures/wrong-composefile/build-error.yml @@ -0,0 +1,3 @@ +services: + simple: + build: service1 diff --git a/local/e2e/compose/fixtures/wrong-composefile/compose.yml b/local/e2e/compose/fixtures/wrong-composefile/compose.yml new file mode 100644 index 000000000..323e11026 --- /dev/null +++ b/local/e2e/compose/fixtures/wrong-composefile/compose.yml @@ -0,0 +1,4 @@ +services: + simple: + image: nginx + wrongField: test diff --git a/local/e2e/compose/fixtures/wrong-composefile/service1/Dockerfile b/local/e2e/compose/fixtures/wrong-composefile/service1/Dockerfile new file mode 100644 index 000000000..b57af6196 --- /dev/null +++ b/local/e2e/compose/fixtures/wrong-composefile/service1/Dockerfile @@ -0,0 +1,17 @@ +# Copyright 2020 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. + +FROM nginx + +WRONG DOCKERFILE diff --git a/local/e2e/compose/fixtures/wrong-composefile/unknown-image.yml b/local/e2e/compose/fixtures/wrong-composefile/unknown-image.yml new file mode 100644 index 000000000..3000c4686 --- /dev/null +++ b/local/e2e/compose/fixtures/wrong-composefile/unknown-image.yml @@ -0,0 +1,3 @@ +services: + simple: + image: unknownimage diff --git a/local/e2e/compose/metrics_test.go b/local/e2e/compose/metrics_test.go new file mode 100644 index 000000000..1600d6ea3 --- /dev/null +++ b/local/e2e/compose/metrics_test.go @@ -0,0 +1,86 @@ +/* + Copyright 2020 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 e2e + +import ( + "fmt" + "testing" + "time" + + "gotest.tools/v3/assert" + "gotest.tools/v3/icmd" + + . "github.com/docker/compose-cli/utils/e2e" +) + +func TestComposeMetrics(t *testing.T) { + c := NewParallelE2eCLI(t, binDir) + s := NewMetricsServer(c.MetricsSocket()) + s.Start() + defer s.Stop() + + started := false + for i := 0; i < 30; i++ { + c.RunDockerCmd("help", "ps") + if len(s.GetUsage()) > 0 { + started = true + fmt.Printf(" [%s] Server up in %d ms\n", t.Name(), i*100) + break + } + time.Sleep(100 * time.Millisecond) + } + assert.Assert(t, started, "Metrics mock server not available after 3 secs") + + t.Run("catch specific failure metrics", func(t *testing.T) { + s.ResetUsage() + + res := c.RunDockerOrExitError("compose", "-f", "../compose/fixtures/does-not-exist/compose.yml", "build") + res.Assert(t, icmd.Expected{ExitCode: 14, Err: "compose/fixtures/does-not-exist/compose.yml: no such file or directory"}) + res = c.RunDockerOrExitError("compose", "-f", "../compose/fixtures/wrong-composefile/compose.yml", "up", "-d") + res.Assert(t, icmd.Expected{ExitCode: 15, Err: "services.simple Additional property wrongField is not allowed"}) + res = c.RunDockerOrExitError("compose", "up") + res.Assert(t, icmd.Expected{ExitCode: 14, Err: "can't find a suitable configuration file in this directory or any parent: not found"}) + res = c.RunDockerOrExitError("compose", "up", "-f", "../compose/fixtures/wrong-composefile/compose.yml") + res.Assert(t, icmd.Expected{ExitCode: 16, Err: "unknown shorthand flag: 'f' in -f"}) + res = c.RunDockerOrExitError("compose", "up", "--file", "../compose/fixtures/wrong-composefile/compose.yml") + res.Assert(t, icmd.Expected{ExitCode: 16, Err: "unknown flag: --file"}) + res = c.RunDockerOrExitError("compose", "donw", "--file", "../compose/fixtures/wrong-composefile/compose.yml") + res.Assert(t, icmd.Expected{ExitCode: 16, Err: `unknown docker command: "compose donw"`}) + res = c.RunDockerOrExitError("compose", "--file", "../compose/fixtures/wrong-composefile/unknown-image.yml", "pull") + res.Assert(t, icmd.Expected{ExitCode: 18, Err: `pull access denied for unknownimage, repository does not exist or may require 'docker login'`}) + res = c.RunDockerOrExitError("compose", "--file", "../compose/fixtures/wrong-composefile/build-error.yml", "build") + res.Assert(t, icmd.Expected{ExitCode: 17, Err: `line 17: unknown instruction: WRONG`}) + res = c.RunDockerOrExitError("compose", "--file", "../compose/fixtures/wrong-composefile/build-error.yml", "up") + res.Assert(t, icmd.Expected{ExitCode: 17, Err: `line 17: unknown instruction: WRONG`}) + res = c.RunDockerOrExitError("compose", "--file", "../compose/fixtures/wrong-composefile/unknown-image.yml", "up") + res.Assert(t, icmd.Expected{ExitCode: 17, Err: `pull access denied, repository does not exist or may require authorization`}) + + usage := s.GetUsage() + assert.DeepEqual(t, []string{ + `{"command":"compose build","context":"moby","source":"cli","status":"failure-file-not-found"}`, + `{"command":"compose up","context":"moby","source":"cli","status":"failure-compose-parse"}`, + `{"command":"compose up","context":"moby","source":"cli","status":"failure-file-not-found"}`, + `{"command":"compose up","context":"moby","source":"cli","status":"failure-cmd-syntax"}`, + `{"command":"compose up","context":"moby","source":"cli","status":"failure-cmd-syntax"}`, + `{"command":"compose","context":"moby","source":"cli","status":"failure-cmd-syntax"}`, + `{"command":"compose pull","context":"moby","source":"cli","status":"failure-pull"}`, + `{"command":"compose build","context":"moby","source":"cli","status":"failure-build"}`, + `{"command":"compose up","context":"moby","source":"cli","status":"failure-build"}`, + `{"command":"compose up","context":"moby","source":"cli","status":"failure-build"}`, + }, usage) + }) +} diff --git a/local/e2e/cli-only/mockmetrics.go b/utils/e2e/mockmetrics.go similarity index 99% rename from local/e2e/cli-only/mockmetrics.go rename to utils/e2e/mockmetrics.go index a12a8853b..681d97999 100644 --- a/local/e2e/cli-only/mockmetrics.go +++ b/utils/e2e/mockmetrics.go @@ -14,7 +14,7 @@ limitations under the License. */ -package main +package e2e import ( "io/ioutil"