explicitly handle errors when wrapping them #5851
Conversation
The errors.Wrap and errors.Wrapf functions gracefully handle nil-errors. This allows them to be used unconditionally regardless if an error was produced. While this can be convenient, it can also be err-prone, as replacing these w 8000 ith stdlib errors means they unconditionally produce an error. This patch replaces code uses of errors.Wrap to be gated by a check for nil-errors to future-proof our code. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (22.72%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #5851 +/- ##
==========================================
- Coverage 58.81% 58.74% -0.07%
==========================================
Files 349 349
Lines 29556 29570 +14
==========================================
- Hits 17382 17371 -11
- Misses 11189 11212 +23
- Partials 985 987 +2 |
| @@ -67,7 +67,10 @@ func recursiveInterpolate(value any, path Path, opts Options) (any, error) { | |||
| return newValue, nil | |||
| } | |||
| casted, err := caster(newValue) | |||
| return casted, newPathError(path, errors.Wrap(err, "failed to cast to expected type")) | |||
| if err != nil { | |||
| return casted, newPathError(path, errors.Wrap(err, "failed to cast to expected type")) | |||
There was a problem hiding this comment.
Wasn't sure if caster would still return a value, so I left that one as-is, and just return it (as it did before); it should probably be nil or "something else" though.
| @@ -121,7 +121,10 @@ func (c *client) PutManifest(ctx context.Context, ref reference.Named, manifest | |||
| } | |||
|
|
|||
| dgst, err := manifestService.Put(ctx, manifest, opts...) | |||
| return dgst, errors.Wrapf(err, "failed to put manifest %s", ref) | |||
| if err != nil { | |||
| return dgst, errors.Wrapf(err, "failed to put manifest %s", ref) | |||
There was a problem hiding this comment.
same here, is dgst set on error?
There was a problem hiding this comment.
Yeah, honestly, same situation as my other comment; we should probably return a nil or "zero" value here, because results should (almost) never be consumed if there's an error, but this was also a case where I would have to check all possible implementations; manifestService.Get is an interface, so now the fun starts to check which one we actually use, and how that one behaves 🙈
So I was taking the "safe" side and kept it as-is, leaving that for a future exercise 😅
💯 for return values as well (I made some changes similar in the client some time ago); I like seeing (which could be valid for some sentinel errors, but those should be an exception, so having them stand out as "this is different" is better as well!) |
|
FWIW; we're considering a patch release after the weekend, and want to skip the "cherry-pick" dance and instead fast-forward the release branches. This change should probably do no harm but doesn't fix any bugs or otherwise, so we can decide to keep this one for "later" to keep the diff small (either way would be fine with me though); cc @neersighted @vvoland |
|
Bringing this one in, as we did v28.0.1 |
The errors.Wrap and errors.Wrapf functions gracefully handle nil-errors. This allows them to be used unconditionally regardless if an error was produced.
While this can be convenient, it can also be err-prone, as replacing these with stdlib errors means they unconditionally produce an error.
This patch replaces code uses of errors.Wrap to be gated by a check for nil-errors to future-proof our code.
- What I did
- How I did it
- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)