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.

Sign up for GitHub

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

Jump to bottom

don't obfuscate error during volume create #32268

Merged
merged 1 commit into from
Apr 5, 2017
Merged

don't obfuscate error during volume create #32268

merged 1 commit into from
Apr 5, 2017

Conversation

cpuguy83
Copy link
Member

No description provided.

@cpuguy83 cpuguy83 mentioned this pull request Mar 31, 2017
Closed
vdemeester
vdemeester approved these changes Mar 31, 2017
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@icecrime
Copy link
Contributor

LGTM

tonistiigi
tonistiigi reviewed Mar 31, 2017
View reviewed changes
daemon/create.go Outdated
@@ -243,7 +243,7 @@ func (daemon *Daemon) VolumeCreate(name, driverName string, opts, labels map[str
v, err := daemon.volumes.Create(name, driverName, opts, labels)
if err != nil {
if volumestore.IsNameConflict(err) {
return nil, fmt.Errorf("A volume named %s already exists. Choose a different volume name.", name)
return nil, errors.Wrapf(err, "a volume named %s already exists, choose a different volume name")
Copy link
Member

Choose a reason for hiding this comment

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

missing , name)

Copy link
Member

Choose a reason for hiding this comment

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

oh you are right !

@cpuguy83 cpuguy83 force-pushed the volume_err_obfuscate branch from 02b0f92 to 8f241ce Compare March 31, 2017 16:16
@cpuguy83
Copy link
Member Author

Fixed.

@tonistiigi
Copy link
Member

LGTM

@aaronlehmann
Copy link
Contributor

I think this will lead to very ugly errors. In the simplest case, you'll get:

a volume named foo already exists, choose a different volume name: volume name must be unique

Adding volume name must be unique doesn't do anything to improve the error message. It just makes it redundant.

The only situation where this change would help is if err is something that wraps errNameConflict, in which case we'll have an even longer error message, containing the above plus something else.

@cpuguy83
Copy link
Member Author

@aaronlehmann In the happy case when it's a true name conflict yes, it's not as succinct, however when it's some other issue with a driver it's important to be able to get this information.

This is a situation where we've found a volume in the store with the given name but ran into some error while trying to determine if the entry is stale or not.

@aaronlehmann
Copy link
Contributor

Then can we omit this extra layer of wrapping? The error will always contain volume name must be unique.

@cpuguy83 cpuguy83 force-pushed the volume_err_obfuscate branch from 8f241ce to 90d0978 Compare March 31, 2017 17:29
@cpuguy83
Copy link
Member Author

Done

@aaronlehmann
Copy link
Contributor

LGTM

@tonistiigi
Copy link
Member

With the current version you can just remove the extra condition as the reporting is same for all errors. To my taste is should be "failed to create volume foo: volume name must be unique".

@cpuguy83 cpuguy83 force-pushed the volume_err_obfuscate branch from 90d0978 to 2a15872 Compare March 31, 2017 17:36
@cpuguy83
Copy link
Member Author

Fixed again.

@thaJeztah
Copy link
Member

@cpuguy83 tests are failing

@thaJeztah thaJeztah added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite labels Apr 1, 2017
@cpuguy83 cpuguy83 force-pushed the volume_err_obfuscate branch from 2a15872 to 3b80094 Compare April 4, 2017 01:04
@cpuguy83 cpuguy83 removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 4, 2017
@cpuguy83
Copy link
Member Author

cpuguy83 commented Apr 4, 2017

Tests fixed.

thaJeztah
thaJeztah approved these changes Apr 4, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM (if green 😅

@thaJeztah
Copy link
Member

This a new failure?

17:16:51 
17:16:51 ----------------------------------------------------------------------
17:16:51 FAIL: docker_api_containers_test.go:372: DockerSuite.TestContainerAPITop
17:16:51 
17:16:51 docker_api_containers_test.go:393:
17:16:51     c.Assert(top.Processes[0][10], checker.Equals, "/bin/sh -c top")
17:16:51 ... obtained string = "top"
17:16:51 ... expected string = "/bin/sh -c top"
17:16:51 
17:16:51 
17:16:51 ----------------------------------------------------------------------

@cpuguy83
Copy link
Member Author

cpuguy83 commented Apr 5, 2017

Certainly unrelated. I haven't seen this before, though.

@cpuguy83 cpuguy83 merged commit b7c3b31 into moby:master Apr 5, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 5, 2017
@cpuguy83 cpuguy83 deleted the volume_err_obfuscate branch April 5, 2017 13:05
@thaJeztah thaJeztah mentioned this pull request May 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi left review comments

@vdemeester vdemeester vdemeester approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
status/4-merge
Projects
None yet
Milestone
17.05.0
Development

Successfully merging this pull request may close these issues.
7 participants
@cpuguy83 @icecrime @tonistiigi @aaronlehmann @thaJeztah @vdemeester @GordonTheTurtle