From 16edf8bffbae06c8cc2df37a92a064778c38e9e4 Mon Sep 17 00:00:00 2001 From: CrazyMax Date: Thu, 3 Feb 2022 19:11:05 +0100 Subject: [PATCH] builder: conditional warning for wcow Signed-off-by: CrazyMax --- cmd/docker/builder.go | 39 ++++++++++------- cmd/docker/builder_test.go | 86 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 20 deletions(-) diff --git a/cmd/docker/builder.go b/cmd/docker/builder.go index faa7adbeca..03d6055b8e 100644 --- a/cmd/docker/builder.go +++ b/cmd/docker/builder.go @@ -34,43 +34,50 @@ func newBuilderError(warn bool, err error) error { if pluginmanager.IsNotFound(err) { return errors.New(errorMsg) } - return fmt.Errorf("%w\n\n%s", err, errorMsg) + if err != nil { + return fmt.Errorf("%w\n\n%s", err, errorMsg) + } + return fmt.Errorf("%s", errorMsg) } func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []string) ([]string, []string, error) { + var useLegacy bool + var useBuilder bool + // check DOCKER_BUILDKIT env var is present and - // if not assume we want to use a builder - var enforcedBuilder bool + // if not assume we want to use the builder component if v, ok := os.LookupEnv("DOCKER_BUILDKIT"); ok { enabled, err := strconv.ParseBool(v) if err != nil { return args, osargs, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value") } if !enabled { - return args, osargs, nil + useLegacy = true + } else { + useBuilder = true } - enforcedBuilder = true } // if a builder alias is defined, use it instead // of the default one - isAlias := false builderAlias := builderDefaultPlugin aliasMap := dockerCli.ConfigFile().Aliases if v, ok := aliasMap[keyBuilderAlias]; ok { - isAlias = true + useBuilder = true builderAlias = v } - // wcow build command must use the legacy builder for buildx - // if not opt-in through a builder alias - if !isAlias && dockerCli.ServerInfo().OSType == "windows" { + // is this a build that should be forwarded to the builder? + fwargs, fwosargs, forwarded := forwardBuilder(builderAlias, args, osargs) + if !forwarded { return args, osargs, nil } - // are we using a cmd that should be forwarded to the builder? - fwargs, fwosargs, forwarded := forwardBuilder(builderAlias, args, osargs) - if !forwarded { + if useLegacy { + // display warning if not wcow and continue + if dockerCli.ServerInfo().OSType != "windows" { + _, _ = fmt.Fprintln(dockerCli.Err(), newBuilderError(true, nil)) + } return args, osargs, nil } @@ -80,9 +87,9 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st perr = plugin.Err } if perr != nil { - // if builder enforced with DOCKER_BUILDKIT=1, cmd fails if plugin missing or broken - if enforcedBuilder { - return fwargs, fwosargs, newBuilderError(false, perr) + // if builder enforced with DOCKER_BUILDKIT=1, cmd must fail if plugin missing or broken + if useBuilder { + return args, osargs, newBuilderError(false, perr) } // otherwise, display warning and continue _, _ = fmt.Fprintln(dockerCli.Err(), newBuilderError(true, perr)) diff --git a/cmd/docker/builder_test.go b/cmd/docker/builder_test.go index bc0f3fa0a1..c7e70d61f4 100644 --- a/cmd/docker/builder_test.go +++ b/cmd/docker/builder_test.go @@ -3,17 +3,27 @@ package main import ( "bytes" "os" + "runtime" "testing" "github.com/docker/cli/cli/command" + "github.com/docker/cli/internal/test/output" "gotest.tools/v3/assert" "gotest.tools/v3/env" "gotest.tools/v3/fs" ) -func TestBuild(t *testing.T) { +var pluginFilename = "docker-buildx" + +func init() { + if runtime.GOOS == "windows" { + pluginFilename = pluginFilename + ".exe" + } +} + +func TestBuildWithBuilder(t *testing.T) { dir := fs.NewDir(t, t.Name(), - fs.WithFile("docker-buildx", `#!/bin/sh + fs.WithFile(pluginFilename, `#!/bin/sh echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortDescription":"Build with BuildKit"}'`, fs.WithMode(0777)), ) defer dir.Remove() @@ -36,10 +46,17 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD func TestBuildkitDisabled(t *testing.T) { defer env.Patch(t, "DOCKER_BUILDKIT", "0")() - var b bytes.Buffer - dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(&b)) + dir := fs.NewDir(t, t.Name(), + fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0777)), + ) + defer dir.Remove() + + b := bytes.NewBuffer(nil) + + dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b)) assert.NilError(t, err) + dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()} tcmd := newDockerCommand(dockerCli) tcmd.SetArgs([]string{"build", "."}) @@ -50,4 +67,65 @@ func TestBuildkitDisabled(t *testing.T) { args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args) assert.NilError(t, err) assert.DeepEqual(t, []string{"build", "."}, args) + + output.Assert(t, b.String(), map[int]func(string) error{ + 0: output.Suffix("DEPRECATED: The legacy builder is deprecated and will be removed in a future release."), + }) +} + +func TestBuilderBroken(t *testing.T) { + dir := fs.NewDir(t, t.Name(), + fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0777)), + ) + defer dir.Remove() + + b := bytes.NewBuffer(nil) + + dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b)) + assert.NilError(t, err) + dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()} + + tcmd := newDockerCommand(dockerCli) + tcmd.SetArgs([]string{"build", "."}) + + cmd, args, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + + args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args) + assert.NilError(t, err) + assert.DeepEqual(t, []string{"build", "."}, args) + + output.Assert(t, b.String(), map[int]func(string) error{ + 0: output.Prefix("failed to fetch metadata:"), + 2: output.Suffix("DEPRECATED: The legacy builder is deprecated and will be removed in a future release."), + }) +} + +func TestBuilderBrokenEnforced(t *testing.T) { + defer env.Patch(t, "DOCKER_BUILDKIT", "1")() + + dir := fs.NewDir(t, t.Name(), + fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0777)), + ) + defer dir.Remove() + + b := bytes.NewBuffer(nil) + + dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b)) + assert.NilError(t, err) + dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()} + + tcmd := newDockerCommand(dockerCli) + tcmd.SetArgs([]string{"build", "."}) + + cmd, args, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + + args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args) + assert.DeepEqual(t, []string{"build", "."}, args) + + output.Assert(t, err.Error(), map[int]func(string) error{ + 0: output.Prefix("failed to fetch metadata:"), + 2: output.Suffix("ERROR: BuildKit is enabled but the buildx component is missing or broken."), + }) }