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.

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

Ignore the daemon log config when building images #29552

Merged
merged 1 commit into from Feb 7, 2017

Conversation

Projects
None yet
7 participants
@dnephin
Member

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

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 20, 2016

Member

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

Member

AkihiroSuda commented Dec 20, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 20, 2016

Member

Yes, setting changelog SGTM

Member

thaJeztah commented Dec 20, 2016

Yes, setting changelog SGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 20, 2016

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

Member

thaJeztah commented Dec 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Dec 20, 2016

Member

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

Member

dnephin commented Dec 20, 2016

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

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 26, 2017

Contributor

@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.

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jan 27, 2017

Member

@dnephin needs a rebase 👼

Member

vdemeester commented Jan 27, 2017

@dnephin needs a rebase 👼

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 30, 2017

Member

rebased

Member

dnephin commented Jan 30, 2017

rebased

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

Still insisting on trying "none" :)

Contributor

LK4D4 commented Jan 30, 2017

Still insisting on trying "none" :)

Ignore the daemon log config when building images.
Logs created by build containers should be handled by the daemon, not by logging drivers.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 30, 2017

Member

Looks like none works! I've made that change

Member

dnephin commented Jan 30, 2017

Looks like none works! I've made that change

@cpuguy83

LGTM

@vdemeester

LGTM 🐸

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

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30114 has succeeded
Details
janky Jenkins build Docker-PRs 38727 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9755 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 7, 2017

@dnephin dnephin deleted the dnephin:fix-build-with-log-driver branch Feb 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment