Merge pull request #13106 from duglin/HumanizeCliErrors

Use stderr instead of logrus for CLI error messages

api/client/build.go

@@ -17,7 +17,6 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/graph/tags"
 	"github.com/docker/docker/pkg/archive"
@@ -192,7 +191,7 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
 	// windows: show error message about modified file permissions
 	// FIXME: this is not a valid warning when the daemon is running windows. should be removed once docker engine for windows can build.
 	if runtime.GOOS == "windows" {
-		logrus.Warn(`SECURITY WARNING: You are building a Docker image from Windows against a Linux 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.`)
+		fmt.Fprintln(cli.err, `SECURITY WARNING: You are building a Docker image from Windows against a Linux 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.`)
 	}
 
 	var body io.Reader

api/client/cli.go

@@ -63,6 +63,14 @@ var funcMap = template.FuncMap{
 	},
 }
 
+func (cli *DockerCli) Out() io.Writer {
+	return cli.out
+}
+
+func (cli *DockerCli) Err() io.Writer {
+	return cli.err
+}
+
 func (cli *DockerCli) getMethod(args ...string) (func(...string) error, bool) {
 	camelArgs := make([]string, len(args))
 	for i, s := range args {

api/client/exec.go

@@ -71,7 +71,7 @@ func (cli *DockerCli) CmdExec(args ...string) error {
 	defer func() {
 		logrus.Debugf("End of CmdExec(), Waiting for hijack to finish.")
 		if _, ok := <-hijacked; ok {
-			logrus.Errorf("Hijack did not finish (chan still open)")
+			fmt.Fprintln(cli.err, "Hijack did not finish (chan still open)")
 		}
 	}()
 
@@ -109,7 +109,7 @@ func (cli *DockerCli) CmdExec(args ...string) error {
 
 	if execConfig.Tty && cli.isTerminalIn {
 		if err := cli.monitorTtySize(execID, true); err != nil {
-			logrus.Errorf("Error monitoring TTY size: %s", err)
+			fmt.Fprintf(cli.err, "Error monitoring TTY size: %s\n", err)
 		}
 	}
 

api/client/run.go

@@ -133,7 +133,7 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 	defer func() {
 		logrus.Debugf("End of CmdRun(), Waiting for hijack to finish.")
 		if _, ok := <-hijacked; ok {
-			logrus.Errorf("Hijack did not finish (chan still open)")
+			fmt.Fprintln(cli.err, "Hijack did not finish (chan still open)")
 		}
 	}()
 	if config.AttachStdin || config.AttachStdout || config.AttachStderr {
@@ -183,7 +183,7 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 	defer func() {
 		if *flAutoRemove {
 			if _, _, err = readBody(cli.call("DELETE", "/containers/"+createResponse.ID+"?v=1", nil, nil)); err != nil {
-				logrus.Errorf("Error deleting container: %s", err)
+				fmt.Fprintf(cli.err, "Error deleting container: %s\n", err)
 			}
 		}
 	}()
@@ -195,7 +195,7 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 
 	if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && cli.isTerminalOut {
 		if err := cli.monitorTtySize(createResponse.ID, false); err != nil {
-			logrus.Errorf("Error monitoring TTY size: %s", err)
+			fmt.Fprintf(cli.err, "Error monitoring TTY size: %s\n", err)
 		}
 	}
 

api/client/start.go

@@ -30,7 +30,7 @@ func (cli *DockerCli) forwardAllSignals(cid string) chan os.Signal {
 				}
 			}
 			if sig == "" {
-				logrus.Errorf("Unsupported signal: %v. Discarding.", s)
+				fmt.Fprintf(cli.err, "Unsupported signal: %v. Discarding.\n", s)
 			}
 			if _, _, err := readBody(cli.call("POST", fmt.Sprintf("/containers/%s/kill?signal=%s", cid, sig), nil, nil)); err != nil {
 				logrus.Debugf("Error sending signal: %s", err)
@@ -96,7 +96,7 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 		defer func() {
 			logrus.Debugf("CmdStart() returned, defer waiting for hijack to finish.")
 			if _, ok := <-hijacked; ok {
-				logrus.Errorf("Hijack did not finish (chan still open)")
+				fmt.Fprintln(cli.err, "Hijack did not finish (chan still open)")
 			}
 			cli.in.Close()
 		}()
@@ -149,7 +149,7 @@ func (cli *DockerCli) CmdStart(args ...string) error {
 	if *openStdin || *attach {
 		if tty && cli.isTerminalOut {
 			if err := cli.monitorTtySize(cmd.Arg(0), false); err != nil {
-				logrus.Errorf("Error monitoring TTY size: %s", err)
+				fmt.Fprintf(cli.err, "Error monitoring TTY size: %s\n", err)
 			}
 		}
 		if attchErr := <-cErr; attchErr != nil {

api/client/version.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"runtime"
 
-	"github.com/Sirupsen/logrus"
 	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/autogen/dockerversion"
@@ -40,7 +39,7 @@ func (cli *DockerCli) CmdVersion(args ...string) error {
 
 	var v types.Version
 	if err := json.NewDecoder(stream).Decode(&v); err != nil {
-		logrus.Errorf("Error reading remote version: %s", err)
+		fmt.Fprintf(cli.err, "Error reading remote version: %s\n", err)
 		return err
 	}
 

docker/docker.go

@@ -46,7 +46,8 @@ func main() {
 	if *flLogLevel != "" {
 		lvl, err := logrus.ParseLevel(*flLogLevel)
 		if err != nil {
-			logrus.Fatalf("Unable to parse logging level: %s", *flLogLevel)
+			fmt.Fprintf(os.Stderr, "Unable to parse logging level: %s\n", *flLogLevel)
+			os.Exit(1)
 		}
 		setLogLevel(lvl)
 	} else {
@@ -70,7 +71,12 @@ func main() {
 		}
 		defaultHost, err := opts.ValidateHost(defaultHost)
 		if err != nil {
-			logrus.Fatal(err)
+			if *flDaemon {
+				logrus.Fatal(err)
+			} else {
+				fmt.Fprint(os.Stderr, err)
+			}
+			os.Exit(1)
 		}
 		flHosts = append(flHosts, defaultHost)
 	}
@@ -87,7 +93,8 @@ func main() {
 	}
 
 	if len(flHosts) > 1 {
-		logrus.Fatal("Please specify only one -H")
+		fmt.Fprintf(os.Stderr, "Please specify only one -H")
+		os.Exit(0)
 	}
 	protoAddrParts := strings.SplitN(flHosts[0], "://", 2)
 
@@ -108,7 +115,8 @@ func main() {
 		certPool := x509.NewCertPool()
 		file, err := ioutil.ReadFile(*flCa)
 		if err != nil {
-			logrus.Fatalf("Couldn't read ca cert %s: %s", *flCa, err)
+			fmt.Fprintf(os.Stderr, "Couldn't read ca cert %s: %s\n", *flCa, err)
+			os.Exit(1)
 		}
 		certPool.AppendCertsFromPEM(file)
 		tlsConfig.RootCAs = certPool
@@ -123,7 +131,8 @@ func main() {
 			*flTls = true
 			cert, err := tls.LoadX509KeyPair(*flCert, *flKey)
 			if err != nil {
-				logrus.Fatalf("Couldn't load X509 key pair: %q. Make sure the key is encrypted", err)
+				fmt.Fprintf(os.Stderr, "Couldn't load X509 key pair: %q. Make sure the key is encrypted\n", err)
+				os.Exit(1)
 			}
 			tlsConfig.Certificates = []tls.Certificate{cert}
 		}
@@ -140,11 +149,13 @@ func main() {
 	if err := cli.Cmd(flag.Args()...); err != nil {
 		if sterr, ok := err.(client.StatusError); ok {
 			if sterr.Status != "" {
-				logrus.Println(sterr.Status)
+				fmt.Fprintln(cli.Err(), sterr.Status)
+				os.Exit(1)
 			}
 			os.Exit(sterr.StatusCode)
 		}
-		logrus.Fatal(err)
+		fmt.Fprintln(cli.Err(), err)
+		os.Exit(1)
 	}
 }
 

integration-cli/docker_cli_build_test.go

@@ -4808,7 +4808,7 @@ func (s *DockerSuite) TestBuildRenamedDockerfile(c *check.C) {
 		c.Fatalf("test5 was supposed to fail to find passwd")
 	}
 
-	if expected := fmt.Sprintf("The Dockerfile (%s) must be within the build context (.)", strings.Replace(nonDockerfileFile, `\`, `\\`, -1)); !strings.Contains(out, expected) {
+	if expected := fmt.Sprintf("The Dockerfile (%s) must be within the build context (.)", nonDockerfileFile); !strings.Contains(out, expected) {
 		c.Fatalf("wrong error messsage:%v\nexpected to contain=%v", out, expected)
 	}
 
@@ -5404,7 +5404,7 @@ func (s *DockerSuite) TestBuildBadCmdFlag(c *check.C) {
 		c.Fatal("Build should have failed")
 	}
 
-	exp := `"Unknown flag: boo"`
+	exp := "\nUnknown flag: boo\n"
 	if !strings.Contains(out, exp) {
 		c.Fatalf("Bad output\nGot:%s\n\nExpected to contain:%s\n", out, exp)
 	}
@@ -5421,7 +5421,7 @@ func (s *DockerSuite) TestBuildRUNErrMsg(c *check.C) {
 		c.Fatal("Should have failed to build")
 	}
 
-	exp := "The command '/bin/sh -c badEXE a1 \\\\& a2\\ta3' returned a non-zero code: 127"
+	exp := `The command '/bin/sh -c badEXE a1 \& a2	a3' returned a non-zero code: 127`
 	if !strings.Contains(out, exp) {
 		c.Fatalf("RUN doesn't have the correct output:\nGot:%s\nExpected:%s", out, exp)
 	}

integration-cli/docker_cli_daemon_test.go

@@ -907,8 +907,8 @@ func (s *DockerDaemonSuite) TestDaemonLoggingDriverNoneLogsError(c *check.C) {
 	if err == nil {
 		c.Fatalf("Logs should fail with \"none\" driver")
 	}
-	if !strings.Contains(out, `\"logs\" command is supported only for \"json-file\" logging driver`) {
-		c.Fatalf("There should be error about non-json-file driver, got %s", out)
+	if !strings.Contains(out, `"logs" command is supported only for "json-file" logging driver`) {
+		c.Fatalf("There should be error about non-json-file driver, got: %s", out)
 	}
 }
 

integration-cli/docker_cli_help_test.go

@@ -152,3 +152,32 @@ func (s *DockerSuite) TestHelpTextVerify(c *check.C) {
 	}
 
 }
+
+func (s *DockerSuite) TestHelpErrorStderr(c *check.C) {
+	// If we had a generic CLI test file this one shoudl go in there
+
+	cmd := exec.Command(dockerBinary, "boogie")
+	out, ec, err := runCommandWithOutput(cmd)
+	if err == nil || ec == 0 {
+		c.Fatalf("Boogie command should have failed")
+	}
+
+	expected := "docker: 'boogie' is not a docker command. See 'docker --help'.\n"
+	if out != expected {
+		c.Fatalf("Bad output from boogie\nGot:%s\nExpected:%s", out, expected)
+	}
+
+	cmd = exec.Command(dockerBinary, "rename", "foo", "bar")
+	out, ec, err = runCommandWithOutput(cmd)
+	if err == nil || ec == 0 {
+		c.Fatalf("Rename should have failed")
+	}
+
+	expected = `Error response from daemon: no such id: foo
+Error: failed to rename container named foo
+`
+	if out != expected {
+		c.Fatalf("Bad output from rename\nGot:%s\nExpected:%s", out, expected)
+	}
+
+}

integration-cli/docker_cli_run_test.go

@@ -2051,7 +2051,7 @@ func (s *DockerSuite) TestRunWithBadDevice(c *check.C) {
 	if err == nil {
 		c.Fatal("Run should fail with bad device")
 	}
-	expected := `\"/etc\": not a device node`
+	expected := `"/etc": not a device node`
 	if !strings.Contains(out, expected) {
 		c.Fatalf("Output should contain %q, actual out: %q", expected, out)
 	}