Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore the daemon log config when building images #29552

Merged
merged 1 commit into from Feb 7, 2017
Merged

Ignore the daemon log config when building images #29552

merged 1 commit into from Feb 7, 2017

Conversation

dnephin
Copy link
Member

@dnephin dnephin commented Dec 19, 2016

Fixes #19259

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

AkihiroSuda
AkihiroSuda reviewed Dec 20, 2016
View reviewed changes
integration-cli/docker_cli_daemon_test.go
@@ -1193,6 +1193,19 @@ func (s *DockerDaemonSuite) TestDaemonLoggingDriverNoneLogsError(c *check.C) {
c.Assert(out, checker.Contains, expected)
}

func (s *DockerDaemonSuite) TestDaemonLoggingDriverNoneWithBuild(c *check.C) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😅 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I've updated the test name

@AkihiroSuda
Copy link
Member

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

@thaJeztah
Copy link
Member

Yes, setting changelog SGTM

@thaJeztah
Copy link
Member

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

@thaJeztah thaJeztah mentioned this pull request Dec 20, 2016
Closed
@dnephin
Copy link
Member Author

dnephin commented Dec 20, 2016

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

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 26, 2017

@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
Copy link
Member

@dnephin needs a rebase 👼

@dnephin
Copy link
Member Author

dnephin commented Jan 30, 2017

rebased

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 30, 2017

Still insisting on trying "none" :)

@dnephin
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
Copy link
Member Author

dnephin commented Jan 30, 2017

Looks like none works! I've made that change

cpuguy83
cpuguy83 approved these changes Feb 7, 2017
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

vdemeester
vdemeester approved these changes Feb 7, 2017
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐸

@dnephin dnephin merged commit 1b4e2b7 into moby:master Feb 7, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 7, 2017
@dnephin dnephin deleted the fix-build-with-log-driver branch February 7, 2017 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@vdemeester vdemeester vdemeester approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees
No one assigned
Labels
impact/changelog status/2-code-review
Projects
None yet
Milestone
17.04.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dnephin @AkihiroSuda @thaJeztah @LK4D4 @vdemeester @cpuguy83 @GordonTheTurtle