Change the quiet flag behavior in the build command

Right now, the quiet (-q, --quiet) flag ignores the output
generated from within the container.

However, it ought to be quiet in a way that all kind
of diagnostic output should be ignored, unless the build
process fails.

This patch makes the quiet flag behave in the following way:
 1. If the build process succeeds, stdout contains the image ID
    and stderr is empty.
 2. If the build process fails, stdout is empty and stderr
    has the error message and the diagnostic output of that process.

If the quiet flag is not set, then everything goes to stdout
and error messages, if there are any, go to stderr.

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>

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,6 +207,9 @@ 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 {
@@ -206,6 +217,10 @@ func (br *buildRouter) postBuild(ctx context.Context, w http.ResponseWriter, r *
 	}
 	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() {
-		go func() {
+		errCh <- b.docker.ContainerAttach(cID, nil, b.Stdout, b.Stderr, true)
-			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
-		// Block on reading output from container, stop on err or chan closed
+	if err := <-errCh; err != nil {
-		if err := <-errCh; err != nil {
+		return err
-			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"

integration-cli/docker_utils.go

@@ -1308,6 +1308,47 @@ func buildImageFromContextWithOut(name string, ctx *FakeContext, useCache bool,
 	return id, out, nil
 }
 
+func buildImageFromContextWithStdoutStderr(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, string, string, error) {
+	args := []string{"build", "-t", name}
+	if !useCache {
+		args = append(args, "--no-cache")
+	}
+	args = append(args, buildFlags...)
+	args = append(args, ".")
+	buildCmd := exec.Command(dockerBinary, args...)
+	buildCmd.Dir = ctx.Dir
+
+	stdout, stderr, exitCode, err := runCommandWithStdoutStderr(buildCmd)
+	if err != nil || exitCode != 0 {
+		return "", stdout, stderr, fmt.Errorf("failed to build the image: %s", stdout)
+	}
+	id, err := getIDByName(name)
+	if err != nil {
+		return "", stdout, stderr, err
+	}
+	return id, stdout, stderr, nil
+}
+
+func buildImageFromGitWithStdoutStderr(name string, ctx *fakeGit, useCache bool, buildFlags ...string) (string, string, string, error) {
+	args := []string{"build", "-t", name}
+	if !useCache {
+		args = append(args, "--no-cache")
+	}
+	args = append(args, buildFlags...)
+	args = append(args, ctx.RepoURL)
+	buildCmd := exec.Command(dockerBinary, args...)
+
+	stdout, stderr, exitCode, err := runCommandWithStdoutStderr(buildCmd)
+	if err != nil || exitCode != 0 {
+		return "", stdout, stderr, fmt.Errorf("failed to build the image: %s", stdout)
+	}
+	id, err := getIDByName(name)
+	if err != nil {
+		return "", stdout, stderr, err
+	}
+	return id, stdout, stderr, nil
+}
+
 func buildImageFromPath(name, path string, useCache bool, buildFlags ...string) (string, error) {
 	args := []string{"build", "-t", name}
 	if !useCache {

man/docker-build.1.md

@@ -81,7 +81,7 @@ set as the **URL**, the repository is cloned locally and then sent as the contex
    Always attempt to pull a newer version of the image. The default is *false*.
 
 **-q**, **--quiet**=*true*|*false*
-   Suppress the verbose output generated by the containers. The default is *false*.
+   Suppress the build output and print image ID on success. The default is *false*.
 
 **--rm**=*true*|*false*
    Remove intermediate containers after a successful build. The default is *true*.