main: return an exit code (12) for "bad password" errors#4959
main: return an exit code (12) for "bad password" errors#4959MichaelEischer merged 1 commit intorestic:masterfrom
Conversation
MichaelEischer
left a comment
There was a problem hiding this comment.
Good idea. I'd like to follow a different way to implement this though, see the comments for details.
92f24a9 to
12ecd55
Compare
cmd/restic/main.go
Outdated
| fmt.Fprintln(os.Stderr, sc.Text()) | ||
| } | ||
| } | ||
| fmt.Fprintf(os.Stderr, "Fatal: %v\n", err) |
There was a problem hiding this comment.
This was a big swing by me - happy to go a different way.
Previously, IsFatal was used to flag "expected errors that will bubble up to main.go" - and thus any other errors should get all this debugging info added, since they were unexpected.
But if we are moving away from fatal errors and just using the original errors instead, we can either (a) add a case statement for each expected error, or (b) drop this extra debugging step for unexpected errors. I chose the latter, but am happy to do the former (or some other idea).
There was a problem hiding this comment.
That will definitely help moving forwards. But in my opinion it is too invasive to include in a patch version. So we'll have to change this separately from adding the exit code if the latter should be included in 0.17.1
There was a problem hiding this comment.
(I myself do not need this to land on a specific schedule - so if you want to save some of the changes for 0.18, I won't be annoyed. 😄)
Let's recap the options here:
- Don't wrap as a FatalError. Just wrap with
fmt.Errorf("%w")- This causes the full stacktrace to be printed to the console for this error, since main.go no longer sees it as an expected error.
- So option (1): drop stacktraces from main.go for all errors (what I've done here)
- Or option (2): drop stacktraces from main.go for just
repository.ErrNoKeyFounderrors
- Or option (3): Allow FatalErrors to properly wrap and hold an inside error. (my original version)
It sounds like option 3 you don't like in general. Option 1 you don't love in a patch version.
But option 2 is maybe fine as a short term fix? And maybe in 0.18 and/or as we grow more "expected error" cases, we can revisit.
There was a problem hiding this comment.
OK, I added option 2 (just a manual check for this one error) to this PR.
MichaelEischer
left a comment
There was a problem hiding this comment.
The changelog entry went missing. Please restore it.
I'd prefer to stick with just warping the errors from opening a repository with fmt.Errorf("Fatal: %w", err) for now. Significantly reworking the error handling should happen separately and also doesn't belong into a patch release.
cmd/restic/main.go
Outdated
| fmt.Fprintln(os.Stderr, sc.Text()) | ||
| } | ||
| } | ||
| fmt.Fprintf(os.Stderr, "Fatal: %v\n", err) |
There was a problem hiding this comment.
That will definitely help moving forwards. But in my opinion it is too invasive to include in a patch version. So we'll have to change this separately from adding the exit code if the latter should be included in 0.17.1
eee8d51 to
657ce77
Compare
cmd/restic/global.go
Outdated
| return nil, err | ||
| } | ||
| return nil, errors.Fatalf("%s", err) | ||
| return nil, err |
There was a problem hiding this comment.
This still changes the error handling when loading the repository key does not work. I'd like to keep the current fatal error handling for those cases.
Either change this to
| return nil, err | |
| if errors.IsFatal(err) || errors.Is(err, repository.ErrNoKeyFound) { | |
| return nil, err | |
| } | |
| return nil, errors.Fatalf("%s", err) |
or
| return nil, err | |
| if errors.IsFatal(err) { | |
| return nil, err | |
| } | |
| return nil, fmt.Errorf("Fatal: %w", err) |
There was a problem hiding this comment.
Ah gotcha - yeah, I can make that change. Suggestion 1 feels better, since it means less errors hitting the "unexpected error, let's show the stacktrace" code flow.
But can I also step back and ask about the thinking around FatalError? I kind of like it as a concept, and still like my original fix here that added wrapping support.
FatalError solves one problem currently, and could be augmented to do more:
- (currently) It flags errors as "expected" so that when
main.gogets an exit error that isn't Fatal, we know to add extra debugging info. - (future) It could hold an exit code (from a centrally maintained list), so that
main.godoesn't need to have its own switch/case list of them. This sort of thing would be particularly handy in the future, when a EACCES syscall errno in one part of the code might mean exit code 72, but in another part of the code would be exit code 76. You'd have to wrap the the syscall.Errno in a custom error anyway so thatmain.gocould disambiguate the cases.
If you are mentally deprecating FatalError, I imagine that main.go's switch/case list is going to keep growing for both those use cases. I personally vibe with the FatalError approach over that.
There was a problem hiding this comment.
OK, this PR is now up-to-date and includes suggestion 1.
Happy to land this while also nattering about FatalError.
There was a problem hiding this comment.
Hmm, another advantage of FatalError-with-code is that it’s easier to test, versus main.go
There was a problem hiding this comment.
If you are mentally deprecating FatalError, I imagine that
main.go's switch/case list is going to keep growing for both those use cases. I personally vibe with the FatalError approach over that.
I haven't though particularly much about how a replacement for the current FatalError should look like. The only thing that's clear is that the current "convert everything into a string"-approach doesn't work. But I also agree that the switch/case list in main.go is already a mess.
2. It could hold an exit code (from a centrally maintained list), so that
main.godoesn't need to have its own switch/case list of them.
Sounds reasonable. I'm not entirely sure yet which layer should make that translation. Should it already happen in internal/ or wait until the cmd/restic/ code? Just blindly wrapping all errors also leaks quite a lot of implementation details, which probably should be avoided.
Quite a lot of code currently just passes an error up the call chain without wrapping it in any way, which makes it rather hard to pinpoint the origin. So, we'll need some mix of both. Not sure how that will look in the end.
The main reason why I don't want these changes in this PR is that they are too invasive for inclusion in a patch release.
What does this PR change? What problem does it solve?
This is useful for a wrapping app to be able to detect that the user provided the wrong key and then re-prompt the user.
As part of this, I stopped converting the error into a fatal error, to avoid swallowing the original error - and then changed how main handles non-fatal errors.
(I did not think this was a big enough change to warrant a changelog entry, but my sense of that is not well-tuned. Please let me know if you'd like one.)
Was the change previously discussed in an issue or on the forum?
No
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.