feat: Add uc service start/stop commands
#195
Conversation
ca9b794 to
5f51eff
Compare
psviderski
left a comment
There was a problem hiding this comment.
LGTM! What do you think about supporting the timeout and signal flags for stop like docker stop supports?
We can address this in the follow-up PR if you like but would be good to do this to not come back to stop in the near future. Just let me know.
Regarding start, I can't think of anything immediately necessary.
Co-authored-by: Pasha Sviderski <me@psviderski.name>
|
Good call on the stop signal/timeout options. Added. |
| } | ||
| if opts.timeout != 0 { | ||
| stopOpts.Timeout = &opts.timeout | ||
| } |
There was a problem hiding this comment.
It seems this behaviour is different to the default docker's behaviour of --timeout. Please see https://github.com/docker/cli/blob/master/cli/command/container/stop.go#L60-L64 how it handles when --timeout is not set or not (.Changed("timeout")).
Note that 0 is valid value that is different to nil: https://github.com/moby/moby/blob/8068dfb686a09fa66a577a4b94646d15349802dc/client/container_stop.go#L16-L24
There was a problem hiding this comment.
Updated to use the same flag Changed check to set the timeout option properly.
| }, | ||
| } | ||
| cmd.Flags().StringVarP(&opts.signal, "signal", "s", "", "Signal to send to the container") | ||
| cmd.Flags().IntVarP(&opts.timeout, "timeout", "t", 0, "Seconds to wait before killing the container") |
There was a problem hiding this comment.
Note: I kept the "default" 0 here as that's what the docker code does: https://github.com/docker/cli/blob/master/cli/command/container/stop.go#L48
Basic implementation of service start/stop commands from #183.
The linter isn't happy with the duplication, but I'm not sure the best way to deal with that while also keeping with project conventions...edit: ignoring linter complaints about duplication for these commands.