ROX-10259: Remove deprecated errorhelpers errors#1372
Conversation
…ntViolation, ErrNoCredentials, ErrNoValidRole, ErrNotAuthorized, ErrNoAuthzConfigured
|
Tag for build #457678 is 💻 For deploying this image using the dev scripts, run the following first: export MAIN_IMAGE_TAG='3.69.x-463-ga0129aa273'🕹️ A |
| } | ||
| if !exists { | ||
| return nil, errors.Wrapf(errorhelpers.ErrNotFound, "alert with id '%s' does not exist", request.GetId()) | ||
| return nil, errors.Wrapf(errox.NotFound, "alert with id '%s' does not exist", request.GetId()) |
There was a problem hiding this comment.
Would it be possible to replace such cases with
| return nil, errors.Wrapf(errox.NotFound, "alert with id '%s' does not exist", request.GetId()) | |
| return nil, errox.NotFound.Newf("alert with id '%s' does not exist", request.GetId()) |
?
That would remove the useless "not found" from the resulting error message.
There was a problem hiding this comment.
@0x656b694d Thank you for the suggestion. We want to make changes in several PRs so that it's easier to review them. This PR is the simple removal of pkg/errorhelpers/errors.go. Next will be removing helper (wrapper) functions for CausedBy. After that, we can start with further polishing. We can decide if we want to keep the message prefix or remove it.
rukletsov
left a comment
rukletsov
left a comment
There was a problem hiding this comment.
Looks like a bulk replace — no brainer. I left a couple of comments, not as immediate suggestions but more as sharing my thoughts about the current state and possible improvements.
| return s.getScannerCertExpiry(ctx) | ||
| } | ||
| return nil, errors.Wrapf(errorhelpers.ErrInvalidArgs, "invalid component: %v", request.GetComponent()) | ||
| return nil, errors.Wrapf(errox.InvalidArgs, "invalid component: %v", request.GetComponent()) |
There was a problem hiding this comment.
FWIW, this another example of a useless prefix: "invalid arguments: invalid component: orange ocean"
| if len(result) < len(ids) { | ||
| missingIds := set.NewStringSet(ids...).Difference(search.ResultsToIDSet(result)) | ||
| return errors.Wrapf(errorhelpers.ErrNotFound, "Following CVEs not found: %s", strings.Join(missingIds.AsSlice(), ", ")) | ||
| return errors.Wrapf(errox.NotFound, "Following CVEs not found: %s", strings.Join(missingIds.AsSlice(), ", ")) |
There was a problem hiding this comment.
And another one, introducing tautology (and capitalized!): "not found: Following CVEs not found: CVE-xxyyz"
| } | ||
| if err := reconcileNotifierConfigWithExisting(updateRequest.GetNotifier(), existingNotifierConfig); err != nil { | ||
| return errors.Wrap(errorhelpers.ErrInvalidArgs, err.Error()) | ||
| return errors.Wrap(errox.InvalidArgs, err.Error()) |
There was a problem hiding this comment.
And this is a good use case for .CausedBy() I think. The pattern errors.Wrap(sentinel, err.Error()) was mostly introduced in https://github.com/stackrox/rox/pull/8239 where we replaced gRPC status errors with this pattern (because we introduced automatic sentinel -> gRPC error conversion before). However, I'm not sure whether all occurrences of errors.Wrap(sentinel, err.Error()) can be auto replaced, probably we'll need to look at each of them.
It is unfortunate that we only keep the error message here. In the future we might want to also track and print stack traces to the log and having a tailored version .CausedBy(errorWithStackTrace) will help us to preserve the stack of the cause.
3 participants
Description
We have removed deprecated
errorhelperserrors and changed to a new approach provided byerroxpackage.Note: If a review of the whole PR is difficult because we are touching too many files here then it is possible to review commit by commit.
Checklist
[ ] Unit test and regression tests added- nothing is added, some of them are modified to match replacements[ ] Evaluated and added CHANGELOG entry if required- not relevant because underlying changes are done[ ] Determined and documented upgrade steps- nothing relevant for upgrade steps[ ] Documented user facing changes (create PR based on stackrox/openshift-docs and merge into rhacs-docs)- no user-facing changesTesting Performed
The following things are tested locally:
make imageworksmake styleworks, so that the linting of all go files is correct. With it we are checking that correct packages are included or removed