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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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)

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-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 (ab72d63).

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

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.

3 participants