-
Notifications
You must be signed in to change notification settings - Fork 255
Add Stop command on the gRPC side.
#99
Conversation
| rpc Exec(ExecRequest) returns (ExecResponse); | ||
| } | ||
|
|
||
| message Port { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this is on top of #98, otherwise not sure why we have ports, etc. added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snuck in this PR, it's kinda related to #98 but it can stay here, they are independent.
moby/backend.go
Outdated
| } | ||
|
|
||
| func (ms *mobyService) Stop(ctx context.Context, containerName string) error { | ||
| timeout := 1 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 second might be too short. What does docker/cli and docker/compose use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeout is a parameter to stop now
silvin-lubecki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a fix on the options and a test to add, otherwise LGTM
a428ab2 to
c4a04ca
Compare
This is a mixed PR and I am really sorry about it, it contains:
--allflag todocker psallparameter in the interfaces and in the gRPC layerStopcommand to stop a running container (only implemented it in moby, still needs to be implemented for ACI).I just realized there is an inconsistency in the naming,
docker rmcalls theDeletemethod, we would need to create a new methodRemovefor this and not useDeleteonrm.