Ignore the daemon log config when building images by dnephin · Pull Request #29552 · moby/moby

dnephin · 2016-12-19T20:18:55Z

Fixes #19259

I believe the log config setting should be ignored by the builder. The logs should also be handled by the daemon.

AkihiroSuda · 2016-12-20T04:36:35Z

integration-cli/docker_cli_daemon_test.go

nit: can you consider adding some description comment to this test?

What sort of descriptive comment? I prefer to just use a descriptive name.

I was considering something like // Test for Issue 19259: the daemon log config should be ignored when building images.
But it is ok to use a descriptive name.

Just IMO, TestDaemonLoggingDriverShouldBeIgnoredForBuild would be more descriptive? (too verbose? 😅 )

Cool, I've updated the test name

AkihiroSuda · 2016-12-20T04:37:58Z

IMO we should set impact/changelog label for this PR.
WDYT? @dnephin @thaJeztah

thaJeztah · 2016-12-20T10:35:01Z

Yes, setting changelog SGTM

thaJeztah · 2016-12-20T10:44:08Z

Per the discussion on #19259 (comment), we could also consider using logs=none, or would that be too much of a change?

Alternatively, set logs=none by default, and use logs=json-file if --rm=false is set

dnephin · 2016-12-20T14:59:37Z

I believe if you use logs=none then you don't see the output during the build.

LK4D4 · 2017-01-26T18:25:51Z

@dnephin I always using none and I see the output, it depends on attach and not logs.
This is definitely a bug IMO, so I'm putting it in code-review.

vdemeester · 2017-01-27T16:58:56Z

@dnephin needs a rebase 👼

dnephin · 2017-01-30T15:08:42Z

rebased

LK4D4 · 2017-01-30T15:55:03Z

Still insisting on trying "none" :)

Logs created by build containers should be handled by the daemon, not by logging drivers. Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin · 2017-01-30T16:01:31Z

Looks like none works! I've made that change

cpuguy83

LGTM

vdemeester

LGTM 🐸

GordonTheTurtle added the status/0-triage label Dec 19, 2016

AkihiroSuda reviewed Dec 20, 2016

View reviewed changes

AkihiroSuda added status/1-design-review and removed status/0-triage labels Dec 20, 2016

thaJeztah added the impact/changelog label Dec 20, 2016

thaJeztah mentioned this pull request Dec 20, 2016

Expose appropriate docker run options to docker build #19259

Closed

dnephin force-pushed the fix-build-with-log-driver branch from 0f85707 to be28ce6 Compare December 27, 2016 20:45

dnephin added the rebuild/windowsRS1 label Dec 28, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Dec 28, 2016

dnephin force-pushed the fix-build-with-log-driver branch from be28ce6 to 3b86df7 Compare December 29, 2016 19:33

dnephin added the rebuild/windowsRS1 label Dec 30, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Dec 30, 2016

LK4D4 added status/2-code-review and removed status/1-design-review labels Jan 26, 2017

dnephin force-pushed the fix-build-with-log-driver branch from 3b86df7 to 7af3969 Compare January 30, 2017 15:08

Ignore the daemon log config when building images.

80b642f

Logs created by build containers should be handled by the daemon, not by logging drivers. Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the fix-build-with-log-driver branch from 7af3969 to 80b642f Compare January 30, 2017 16:01

cpuguy83 approved these changes Feb 7, 2017

View reviewed changes

vdemeester approved these changes Feb 7, 2017

View reviewed changes

dnephin merged commit 1b4e2b7 into moby:master Feb 7, 2017

GordonTheTurtle added this to the 1.14.0 milestone Feb 7, 2017

dnephin deleted the fix-build-with-log-driver branch February 7, 2017 20:47

Conversation

dnephin commented Dec 19, 2016

Uh oh!

AkihiroSuda Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Dec 20, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Dec 20, 2016

Uh oh!

thaJeztah commented Dec 20, 2016

Uh oh!

thaJeztah commented Dec 20, 2016

Uh oh!

dnephin commented Dec 20, 2016

Uh oh!

LK4D4 commented Jan 26, 2017

Uh oh!

vdemeester commented Jan 27, 2017

Uh oh!

dnephin commented Jan 30, 2017

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

dnephin commented Jan 30, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants