-
Notifications
You must be signed in to change notification settings - Fork 562
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
cmd/snap-update-ns: improve wording in many errors #5272
Conversation
Many error messages were of the form "cannot create path /full/path: cannot do stuff to segment "path": real reason for the error". Those got simplified to just "cannot do stuff to /path/that/failed: real reason for the error". Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The target (aka oldname) cannot be empty so we should just check it internally in the place that creates those symlinks. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Attempting to open an unclean paths now provide a more complete and correct error message than before. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Codecov Report
@@ Coverage Diff @@
## master #5272 +/- ##
=========================================
Coverage ? 78.84%
=========================================
Files ? 500
Lines ? 37477
Branches ? 0
=========================================
Hits ? 29548
Misses ? 5529
Partials ? 2400
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with one question.
@@ -116,9 +111,6 @@ func (c *Change) createPath(path string, pokeHoles bool, sec *Secure) ([]*Change | |||
// performed the hole poking and thus additional changes must be nil. | |||
_, err = c.createPath(path, false, sec) | |||
} | |||
} else if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably ok and means we will not decorate the error with extra message and just return it as is; is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as a part of this change the errors produced by things called by createPath internally now return appropriate error messages by themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
This branch contains assorted tweaks for how error handling in snap-update-ns looks like.
I will have more similar patches as I work towards the validation solution but I think this is
okay to propose already.