Remove Job from docker kill#12248
Conversation
api/server/server.go
Outdated
There was a problem hiding this comment.
I think the signal should be converted to an int in the API layer so that a StatusBadRequest can be returned for things like this as int is the final type for this var.
There was a problem hiding this comment.
+1
Then we can remove daemon.ContainerKill
There was a problem hiding this comment.
I thought about that, but I wasn't sure if we wanted callers to ContainerKill be able to pass in both ints and the string version w/o forcing everyone to convert it themselves.
There was a problem hiding this comment.
I think we should enforce type. That was whole idea of removing jobs.
9be85f3 to
dc90e66
Compare
There was a problem hiding this comment.
What will be if sig == ""?
Can we move whole ContainerKill here to work with container instance?
There was a problem hiding this comment.
If sig == "" then the normal container.Kill() will be done.
I thought about removing ContainerKill() but then each potential caller would need to do the logging themselves and I didn't like the idea of that.
There was a problem hiding this comment.
Ok, just looks weird comparing to other merged PRs. pause/unpause in particular. Also I think there won't be other callers.
There was a problem hiding this comment.
This might be a bigger discussion (bordering on bikeshedding :-) ) but to me the api/server code is all about the http transport and I expect we'll eventually have others, so when that day comes I would want logic that isn't specific to the transport to be done within the daemon itself. And I consider logging of a container being killed to be the job of the deamon (or the container itself) but not the api/http/transport layer. So, in that respect I would prefer if we didn't remove those other daemon funcs, if nothing else to have consistency and proper separation of concern.
daemon/kill.go
Outdated
There was a problem hiding this comment.
I'm not sure that I want introduce new event in this PR.
There was a problem hiding this comment.
oh, is there a formal process to get events approved? I assumed this was an old comment and would be easy to fix. I'll revert it.
There was a problem hiding this comment.
actually, can I just reuse the same event from above?
There was a problem hiding this comment.
I think it should be sort of separate PR.
I don't know how to post signal to events - it is sorta break events format.
382026d to
d7ad0f2
Compare
Signed-off-by: Doug Davis <[email protected]>
|
ok I think I addressed all of the comments. |
|
LGTM |
1 similar comment
|
LGTM |
Remove Job from `docker kill`
Signed-off-by: Doug Davis [email protected]