Expose meaningful error text from yamlpatch #5273
Conversation
The faulty JSONPatch tests now ensure that the resulting error message actually mentions the faulty value. Fixes: GoogleContainerTools#5259 Signed-off-by: Paul "Hampy" Hampson <p_hampson@wargaming.net>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #5273 +/- ##
=======================================
Coverage 71.92% 71.92%
=======================================
Files 389 389
Lines 14149 14149
=======================================
Hits 10176 10176
Misses 3226 3226
Partials 747 747
Continue to review full report at Codecov.
|
gsquared94
added
kokoro:force-run
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@TBBle the CLA issue seems to have been fixed. Can you rebase on latest master and publish the PR if you think it's ready for a review? |
|
I just need to get sign-off from my end (internal company policy), and I'll rebase this once that happens. Hopefully tomorrow. |
| defer func() { | ||
| if errPanic := recover(); errPanic != nil { | ||
| valid = false | ||
| err = fmt.Errorf("invalid path: %s", patch.Path) |
There was a problem hiding this comment.
Can you please create a new error code for this use case and
Add a Distinct Err Code in proto/skaffold.proto e.g. INIT_INVALID_PROFILE_PATCH,
Here "INIT" stands for the phase initialization.
run ./hack/generate_proto.sh
You can use the new sError.NewError to create errors with the distinct error code you just defined e.g
return sErrors.NewError(err,
proto.ActionableErr{
Message: fmt.Sprintf("invalid path %s", patch.Path),
ErrCode: proto.StatusCode_ INIT_INVALID_PROFILE_PATCH,
Suggestions: []*proto.Suggestion{
{
SuggestionCode: proto.SuggestionCode_FIX_PATCH_FILE,
Action: "Please fix a valid patch file and try again",
},
},
})
There was a problem hiding this comment.
To be clear, this is not a new error, this was the pre-existing error. If I'm going to change it, it'll become "yamlpatch panic'd", or I'll just wrap up errPanic, since I have no test case for this.
There was a problem hiding this comment.
ACK. we are trying to collect distinct errors that happen during skaffold run and adding an error code will help us
Were you able to get a sign off ? |
|
I haven't had a chance to come back and make the requested changes yet. Hopefully later this week, otherwise it'll be next week. |
|
@TBBle any updates here? would love to get this merged |
|
Oops, this slipped past me last week. I should be able to get it done this week. |
|
hey @TBBle , just want to check in on this again :) |
|
Ooops, sorry. I must have marked this 'read' by accident. I do still plan to come back and recreate this PR. |
|
Closing this for now. Please reopen when you're able to @TBBle |
Fixes: #5259
Description
The faulty JSONPatch tests now ensure that the resulting error message actually mentions the faulty value.
User facing changes (remove if N/A)
Per #5259, any failure from yamlpatch simply logs
even if the path itself was valid.
Now, for the example given there, we get
Other example errors I got while messing with that file to introduce different errors:
The existing unit-test outputs
Note that I don't have an example of the "known to panic" case mentioned in the
tryPatchdocumentation, so that still returns the same error text as previously.