Merge pull request moby#17428 from ripcurld00d/cli_ninja_build

Change the quiet flag behavior in the build command [closes moby#17623]

api/client/build.go

@@ -3,6 +3,7 @@ package client
 import (
 	"archive/tar"
 	"bufio"
+	"bytes"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -42,7 +43,7 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
 	cmd := Cli.Subcmd("build", []string{"PATH | URL | -"}, Cli.DockerCommands["build"].Description, true)
 	flTags := opts.NewListOpts(validateTag)
 	cmd.Var(&flTags, []string{"t", "-tag"}, "Name and optionally a tag in the 'name:tag' format")
-	suppressOutput := cmd.Bool([]string{"q", "-quiet"}, false, "Suppress the verbose output generated by the containers")
+	suppressOutput := cmd.Bool([]string{"q", "-quiet"}, false, "Suppress the build output and print image ID on success")
 	noCache := cmd.Bool([]string{"-no-cache"}, false, "Do not use cache when building the image")
 	rm := cmd.Bool([]string{"-rm"}, true, "Remove intermediate containers after a successful build")
 	forceRm := cmd.Bool([]string{"-force-rm"}, false, "Always remove intermediate containers")
@@ -87,20 +88,32 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
 		contextDir    string
 		tempDir       string
 		relDockerfile string
+		progBuff      io.Writer
+		buildBuff     io.Writer
 	)
 
+	progBuff = cli.out
+	buildBuff = cli.out
+	if *suppressOutput {
+		progBuff = bytes.NewBuffer(nil)
+		buildBuff = bytes.NewBuffer(nil)
+	}
+
 	switch {
 	case specifiedContext == "-":
 		tempDir, relDockerfile, err = getContextFromReader(cli.in, *dockerfileName)
 	case urlutil.IsGitURL(specifiedContext) && hasGit:
 		tempDir, relDockerfile, err = getContextFromGitURL(specifiedContext, *dockerfileName)
 	case urlutil.IsURL(specifiedContext):
-		tempDir, relDockerfile, err = getContextFromURL(cli.out, specifiedContext, *dockerfileName)
+		tempDir, relDockerfile, err = getContextFromURL(progBuff, specifiedContext, *dockerfileName)
 	default:
 		contextDir, relDockerfile, err = getContextFromLocalDir(specifiedContext, *dockerfileName)
 	}
 
 	if err != nil {
+		if *suppressOutput && urlutil.IsURL(specifiedContext) {
+			fmt.Fprintln(cli.err, progBuff)
+		}
 		return fmt.Errorf("unable to prepare context: %s", err)
 	}
 
@@ -169,7 +182,7 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
 	context = replaceDockerfileTarWrapper(context, newDockerfile, relDockerfile)
 
 	// Setup an upload progress bar
-	progressOutput := streamformatter.NewStreamFormatter().NewProgressOutput(cli.out, true)
+	progressOutput := streamformatter.NewStreamFormatter().NewProgressOutput(progBuff, true)
 
 	var body io.Reader = progress.NewProgressReader(context, progressOutput, 0, "", "Sending build context to Docker daemon")
 
@@ -230,13 +243,16 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
 		return err
 	}
 
-	err = jsonmessage.DisplayJSONMessagesStream(response.Body, cli.out, cli.outFd, cli.isTerminalOut)
+	err = jsonmessage.DisplayJSONMessagesStream(response.Body, buildBuff, cli.outFd, cli.isTerminalOut)
 	if err != nil {
 		if jerr, ok := err.(*jsonmessage.JSONError); ok {
 			// If no error code is set, default to 1
 			if jerr.Code == 0 {
 				jerr.Code = 1
 			}
+			if *suppressOutput {
+				fmt.Fprintf(cli.err, "%s%s", progBuff, buildBuff)
+			}
 			return Cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code}
 		}
 	}
@@ -246,6 +262,11 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
 		fmt.Fprintln(cli.err, `SECURITY WARNING: You are building a Docker image from Windows against a non-Windows Docker host. All files and directories added to build context will have '-rwxr-xr-x' permissions. It is recommended to double check and reset permissions for sensitive files and directories.`)
 	}
 
+	// Everything worked so if -q was provided the output from the daemon
+	// should be just the image ID and we'll print that to stdout.
+	if *suppressOutput {
+		fmt.Fprintf(cli.out, "%s", buildBuff)
+	}
 	// Since the build was successful, now we must tag any of the resolved
 	// images from the above Dockerfile rewrite.
 	for _, resolved := range resolvedTags {

api/server/router/build/build_routes.go

@@ -1,6 +1,7 @@
 package build
 
 import (
+	"bytes"
 	"encoding/base64"
 	"encoding/json"
 	"errors"
@@ -72,6 +73,7 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 		authConfigs        = map[string]types.AuthConfig{}
 		authConfigsEncoded = r.Header.Get("X-Registry-Config")
 		buildConfig        = &dockerfile.Config{}
+		notVerboseBuffer   = bytes.NewBuffer(nil)
 	)
 
 	if authConfigsEncoded != "" {
@@ -90,6 +92,9 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 	defer output.Close()
 	sf := streamformatter.NewJSONStreamFormatter()
 	errf := func(err error) error {
+		if !buildConfig.Verbose && notVerboseBuffer.Len() > 0 {
+			output.Write(notVerboseBuffer.Bytes())
+		}
 		// Do not write the error in the http output if it's still empty.
 		// This prevents from writing a 200(OK) when there is an internal error.
 		if !output.Flushed() {
@@ -170,6 +175,9 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 	// Look at code in DetectContextFromRemoteURL for more information.
 	createProgressReader := func(in io.ReadCloser) io.ReadCloser {
 		progressOutput := sf.NewProgressOutput(output, true)
+		if !buildConfig.Verbose {
+			progressOutput = sf.NewProgressOutput(notVerboseBuffer, true)
+		}
 		return progress.NewProgressReader(in, progressOutput, r.ContentLength, "Downloading context", remoteURL)
 	}
 
@@ -199,13 +207,20 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 		AuthConfigs: authConfigs,
 		Archiver:    defaultArchiver,
 	}
+	if !buildConfig.Verbose {
+		docker.OutOld = notVerboseBuffer
+	}
 
 	b, err := dockerfile.NewBuilder(buildConfig, docker, builder.DockerIgnoreContext{ModifiableContext: context}, nil)
 	if err != nil {
 		return errf(err)
 	}
 	b.Stdout = &streamformatter.StdoutFormatter{Writer: output, StreamFormatter: sf}
 	b.Stderr = &streamformatter.StderrFormatter{Writer: output, StreamFormatter: sf}
+	if !buildConfig.Verbose {
+		b.Stdout = &streamformatter.StdoutFormatter{Writer: notVerboseBuffer, StreamFormatter: sf}
+		b.Stderr = &streamformatter.StderrFormatter{Writer: notVerboseBuffer, StreamFormatter: sf}
+	}
 
 	if closeNotifier, ok := w.(http.CloseNotifier); ok {
 		finished := make(chan struct{})
@@ -235,5 +250,12 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 		}
 	}
 
+	// Everything worked so if -q was provided the output from the daemon
+	// should be just the image ID and we'll print that to stdout.
+	if !buildConfig.Verbose {
+		stdout := &streamformatter.StdoutFormatter{Writer: output, StreamFormatter: sf}
+		fmt.Fprintf(stdout, "%s\n", string(imgID))
+	}
+
 	return nil
 }

builder/dockerfile/internals.go

@@ -524,11 +524,9 @@ func (b *Builder) create() (string, error) {
 
 func (b *Builder) run(cID string) (err error) {
 	errCh := make(chan error)
-	if b.Verbose {
-		go func() {
-			errCh <- b.docker.ContainerAttach(cID, nil, b.Stdout, b.Stderr, true)
-		}()
-	}
+	go func() {
+		errCh <- b.docker.ContainerAttach(cID, nil, b.Stdout, b.Stderr, true)
+	}()
 
 	finished := make(chan struct{})
 	defer close(finished)
@@ -546,11 +544,9 @@ func (b *Builder) run(cID string) (err error) {
 		return err
 	}
 
-	if b.Verbose {
-		// Block on reading output from container, stop on err or chan closed
-		if err := <-errCh; err != nil {
-			return err
-		}
+	// Block on reading output from container, stop on err or chan closed
+	if err := <-errCh; err != nil {
+		return err
 	}
 
 	if ret, _ := b.docker.ContainerWait(cID, -1); ret != 0 {

contrib/completion/fish/docker.fish

@@ -95,7 +95,7 @@ complete -c docker -A -f -n '__fish_seen_subcommand_from build' -l force-rm -d '
 complete -c docker -A -f -n '__fish_seen_subcommand_from build' -l help -d 'Print usage'
 complete -c docker -A -f -n '__fish_seen_subcommand_from build' -l no-cache -d 'Do not use cache when building the image'
 complete -c docker -A -f -n '__fish_seen_subcommand_from build' -l pull -d 'Always attempt to pull a newer version of the image'
-complete -c docker -A -f -n '__fish_seen_subcommand_from build' -s q -l quiet -d 'Suppress the verbose output generated by the containers'
+complete -c docker -A -f -n '__fish_seen_subcommand_from build' -s q -l quiet -d 'Suppress the build output and print image ID on success'
 complete -c docker -A -f -n '__fish_seen_subcommand_from build' -l rm -d 'Remove intermediate containers after a successful build'
 complete -c docker -A -f -n '__fish_seen_subcommand_from build' -s t -l tag -d 'Repository name (and optionally a tag) to be applied to the resulting image in case of success'
 

docs/reference/commandline/build.md

@@ -30,7 +30,7 @@ parent = "smn_cli"
       --memory-swap=""                Total memory (memory + swap), `-1` to disable swap
       --no-cache=false                Do not use cache when building the image
       --pull=false                    Always attempt to pull a newer version of the image
-      -q, --quiet=false               Suppress the verbose output generated by the containers
+      -q, --quiet=false               Suppress the build output and print image ID on success
       --rm=true                       Remove intermediate containers after a successful build
       --shm-size=[]                   Size of `/dev/shm`. The format is `<number><unit>`. `number` must be greater than `0`.  Unit is optional and can be `b` (bytes), `k` (kilobytes), `m` (megabytes), or `g` (gigabytes). If you omit the unit, the system uses bytes. If you omit the size entirely, the system uses `64m`.
       -t, --tag=[]                    Name and optionally a tag in the 'name:tag' format

integration-cli/docker_cli_build_test.go

@@ -4941,6 +4941,110 @@ func (s *DockerSuite) TestBuildLabelsCache(c *check.C) {
 
 }
 
+func (s *DockerSuite) TestBuildNotVerboseSuccess(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	// This test makes sure that -q works correctly when build is successful:
+	// stdout has only the image ID (long image ID) and stderr is empty.
+	var stdout, stderr string
+	var err error
+	outRegexp := regexp.MustCompile("^(sha256:|)[a-z0-9]{64}\\n$")
+
+	tt := []struct {
+		Name      string
+		BuildFunc func(string)
+	}{
+		{
+			Name: "quiet_build_stdin_success",
+			BuildFunc: func(name string) {
+				_, stdout, stderr, err = buildImageWithStdoutStderr(name, "FROM busybox", true, "-q", "--force-rm", "--rm")
+			},
+		},
+		{
+			Name: "quiet_build_ctx_success",
+			BuildFunc: func(name string) {
+				ctx, err := fakeContext("FROM busybox", map[string]string{
+					"quiet_build_success_fctx": "test",
+				})
+				if err != nil {
+					c.Fatalf("Failed to create context: %s", err.Error())
+				}
+				defer ctx.Close()
+				_, stdout, stderr, err = buildImageFromContextWithStdoutStderr(name, ctx, true, "-q", "--force-rm", "--rm")
+			},
+		},
+		{
+			Name: "quiet_build_git_success",
+			BuildFunc: func(name string) {
+				git, err := newFakeGit("repo", map[string]string{
+					"Dockerfile": "FROM busybox",
+				}, true)
+				if err != nil {
+					c.Fatalf("Failed to create the git repo: %s", err.Error())
+				}
+				defer git.Close()
+				_, stdout, stderr, err = buildImageFromGitWithStdoutStderr(name, git, true, "-q", "--force-rm", "--rm")
+
+			},
+		},
+	}
+
+	for _, te := range tt {
+		te.BuildFunc(te.Name)
+		if err != nil {
+			c.Fatalf("Test %s shouldn't fail, but got the following error: %s", te.Name, err.Error())
+		}
+		if outRegexp.Find([]byte(stdout)) == nil {
+			c.Fatalf("Test %s expected stdout to match the [%v] regexp, but it is [%v]", te.Name, outRegexp, stdout)
+		}
+		if stderr != "" {
+			c.Fatalf("Test %s expected stderr to be empty, but it is [%#v]", te.Name, stderr)
+		}
+	}
+
+}
+
+func (s *DockerSuite) TestBuildNotVerboseFailure(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	// This test makes sure that -q works correctly when build fails by
+	// comparing between the stderr output in quiet mode and in stdout
+	// and stderr output in verbose mode
+	tt := []struct {
+		TestName  string
+		BuildCmds string
+	}{
+		{"quiet_build_no_from_at_the_beginning", "RUN whoami"},
+		{"quiet_build_unknown_instr", "FROMD busybox"},
+		{"quiet_build_not_exists_image", "FROM busybox11"},
+	}
+
+	for _, te := range tt {
+		_, _, qstderr, qerr := buildImageWithStdoutStderr(te.TestName, te.BuildCmds, false, "-q", "--force-rm", "--rm")
+		_, vstdout, vstderr, verr := buildImageWithStdoutStderr(te.TestName, te.BuildCmds, false, "--force-rm", "--rm")
+		if verr == nil || qerr == nil {
+			c.Fatal(fmt.Errorf("Test [%s] expected to fail but didn't", te.TestName))
+		}
+		if qstderr != vstdout+vstderr {
+			c.Fatal(fmt.Errorf("Test[%s] expected that quiet stderr and verbose stdout are equal; quiet [%v], verbose [%v]", te.TestName, qstderr, vstdout))
+		}
+	}
+}
+
+func (s *DockerSuite) TestBuildNotVerboseFailureRemote(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	// This test ensures that when given a wrong URL, stderr in quiet mode and
+	// stdout and stderr in verbose mode are identical.
+	URL := "http://bla.bla.com"
+	Name := "quiet_build_wrong_remote"
+	_, _, qstderr, qerr := buildImageWithStdoutStderr(Name, "", false, "-q", "--force-rm", "--rm", URL)
+	_, vstdout, vstderr, verr := buildImageWithStdoutStderr(Name, "", false, "--force-rm", "--rm", URL)
+	if qerr == nil || verr == nil {
+		c.Fatal(fmt.Errorf("Test [%s] expected to fail but didn't", Name))
+	}
+	if qstderr != vstdout+vstderr {
+		c.Fatal(fmt.Errorf("Test[%s] expected that quiet stderr and verbose stdout are equal; quiet [%v], verbose [%v]", Name, qstderr, vstdout))
+	}
+}
+
 func (s *DockerSuite) TestBuildStderr(c *check.C) {
 	testRequires(c, DaemonIsLinux)
 	// This test just makes sure that no non-error output goes
@@ -5589,34 +5693,6 @@ func (s *DockerSuite) TestBuildDotDotFile(c *check.C) {
 	}
 }
 
-func (s *DockerSuite) TestBuildNotVerbose(c *check.C) {
-	testRequires(c, DaemonIsLinux)
-	ctx, err := fakeContext("FROM busybox\nENV abc=hi\nRUN echo $abc there", map[string]string{})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	// First do it w/verbose - baseline
-	out, _, err := dockerCmdInDir(c, ctx.Dir, "build", "--no-cache", "-t", "verbose", ".")
-	if err != nil {
-		c.Fatalf("failed to build the image w/o -q: %s, %v", out, err)
-	}
-	if !strings.Contains(out, "hi there") {
-		c.Fatalf("missing output:%s\n", out)
-	}
-
-	// Now do it w/o verbose
-	out, _, err = dockerCmdInDir(c, ctx.Dir, "build", "--no-cache", "-q", "-t", "verbose", ".")
-	if err != nil {
-		c.Fatalf("failed to build the image w/ -q: %s, %v", out, err)
-	}
-	if strings.Contains(out, "hi there") {
-		c.Fatalf("Bad output, should not contain 'hi there':%s", out)
-	}
-
-}
-
 func (s *DockerSuite) TestBuildRUNoneJSON(c *check.C) {
 	testRequires(c, DaemonIsLinux)
 	name := "testbuildrunonejson"