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

boaz0 · 2015-10-28T07:37:08Z

This PR answers the requirement in #7727 and closes #17623

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

However, it ought to be a 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.

TODO:

~~Change the quiet part in the docker build docs~~
~~Write a cli-integration test~~
~~Apparently figure out why the tests fail~~

boaz0 · 2015-10-28T07:50:46Z

Ummm... strange, I don't know why it fails. I'll take a look on it, when I get back from work.
/cc @vieux @tiborvass @duglin - may you shed some light what I did wrong? Thanks 😀

boaz0 · 2015-10-29T15:48:34Z

In the console text, this is the failure message I receive:

FAIL: docker_cli_run_test.go:2163: DockerSuite.TestRunVolumesCleanPaths

docker_cli_run_test.go:2188:
c.Fatalf("Volume was not defined for %s/foo\n%q", prefix, out)
... Error: Volume was not defined for /foo
"/var/lib/docker/volumes/eae1f91f83a0117bf546f30f515eae4c92945565aed814652a7aa8174cfa9208/_data"

However, from what I see the DockerSuite.TestRunVolumesCleanPaths test isn't related to the docker build command and thus to my changes.

Am I right? If so, @vdemeester @cpuguy83 can you please restart the Jenkins windows testing job again? Thanks a bunch. 😄

cpuguy83 · 2015-10-29T16:09:53Z

Correct

duglin · 2015-11-02T16:03:44Z

Couple of things...
1 - doing a strcmp is a bit worrying since its possible (although unlikely) that the string we look for might appear more than once in the output.
2 - this will only get the docker cli flow, not the REST APIs

Have you considering doing this on the daemon side? Then the only output we'd see on the wire would be the resulting image id, for both the docker cli and REST APIs.

boaz0 · 2015-11-02T16:12:36Z

@duglin, thanks for your feedback.
1 - I agree.
2 - I thought of doing this, but I have had a feeling I would have to change the behavior in more than one place. Therefore, I decided to implement this in the client side.

Let me figure out a way to implement that on the daemon side, though. Otherwise, I will get rid of the strcmp-way.

Thanks.

boaz0 · 2015-11-11T19:28:04Z

@duglin -- I tried my best here 😄.
Can you give me another code review?

I am not that satisfied with the implementation, though.

thaJeztah · 2015-11-15T08:11:54Z

@ripcurld00d can you add "closes #17623" to the top description?

boaz0 · 2015-11-15T08:36:39Z

@thaJeztah affirmative ;)

duglin · 2015-11-15T21:01:55Z

First, great name for a branch :-)

I think you may need to tweak the logic when there's an error. Right now if the build fails we'll get the error message but that's it - we only see the error message and not anything else. I think when there's an error you should send the entire buffer along with it. Without that extra text it might be really hard for people to know which line of the Dockerfile caused the issue. Granted we could try to be really smart and only return the last few lines, but I think that's probably overkill and when things go wrong I'd prefer to give as much info as possible to help the user fix it - and for all we know, the true issue could be back on the first step of the Dockerfile. They shouldn't have to rerun again (w/o this flag) just to get the info they need to debug.

I'm not fond of the phrase "script-mode", it feels a bit too focused on one particular usecase. What about just -s and --silent ?

duglin · 2015-11-15T21:14:50Z

api/client/build.go

+		outBuf = cli.out
+	} else {
+		// First check whether the Docker daemon supports this feature.
+		serverResp, err := cli.call("GET", "/version", nil, nil)


While I understand why you're doing this, I'm not sure its worth it or consistent with the rest of Docker. I can't think of any other check like this for other new flags. In general, there's no guarantee that newer CLIs will ever work with old servers w.r.t. new features, so its not worth adding this check. Unless other @docker/maintainers want to start to add this check for other flags we may have added recently (and going forward).

@duglin, no problem. I will remove it. Although, I will propose this check later.

boaz0 · 2015-11-16T09:44:07Z

@duglin thanks again for your feedback. I will work on it and do as you suggest.

Thanks.

duglin · 2015-11-23T11:25:16Z

Hi @ripcurld00d - any update on this? Looks like a rebase is needed too.

boaz0 · 2015-11-23T12:40:33Z

Yeah, I have already done git pull --rebase and still working on it. This week I will push a new update. Be watching 😜

cc @duglin

boaz0 · 2015-11-23T21:35:59Z

@duglin - I have done my part. Your feedback, please 🙏 😄
Thanks a lot!

@boxofrox thoughts?

boaz0 · 2015-11-23T21:46:46Z

pkg/httputils/httputils.go

@@ -54,3 +57,75 @@ func ParseServerHeader(hdr string) (*ServerHeader, error) {
 		OS:  strings.TrimSpace(matches[3]),
 	}, nil
 }
+
+// Version represents the Docker version


I guess I will remove these changes, since we're not making any version-check.

boxofrox · 2015-11-24T01:09:01Z

Looks like this will work from the shell like so.

if docker build -st super-cool-project:latest . 2>/dev/null | read IMAGE_ID; then
  docker tag $IMAGE_ID super-cool-project:1.0
fi

I'm happy with that.

stevvooe · 2015-11-25T01:16:37Z

This functionality is provided by "-q" across other commands. Why the departure from that convention?

duglin · 2015-11-25T01:38:35Z

@stevvooe because build already has a "-q" option and its not "quite" enough

boaz0 · 2015-12-19T22:29:17Z

for heaven sake! when does it end?... i have to re-write another component again... 😭

thaJeztah · 2015-12-19T22:55:20Z

sorry @ripcurld00d, let's try to get this merged asap

I noticed build is failing currently;

---> Making bundle: .ensure-httpserver (in bundles/1.10.0-dev/test-integration-cli)
+ go test -test.timeout=120m -check.v github.com/docker/docker/integration-cli
# github.com/docker/docker/integration-cli
./docker_cli_build_test.go:4963: te.StdoutRegexp undefined (type struct { TestName string; BuildCmdsOrURL string } has no field or method StdoutRegexp)
./docker_cli_build_test.go:4971: no new variables on left side of :=
./docker_cli_build_test.go:4971: cannot use []struct { TestName string; BuildCmds string } literal (type []struct { TestName string; BuildCmds string }) as type []struct { TestName string; BuildCmdsOrURL string } in assignment
./docker_cli_build_test.go:4981: te.BuildCmds undefined (type struct { TestName string; BuildCmdsOrURL string } has no field or method BuildCmds)
./docker_cli_build_test.go:4982: te.BuildCmds undefined (type struct { TestName string; BuildCmdsOrURL string } has no field or method BuildCmds)
FAIL    github.com/docker/docker/integration-cli [build failed]

boaz0 · 2015-12-19T22:56:31Z

@thaJeztah not giving up yet! ✊

thaJeztah · 2015-12-19T23:05:17Z

🤘 sorry it's a really bad time with rebases. we're re-organizing loads of things and moving some packages to separate repositories. I really hope we'll be able to get this merged asap

boaz0 · 2015-12-20T00:34:39Z

🏇 let the game begin...
@duglin -- review the code at your free time...

boaz0 · 2015-12-20T11:03:32Z

grrr windows fails again:

FAIL: docker_cli_build_test.go:1874: DockerSuite.TestBuildCancellationKillsSleep

docker_cli_build_test.go:1945:
c.Fatal("failed to observe build container start in timely fashion")
... Error: failed to observe build container start in timely fashion
[....]
FAIL: docker_cli_events_test.go:217: DockerSuite.TestEventsImageImport

docker_cli_events_test.go:254:
c.Fatal("failed to observe image import in timely fashion")
... Error: failed to observe image import in timely fashion

those aren't related to my changes... 😞

duglin · 2015-12-20T12:51:32Z

integration-cli/docker_cli_build_test.go

+	}
+}
+
+func (s *DockerSuite) TestBuildNotVerboseFailureRemote(c *check.C) {


Because of the 4 variants at https://github.com/docker/docker/pull/17428/files#diff-3a1632a75e011091e0bae6f8c8435fd2L93 can we add a test for when the URL points to a github repo and for when the Dockerfile comes from stdin ?

Aside from that I think this is ready to go!

@duglin w/o using FakeGit? like giving a real github repo?

would using FakeGit be bad for the happy-path case? Could use a bogus git repo URL for the failure case.

@duglin why using git repo or stdin in the happy-path case? we know it generates an image ID. I thought that it would be useful for the sad-path case, to confirm that the output is similar to when the verbose mode is uesd.

I was thinking that since we have four different code paths we might run into four different ways that output could be sent to the screen, and we should make sure -q caught all 4.

@duglin uh huh. I see. OK. Let me figure out how to do a test with stdin, though.

@duglin looking at the integration-cli/docker_utils.go, we are already passing the "Dockerfile" through stdin.

Therefore, I am going to add the FakeGit and the FakeContext methods.

ewindisch · 2015-12-20T14:01:58Z

If there is an error, is the exit status set to non-zero? It's not clear on a read-through if this is true (I'd have to dig through the cli package more perhaps to figure this out).

Otherwise looks good.

duglin · 2015-12-20T14:04:15Z

@ewindisch yes it does, that part hasn't changed.

boaz0 · 2015-12-21T07:01:45Z

@duglin done!

EDIT: docker-ps' unit-tests failed. After looking at it, it seems unrelated to my changes. -- #18820 resolves it.

boaz0 · 2015-12-21T13:32:34Z

@thaJeztah can you restart experimental, janky and windows tests, pllz? thanks 🍧

thaJeztah · 2015-12-21T14:31:55Z

@ripcurld00d done! did you rebase, to get the fix #18820 in?

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>

boaz0 · 2015-12-21T14:59:05Z

Now I did 😊

duglin · 2015-12-21T15:37:57Z

LGTM
thanks for the hard work/patience!

duglin · 2015-12-21T15:51:00Z

ping @cpuguy83 @tiborvass

vdemeester · 2015-12-21T16:29:11Z

LGTM 😉

thaJeztah · 2015-12-21T16:59:10Z

Alright!! Looks like we have 2 LGTM's on code, and docs didn't change and already had 2 LGTM's. In my world that means merge 🎉 💃

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

thaJeztah · 2015-12-21T17:01:00Z

Thanks so much @ripcurld00d!

I added impact/changelog; although it can be seen as a bug fix, this flag was currently not really useful, now that it is, I think it's nice to inform people that they can use it again 😄

boaz0 · 2015-12-21T17:11:39Z

👯 💃 OMG OMG OMG OMG 🎉 🎈 👏
I got merged!!!
Thank you all!!! @vdemeester @duglin @thaJeztah @moxiegirl @tiborvass @boxofrox
I couldn't do it w/o you guys. Kudos! 👍

thaJeztah · 2015-12-21T17:32:40Z

jessfraz · 2015-12-21T17:41:48Z

this is adorable

On Mon, Dec 21, 2015 at 9:34 AM, Sebastiaan van Stijn <
notifications@github.com> wrote:

[image: group hug]
https://camo.githubusercontent.com/505375a16e627d84cf7dd975ec18a8275288eaf0/687474703a2f2f692e6b696e6a612d696d672e636f6d2f6761776b65722d6d656469612f696d6167652f75706c6f61642f73616e6e6536743072637a6b756b6e6e747969672e676966

—
Reply to this email directly or view it on GitHub
#17428 (comment).

vdemeester · 2015-12-21T18:19:26Z

🎉

boxofrox · 2015-12-21T21:11:53Z

Congrats @ripcurld00d and thank you for all the work you put into this. I'm looking forward to using this in my shell scripts. My appreciation goes out to the docker team, too. You're doing a great job.

See moby/moby#17428

GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Oct 28, 2015

cpuguy83 added status/1-design-review and removed status/0-triage labels Oct 28, 2015

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Nov 11, 2015

boaz0 mentioned this pull request Nov 15, 2015

Proposal: docker-build flag that displays only the image ID #17623

Closed

boaz0 changed the title ~~Add the stream-off flag to the client~~ Add the script-mode flag to the client [closes #17623] Nov 15, 2015

duglin reviewed Nov 15, 2015
View reviewed changes

boaz0 changed the title ~~Add the script-mode flag to the client [closes #17623]~~ Add the silent flag to the client [closes #17623] Nov 23, 2015

boaz0 reviewed Nov 23, 2015
View reviewed changes

boaz0 mentioned this pull request Dec 20, 2015

Proposal: Add a cidfile flag to docker-build command #18803

Closed

duglin reviewed Dec 20, 2015
View reviewed changes

thaJeztah added a commit that referenced this pull request Dec 21, 2015

Merge pull request #17428 from ripcurld00d/cli_ninja_build

f4ea3b2

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

thaJeztah merged commit f4ea3b2 into moby:master Dec 21, 2015

thaJeztah added the impact/changelog label Dec 21, 2015

davidgiesberg added a commit to davidgiesberg/packer-post-processor-docker-dockerfile that referenced this pull request Feb 4, 2016

Fix for docker build --quiet output format changes in Docker 1.10.0

859807a

See moby/moby#17428

thaJeztah mentioned this pull request Dec 18, 2016

[Feature Request]: docker build should provide an option for more efficient output in batch mode #29507

Closed

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

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

Conversation

boaz0 commented Oct 28, 2015

boaz0 commented Oct 28, 2015

boaz0 commented Oct 29, 2015

cpuguy83 commented Oct 29, 2015

duglin commented Nov 2, 2015

boaz0 commented Nov 2, 2015

boaz0 commented Nov 11, 2015

thaJeztah commented Nov 15, 2015

boaz0 commented Nov 15, 2015

duglin commented Nov 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 commented Nov 16, 2015

duglin commented Nov 23, 2015

boaz0 commented Nov 23, 2015

boaz0 commented Nov 23, 2015

Choose a reason for hiding this comment

boxofrox commented Nov 24, 2015

stevvooe commented Nov 25, 2015

duglin commented Nov 25, 2015

boaz0 commented Dec 19, 2015

thaJeztah commented Dec 19, 2015

boaz0 commented Dec 19, 2015

thaJeztah commented Dec 19, 2015

boaz0 commented Dec 20, 2015

boaz0 commented Dec 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewindisch commented Dec 20, 2015

duglin commented Dec 20, 2015

boaz0 commented Dec 21, 2015

boaz0 commented Dec 21, 2015

thaJeztah commented Dec 21, 2015

boaz0 commented Dec 21, 2015

duglin commented Dec 21, 2015

duglin commented Dec 21, 2015

vdemeester commented Dec 21, 2015

thaJeztah commented Dec 21, 2015

thaJeztah commented Dec 21, 2015

boaz0 commented Dec 21, 2015

thaJeztah commented Dec 21, 2015

jessfraz commented Dec 21, 2015

vdemeester commented Dec 21, 2015

boxofrox commented Dec 21, 2015