Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 27, 2024

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment