refactor: remove error details #5039
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
co-authored-by: tim <[email protected]>
stefreak
force-pushed
the
errors-remove-details
branch
from
5f222c4 to
77abdfc
Compare
stefreak
commented
Sep 7, 2023
Co-authored-by: Tim Beyer <[email protected]>
Co-authored-by: Tim Beyer <[email protected]>
Co-authored-by: Tim Beyer <[email protected]>
Co-authored-by: Tim Beyer <[email protected]>
Co-authored-by: Tim Beyer <[email protected]>
stefreak
changed the title
stefreak
force-pushed
the
errors-remove-details
branch
from
faad4bd to
d61c15d
Compare
|
There is one failing test in the CI. |
vvagaytsev
reviewed
Sep 11, 2023
- Throw GardenError of type "test-failed" or "task-failed" when the actual test or task command failed, e.g. with non-zero exit code. - Do not report "test-failed" or "task-failed" on errors like Pod eviction and on other Kubernetes API errors. Co-authored-by: Tim Beyer <[email protected]>
vvagaytsev
reviewed
Sep 11, 2023
vvagaytsev
reviewed
Sep 11, 2023
vvagaytsev
reviewed
Sep 11, 2023
vvagaytsev
reviewed
Sep 11, 2023
vvagaytsev
approved these changes
Sep 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Remove the
detailproperty and constructor argument fromGardenError.We did this because there were several issues with the
detailproperty:GardenError, thedetailproperty misled developers in some cases to conclude that the error message has enough information for users: The contents of thedetailproperty are not printed to the screen when an error occurs, and sometimes useful information has not been exposed through the message.detailproperty often used to include complex objects, like instances ofGardenor theSolver. The error details sometimes have been exposed to the user through the-o yamland-o jsonoutput flags, or serialized to JSON either accidentally or on purpose through the WebSocket interface or the Garden Cloud API. This led to issues ranging from bad user experience because of huge amounts of YAML, to OOM crashes when serializing huge amounts of JSON. This in turn led to very hard to maintain work-arounds likesanitizeValue. One particular big problem withsanitizeValueis that the return type is only known at runtime so we cannot define a correct TypeScript return type.detailproperty is of typeanyby default but there was still a lot of error handling code that relied on particular details to be defined. Due to this issue there were no type errors when changing the details in any way, and that could lead to really hard to understand issues.We addressed these issues with the following changes:
detailproperty and constructor argument from theGardenErrorABC.GardenError. In the error handling code we usedinstanceofto narrow the type of the error to that subclass, and only then tried accessing the error details.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: