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

Improve duplicate name error message on container create #190

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

marcov
Copy link
Contributor

@marcov marcov commented Jun 19, 2018

Make the error message more informative by specifying the duplicate name
and the existing container ID. Implements #189.

Opening this PR as asked by @rhatdan and agreed with @vrothberg

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2018

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2018

@marcov I think you need to update the ffjson files. If you run make does it recompile them?

@vrothberg
Copy link
Member

LGTM (given the CI passes)

Note that this PR does not yet fully implement #189 as there are other error cases that could be improved in a similar fashion (see https://github.com/containers/storage/blob/master/errors.go). Such cases can be duplicate ID, or layer in use, etc. I suggest looking at the code using the errors and see what can be done (in follow-up PRs).

@nalind
Copy link
Member

nalind commented Jun 19, 2018

@marcov could you go install github.com/pquerna/ffjson, run ffjson containers.go, and add the updated containers_ffjson.go to the patch? That should keep CI from trying to do the same at build-time.

@marcov
Copy link
Contributor Author

marcov commented Jun 19, 2018

@rhatdan, @nalind: I tried to run ffjson locally and it just regenerate exactly the same containers_ffjson.go.

The CI logs make this look more like an issue in the CI env:

rm -f containers_ffjson.go
ffjson containers.go
make: ffjson: Command not found
Makefile:33: recipe for target 'containers_ffjson.go' failed
make: *** [containers_ffjson.go] Error 127

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2018

I think the makefile wants to generate an updated version. If you run make locally does it execute the ffjson command?

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2018

OK I ran it locally and it attempted the compile and changed nothing. But we still need a new file. Just modify a comment in the generated file and then commit and push, and the problem should be fixed.

Signed-off-by: Marco Vedovati <[email protected]>

Make the error message more informative by specifying the duplicate name
and the existing container ID.
@marcov
Copy link
Contributor Author

marcov commented Jun 19, 2018

OK did like you said.

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2018

Thanks Lets see if it passes tests.

@rhatdan
Copy link
Member

rhatdan commented Jun 19, 2018

Woo Hoo, the hack worked.
@nalind PTAL

@nalind
Copy link
Member

nalind commented Jun 19, 2018

LGTM, merging.

@nalind nalind merged commit 51f1f85 into containers:master Jun 19, 2018
@marcov marcov deleted the better-failmsg branch June 19, 2018 18:56
@marcov marcov restored the better-failmsg branch June 19, 2018 18:56
@marcov marcov deleted the better-failmsg branch June 19, 2018 18:56
@marcov marcov restored the better-failmsg branch June 19, 2018 18:57
@vrothberg vrothberg mentioned this pull request Aug 30, 2018
@runcom runcom mentioned this pull request Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants