-
Notifications
You must be signed in to change notification settings - Fork 51
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
Handle different errors and provide nice error messages #162
Conversation
cmd/sourced/cmd/web.go
Outdated
@@ -88,6 +102,10 @@ Once source{d} is fully initialized, the UI will be available, by default at: | |||
|
|||
select { | |||
case err := <-ch: | |||
if shouldFailFast(err) { |
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.
This is unnecessary.
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.
If removed, those errors will be wrapped by a new one, what's wrong in this case
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.
Oh, you did this just to avoid wrapping, sorry. What I meant is that there's no need to fail fast here as it's already the last statement of the function.
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.
yeah. it's a bit confusing
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.
I'll be happy if you propose a better name for this function that filters our errors
workdir.ErrMalformed
dir.ErrNotExist
dir.ErrNotDirectory
Considering it again, it might be better: isNamedError
? wdyt @se7entyse7en?
The underlying problem, which is not trivial imo, is that compose.RunWithIO
can fail in many ways, most of them unknown (or at least for me). And when running web
command, we consider as "expected" all those unknown errors, and we just retry...
But when the error is "known" (from the list above), there is no sense to retry, and we can fail fast with a proper error message.
And I think that there is also another underlying problem: since we're bubbling errors in different places, without distinguishing them, then providing meaningful errors is a difficult task, and then we find these kinds of weird error messages from time to time.
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.
you could eventually wrap also the first error an error occurred while waiting for the container to start
or something along these lines.
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.
I think that refactors exceeds the goal of this PR.
But I pushed b5b2156 that might address your requirement about clarifying different errors that can be returned by openUI
, and that will also avoid your initial —and legit— concern about
if failFast(err) {
return err
} else {
return error.Wrap(err)
}
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.
would that ☝️ be enough?
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.
I'd prefer the refactor as it clearly separates the different types of errors, but it's not a big concern.
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.
I understand your point, and I also think that this function might be too big and with different responsibilities inside, but I do really prefer to avoid refactors when hotfixing. Thanks for being nitpicking (because it helps a lot), and also for being flexible in this case 🙇♂️
@smacker nope. When command execution returns an error, it is printed automatically (I think that it's done by
e.g. $ sourced status
/home/david/.sourced is not a valid config directory: it has no read-write access
Cannot perform this action, config directory is not valid |
I think that all your suggestions were already addressed, so if you agree, this should be ready for the last review from @carlosms (who he is coming back "tomorrow" Monday) |
Signed-off-by: David Pordomingo <[email protected]>
Signed-off-by: David Pordomingo <[email protected]>
Signed-off-by: David Pordomingo <[email protected]>
Signed-off-by: David Pordomingo <[email protected]>
Signed-off-by: David Pordomingo <[email protected]>
cmd/sourced/cmd/root.go
Outdated
case fmt.Sprintf("%T", err) == "*flags.Error": | ||
// syntax error is already logged by go-cli | ||
default: | ||
printRed("Unexpected error") |
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.
Isn't this redundant? I would not print anything in this case, it does not add anything useful for the user
$ sudo mkdir /tmp/aaa
$ HOME=/tmp/aaa sourced status
mkdir /tmp/aaa/.sourced: permission denied
Unexpected error
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.
It was to provide homogeneous error messages, but I assume we can also remove it if it's unexpected;
Anyhow, that should not be "unexpected", will review what happened in that case.
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.
- unknown errors will have no special message but the causing error itself,
- fixed that example, that should be as it follows:
$ HOME=/root/User sourced status
/root/User/.sourced is not a valid config directory: mkdir /root/User/.sourced: permission denied
Cannot perform this action, config directory is not valid
…mmands Signed-off-by: David Pordomingo <[email protected]>
fix #160
current
sourced
could fail in different ways if~/.sourced
does not exist, also some errors are not nice from the user pov.The commits in this PR fixes:
~/.sourced
dir can be created when:init
compose download
workdir.Active
failed__active__
does not exist