Improve error system #3375
Improve error system #3375
Conversation
knative-prow
bot
added
approved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3375 +/- ##
==========================================
+ Coverage 53.13% 55.47% +2.33%
==========================================
Files 173 173
Lines 20096 20112 +16
==========================================
+ Hits 10679 11158 +479
+ Misses 8383 7833 -550
- Partials 1034 1121 +87
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lkingland
left a comment
lkingland
left a comment
There was a problem hiding this comment.
Love this cleanup. Proper Go error patterns (Unwrap, Is etc), comand-aware messages, all the definitions centralized (quarantined?), follows the architecture proposed in the ticket well... and a significant cleanup overall.
Just a few comments below, and we need to see if those E2E failures are a flake after that.
👍🏻
| func wrapValidateError(err error, cmd string) error { | ||
| if cmd != "build" && cmd != "deploy" { | ||
| msg := fmt.Sprintf(` | ||
| Internal error during error-wrapping: specified cmd '%s' not supported`, cmd) | ||
| return fmt.Errorf("%w\n%s", err, msg) | ||
| } |
There was a problem hiding this comment.
This returns an "internal error" for any other commandh. Maybe just pass through the original error for unsupported commands?
There was a problem hiding this comment.
The error is being passed through there using %w as well as at the end of function when the internal error couldnt be matched -- as a fallback to simply return what was passed in.
Did you mean to get rid of this Sprintf wrapping and simply return that internal error as is?
gauron99
force-pushed
the
improve-error-system
branch
from
02ab1d6 to
11f47b6
Compare
|
/lgtm |
|
/retest |
gauron99
force-pushed
the
improve-error-system
branch
from
11f47b6 to
4826247
Compare
gauron99
force-pushed
the
improve-error-system
branch
from
4826247 to
54f2fed
Compare
|
new commit to main, had to rebase |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauron99, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
Introduced 2-layer error system:
Layer 1 (pkg/functions/errors.go): Technical/ library errors (short to the point)
Layer 2 (cmd/errors.go): CLI-friendly wrappers with examples and troubleshooting (user oriented)
Added new error types which now use
Error()andUnwrap()for usingerrors.Is()/As()Removed the verbosity from CMD package
/fix #3051