Use InputPropertyError for validations in the node group components #1592
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
| `an instanceProfile or instanceProfileName is required`, | ||
| parent, | ||
| ); | ||
| throw new pulumi.InputPropertyError({ |
There was a problem hiding this comment.
In these cases where the error could be attributed to multiple properties should we add two property errors or should it stay as a resource error?
There was a problem hiding this comment.
I feel like a single input property error is enough here.
The benefit of the input property error is that a user will be presented with all errors and not only the first validation that failed.
For example compare those two errors for the same program:
Old:
New:
IMO we shouldn't create multiple errors for the same issue when there's multiple properties involved. That's just gonna increase the cognitive complexity of understanding the issue.
There was a problem hiding this comment.
ah I see. I wasn't sure how the property errors would be shown, but since they are just listed out under the resource then that makes total sense.
It looks like that value {{urn:pulumi:..} is something that the engine adds? I'm assuming it only works with primitive types or something?
There was a problem hiding this comment.
That's how dependency resources are visualized here. I guess the URN is the only sensible value that can be shown here 🤔
There was a problem hiding this comment.
Just curious, does it resolve Output values correctly? Wondering if we ever need to be careful about what kind of values we can include here.
There was a problem hiding this comment.
You mean the message the engine adds, right? No, this one shows either that it's an output (e.g. during preview) or the resolved value if it's already resolved.
| ); | ||
| const validationErrors: pulumi.InputPropertyErrorDetails[] = []; | ||
|
|
||
| const instanceProfileName = core.apply((c) => resolveInstanceProfileName(args, c)); |
There was a problem hiding this comment.
What happens when you throw inside of an apply? Do we still get the rest of the validation errors unless they depend on instanceProfileName? And if we eventually do throw because of other errors do these get surfaced then?
| `invalid args for node group ${name}, nodeSecurityGroup and nodeSecurityGroupId are mutually exclusive`, | ||
| parent, | ||
| ); | ||
| validationErrors.push({ |
There was a problem hiding this comment.
If we no longer throw here and keep going, can that lead to us being in a bad place? When we use validation errors like this, do we need to make sure we throw validation errors before any resources are created?
In this case I think we do throw before creating resources, but I'm just curious for future reference.
| `an instanceProfile or instanceProfileName is required`, | ||
| parent, | ||
| ); | ||
| throw new pulumi.InputPropertyError({ |
There was a problem hiding this comment.
Just curious, does it resolve Output values correctly? Wondering if we ever need to be careful about what kind of values we can include here.
flostadler
force-pushed
the
flostadler/proper-errors
branch
from
8aea025 to
929ccdc
Compare
|
This PR has been shipped in release v3.8.0. |
pu/pu introduced new error types for component authors to use. The
InputPropertyErrorandInputPropertiesErrorerrors are pretty printed by the engine and don't confuse users with a load of GRPC stack traces (see docs). Additionally those errors accumulate multiple validation errors and allow presenting them all to users. Previously we exited on the first error.For adding EFA support (#1564) we'd like to make use of these new errors. In preparation for that I've updated the node group components to make use of this new error type.
UX:
Old (missing the second error):
New: