Skip to content
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

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

thaJeztah
Copy link
Member

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.

- Description for the changelog

The `--time` option on `docker stop` and `docker restart` is deprecated and renamed to `--timeout`.

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.04%. Comparing base (b1ae218) to head (df8b345).
Report is 2 commits behind head on master.

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              

@thaJeztah
Copy link
Member Author

@krissetto @laurazard I kept the flag descriptions unmodified for now, as that required more discussion; this one is only renaming the flag to --timeout.

Copy link
Contributor

@krissetto krissetto left a 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")
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completion LGTM

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
Copy link
Member Author

@krissetto I updated the error PTAL if it still LGTY ☺️

@krissetto
Copy link
Contributor

@krissetto I updated the error PTAL if it still LGTY ☺️

looks good to go 🚀

@thaJeztah
Copy link
Member Author

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.

@thaJeztah thaJeztah merged commit 3907414 into docker:master Sep 30, 2024
89 checks passed
@thaJeztah thaJeztah deleted the time_or_timeout branch September 30, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants