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

align "conflicting options" errors for consistency #5488

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

thaJeztah
Copy link
Member

- Description for the changelog

Updated some error messages for "conflicting options" to be more consistent

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.05%. Comparing base (3907414) to head (bd96bda).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5488   +/-   ##
=======================================
  Coverage   60.04%   60.05%           
=======================================
  Files         345      345           
  Lines       23440    23440           
=======================================
+ Hits        14074    14076    +2     
+ Misses       8391     8390    -1     
+ Partials      975      974    -1     

@thaJeztah
Copy link
Member Author

Oh! Looks like I broke something; I was testing some things, so perhaps I forgot to restore some code;

#20 60.26 === FAIL: cli/command/container TestParseRestartPolicyAutoRemove (0.00s)
#20 60.26     opts_test.go:823: Expected error Conflicting options: --restart and --rm, but got none

@thaJeztah
Copy link
Member Author

LOL, yeah, that test isn't great as it reports "but got none" which... is a LIE!

func TestParseRestartPolicyAutoRemove(t *testing.T) {
expected := "Conflicting options: --restart and --rm"
_, _, _, err := parseRun([]string{"--rm", "--restart=always", "img", "cmd"}) //nolint:dogsled
if err == nil || err.Error() != expected {
t.Fatalf("Expected error %v, but got none", expected)
}
}

@@ -52,7 +52,7 @@ func newCreateCommand(dockerCli command.Cli) *cobra.Command {
RunE: func(cmd *cobra.Command, args []string) error {
if len(args) == 1 {
if options.name != "" {
return errors.Errorf("conflicting options: either specify --name or provide positional arg, not both")
return errors.Errorf("conflicting options: cannot specify a volume-name through both --name and as a positional arg")
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a specific (i'd guess historical) reason this command has support for both an option and a positional arg for the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there was some back-and-forth whether name should be a positional arg or a flag. The --name was changed to a positional arg, but because it already shipped was kept (but soft-deprecated / hidden).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I may have been partially responsible for that, considering that docker volume create foobar is more convenient than docker volume create --name=foobar, but later discussions were that, because name is optional, using a --flag is a more common convention.

@thaJeztah thaJeztah merged commit f4fab2c into docker:master Oct 1, 2024
89 checks passed
@thaJeztah thaJeztah deleted the align_errs branch October 1, 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.

3 participants