-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(send-signal): new function ContainersSendSignal
to request the send-signal api's endpoint
#295
feat(send-signal): new function ContainersSendSignal
to request the send-signal api's endpoint
#295
Conversation
9bcff82
to
aeccae3
Compare
ContainersSendSignal
the send-signal api's endpoint
465773d
to
9ed8faa
Compare
ContainersSendSignal
the send-signal api's endpointContainersSendSignal
to request the send-signal api's endpoint
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 question otherwise LGTM
container.go
Outdated
@@ -43,3 +43,18 @@ func (c *Client) ContainersStop(ctx context.Context, appName, containerID string | |||
|
|||
return nil | |||
} | |||
|
|||
func (c *Client) ContainersSendSignal(ctx context.Context, app string, signal string, container string) error { |
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.
naming: I'm not completely convinced by the variable name container
. Is it a name (e.g. web-1
) or an ID (e.g. 5c9b95d201dcea2f34d496a3
)? Depending on the answer I would name it containerName
or containerID
.
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.
It's a containerName
, reasons why it's a docker_name
instead of a docker_id
is explained here https://github.com/Scalingo/api/pull/2096#discussion_r1057484461
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 waiting for the merge of the API PR to finish the review here
Fix #294