fix(core): turn validation errors into metadata errors #2988
Conversation
Validation errors are currently turned into exceptions. This causes issues where missing context values lead to validation errors (such as in the case of `DnsValidatedCertificate`): the error would have been solved by retrieving the context value, but because an exception is thrown the CLI stops immediately and doesn't re-execute. I feel it also makes sense to treat these errors as construct errors, as opposed to exceptions which I feel should be more "you're misusing the API" kind of errors, rather than "something is wrong somewhere" kind of errors. The change I made is the smallest change to achieve the desired effect. If we agree that we want this change, it would probably be better to do it differently, just have `.validate()` implementors call `this.node.addError()` immediately. Fixes #2076.
rix0rrr
force-pushed
the
huijbers/fix-hosted-zone-import-error
branch
from
1d9ffa1 to
41114db
Compare
rix0rrr
changed the title
|
I’d expect this to fail many unit tests that rely on validate throwing. Also, I like the guarantee we provide today that synthesize won’t be called if validation failed because then constructs May have invalid data when they synthesize (E.g. pipeline can assume there will at least be a single stage). |
That's a good point. We can still provide that guarantee though. |
Yep, got some rendering that errors out. But how is it that |
|
Hmm. I still feel metadata errors are the "correct" solution, but this is indeed a harder change than it seems at first sight. |
|
When a test used skipValidation it explicit acknowledges that it’s aware of the resulting behavior |
|
Perhaps short term we can just special case the dummy value so that validation will not fail |
|
Let's punt this for now |
Validation errors are currently turned into exceptions. This causes
issues where missing context values lead to validation errors (such as
in the case of
DnsValidatedCertificate): the error would have beensolved by retrieving the context value, but because an exception is
thrown the CLI stops immediately and doesn't re-execute.
I feel it also makes sense to treat these errors as construct errors,
as opposed to exceptions which I feel should be more "you're misusing
the API" kind of errors, rather than "something is wrong somewhere"
kind of errors.
The change I made is the smallest change to achieve the desired effect.
If we agree that we want this change, it would probably be better to
do it differently, just have
.validate()implementors callthis.node.addError()immediately.Fixes #2076.
Pull Request Checklist
designfolderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.