Remove job from logs #12304
Remove job from logs #12304
Conversation
e9aaaf4 to
77b6514
Compare
611a19a to
0c18630
Compare
|
LGTM |
daemon/logs.go
Outdated
There was a problem hiding this comment.
This is simply config.Stdout == nil && config.Stderr == nil
api/server/server.go
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actualy I think you can do this with additional config parameter, like muxStreams. Api cares only about w.
There was a problem hiding this comment.
@LK4D4 how do you suggest to go with this? moving container check inside d.ContainerLogs?
There was a problem hiding this comment.
There is already check inside ContainerLogs. I was mostly concerned about c.Config.Tty.
There was a problem hiding this comment.
container.Config.Tty value is passed also in cli code
There was a problem hiding this comment.
There was a problem hiding this comment.
what if I do the multiplexing inside d.ContainerLogs? would it be ok? I'll just pass a writeFlusher to ContainerLogs then @LK4D4
There was a problem hiding this comment.
If it'll allow you to remove double check of d.Get, then okay.
|
@LK4D4 take a look, I've moved the checks and mux inside |
d23911b to
9d4ee49
Compare
There was a problem hiding this comment.
this is not really needed but I think it makes code more readable from here...
cc581bd to
caa362d
Compare
daemon/logs.go
Outdated
There was a problem hiding this comment.
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 <me@runcom.ninja>
|
LGTM |
Signed-off-by: Antonio Murdaca me@runcom.ninja