-
Notifications
You must be signed in to change notification settings - Fork 254
Add Stop
command on the gRPC side.
#99
Conversation
@@ -42,16 +42,25 @@ service Containers { | |||
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
@@ -84,6 +90,11 @@ func (ms *mobyService) Run(ctx context.Context, r containers.ContainerConfig) er | |||
return ms.apiClient.ContainerStart(ctx, create.ID, types.ContainerStartOptions{}) | |||
} | |||
|
|||
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
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:
--all
flag todocker ps
all
parameter in the interfaces and in the gRPC layerStop
command 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 rm
calls theDelete
method, we would need to create a new methodRemove
for this and not useDelete
onrm
.