Remove job from logs by runcom · Pull Request #12304 · moby/moby

runcom · 2015-04-11T21:49:55Z

Signed-off-by: Antonio Murdaca [email protected]

duglin · 2015-04-12T14:21:59Z

LGTM

LK4D4 · 2015-04-12T16:26:31Z

daemon/logs.go

This is simply config.Stdout == nil && config.Stderr == nil

LK4D4 · 2015-04-12T17:06:32Z

api/server/server.go

You getting container here and inside daemon. Seems like you have no choice without significant refactoring, but it is not consistent with your previous PR.

Actualy I think you can do this with additional config parameter, like muxStreams. Api cares only about w.

@LK4D4 how do you suggest to go with this? moving container check inside d.ContainerLogs?

There is already check inside ContainerLogs. I was mostly concerned about c.Config.Tty.

container.Config.Tty value is passed also in cli code

https://github.com/docker/docker/blob/master/api/client/logs.go#L54

what if I do the multiplexing inside d.ContainerLogs? would it be ok? I'll just pass a writeFlusher to ContainerLogs then @LK4D4

If it'll allow you to remove double check of d.Get, then okay.

runcom · 2015-04-12T18:54:53Z

@LK4D4 take a look, I've moved the checks and mux inside d.ContainerLogs (I think something can be further simplified though)

runcom · 2015-04-12T19:25:40Z

daemon/logs.go

this is not really needed but I think it makes code more readable from here...

LK4D4 · 2015-04-12T23:48:13Z

daemon/logs.go

I'm pretty sure that version check is not necessary. I blindly copied it from attach, but /logs/ endpoint isn't even exists before "1.6"

Signed-off-by: Antonio Murdaca <[email protected]>

LK4D4 · 2015-04-13T15:38:44Z

LGTM

Remove job from logs

GordonTheTurtle added the status/0-triage label Apr 11, 2015

runcom force-pushed the remove-job-logs branch 2 times, most recently from e9aaaf4 to 77b6514 Compare April 11, 2015 21:54

runcom mentioned this pull request Apr 11, 2015

Remove engine mechanism #12151

Closed

41 tasks

runcom force-pushed the remove-job-logs branch 3 times, most recently from 611a19a to 0c18630 Compare April 12, 2015 00:03

cpuguy83 added status/2-code-review and removed status/0-triage labels Apr 12, 2015

LK4D4 reviewed Apr 12, 2015
View reviewed changes

runcom force-pushed the remove-job-logs branch from 0c18630 to 25dff75 Compare April 12, 2015 16:31

LK4D4 reviewed Apr 12, 2015
View reviewed changes

runcom force-pushed the remove-job-logs branch from 25dff75 to 608590d Compare April 12, 2015 18:54

runcom force-pushed the remove-job-logs branch 2 times, most recently from d23911b to 9d4ee49 Compare April 12, 2015 19:24

runcom reviewed Apr 12, 2015
View reviewed changes

daemon/logs.go

Copy link

Member Author

runcom Apr 12, 2015

Choose a reason for hiding this comment

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

this is not really needed but I think it makes code more readable from here...

runcom force-pushed the remove-job-logs branch 2 times, most recently from cc581bd to caa362d Compare April 12, 2015 20:31

LK4D4 reviewed Apr 12, 2015
View reviewed changes

Remove job from logs

91bfed6

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the remove-job-logs branch from caa362d to 91bfed6 Compare April 13, 2015 06:25

LK4D4 added a commit that referenced this pull request Apr 13, 2015

Merge pull request #12304 from runcom/remove-job-logs

bfb487d

Remove job from logs

LK4D4 merged commit bfb487d into moby:master Apr 13, 2015

runcom deleted the remove-job-logs branch May 8, 2015 18:44

Comments

Conversation

runcom commented Apr 11, 2015

Uh oh!

duglin commented Apr 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Apr 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants