cli/command/container: stop, restart: rename "--time" to "--timeout" #5485
Conversation
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 |
cli/command/container/restart.go
Outdated
| @@ -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.
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
There was a problem hiding this comment.
Yes, agreed; wasn't a fan of the wording either. I recall I went looking for similar errors (because I recalled had some), but looks like I picked the odd one out;
cli/command/cli.go: return errors.New("conflicting options: either specify --host or --context, not both")
cli/command/cli.go: return nil, errors.New("conflicting options: either specify --host or --context, not both")
cli/command/container/opts.go: return nil, errdefs.InvalidParameter(errors.New("conflicting options: cannot attach both user-defined and non-user-defined network-modes"))
cli/command/container/opts.go: return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --network-alias and per-network alias"))
cli/command/container/opts.go: return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --link and per-network links"))
cli/command/container/opts.go: return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --ip and per-network IPv4 address"))
cli/command/container/opts.go: return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --ip6 and per-network IPv6 address"))
cli/command/container/opts.go: return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --mac-address and per-network MAC address"))
cli/command/container/opts.go: return errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --link-local-ip and per-network link-local IP addresses"))
cli/command/container/restart.go: return errors.New("conflicting options: either specify --timeout or --time, not both")
cli/command/container/stop.go: return errors.New("conflicting options: either specify --timeout or --time, not both")
cli/command/volume/create.go: return errors.Errorf("conflicting options: either specify --name or provide positional arg, not both")
cli/command/volume/prune.go: return 0, "", errdefs.InvalidParameter(errors.New("conflicting options: cannot specify both --all and --filter all=1"))
Most use conflicting options: cannot specify both XXX and YYY, which is the error I recalled.
There was a problem hiding this comment.
- opened align "conflicting options" errors for consistency #5488 as a follow-up
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 <[email protected]>
thaJeztah
force-pushed
the
time_or_timeout
branch
from
ab72d63 to
df8b345
Compare
|
@krissetto I updated the error PTAL if it still LGTY |
looks good to go 🚀 |
|
Thanks! I'll bring this in! I may have a look at aligning those other errors in a follow-up, to bring a bit more consistency. |
This renames the
--timeflag as used ondocker stopanddocker restartto--timeout, bringing it in line with other uses for this property, such as--stop-timeoutondocker run.The
--timeoption 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)