-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
cli/command/container: stop, restart: rename "--time" to "--timeout" #5485
base: master
Are you sure you want to change the base?
Conversation
This renames the `--time` flag as used on `docker stop` and `docker restart` to `--timeout`, bringing it in line with other uses for this property, such as `--stop-timeout` on `docker run`. The `--time` option is deprecated and hidden, but will be kept for backward compatibility, as these options existed for a long time. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5485 +/- ##
==========================================
+ Coverage 60.03% 60.04% +0.01%
==========================================
Files 345 345
Lines 23434 23440 +6
==========================================
+ Hits 14068 14074 +6
Misses 8391 8391
Partials 975 975 |
@krissetto @laurazard I kept the flag descriptions unmodified for now, as that required more discussion; this one is only renaming the flag to |
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.
LGTM, just left a thought on the conflicting options msg
@@ -30,8 +30,11 @@ func NewRestartCommand(dockerCli command.Cli) *cobra.Command { | |||
Short: "Restart one or more containers", | |||
Args: cli.RequiresMinArgs(1), | |||
RunE: func(cmd *cobra.Command, args []string) error { | |||
if cmd.Flags().Changed("time") && cmd.Flags().Changed("timeout") { | |||
return errors.New("conflicting options: either specify --timeout or --time, not both") |
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.
Thinking of these error messages a bit..instead of telling the users "multiple things" (the error: what to do, and what not to do), what do you think of something like this (the error: description)?
conflicting options: --timeout and --time cannot be used together
This renames the
--time
flag as used ondocker stop
anddocker restart
to--timeout
, bringing it in line with other uses for this property, such as--stop-timeout
ondocker run
.The
--time
option is deprecated and hidden, but will be kept for backward compatibility, as these options existed for a long time.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)