Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IConstruct.node.addError no-op when called from an aspect #3026

Closed
1 of 5 tasks
strax opened this issue Jun 24, 2019 · 3 comments Β· Fixed by MechanicalRock/tech-radar#14 Β· 4 remaining pull requests
Closed
1 of 5 tasks

IConstruct.node.addError no-op when called from an aspect #3026

strax opened this issue Jun 24, 2019 · 3 comments Β· Fixed by MechanicalRock/tech-radar#14 Β· 4 remaining pull requests
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. needs-reproduction This issue needs reproduction. p1

Comments

@strax
Copy link
Contributor

strax commented Jun 24, 2019

  • I'm submitting a ...

    • πŸͺ² bug report
    • πŸš€ feature request
    • πŸ“š construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    We have an aspect that we use to validate certain properties of constructs. Ideally we'd use construct.node.addError(...) from within the aspect to not bail at the first error. However, calling addError from within the aspect seems to do nothing. As a workaround, we fall back to throwing an error if the validation fails.

  • What is the expected behavior (or behavior of feature suggested)?
    IConstructNode.addError reports an error when it is called from within IConstruct.apply.

  • Please tell us about your environment:

    • CDK CLI Version: 0.35.0
    • Module Version: 0.35.0
    • Language: TypeScript
@NGL321 NGL321 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. @aws-cdk/core Related to core CDK functionality needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 25, 2019
@fulghum fulghum added the p1 label Jun 26, 2019
rix0rrr added a commit that referenced this issue Jun 28, 2019
Add an example of a validating aspect, add `Tokenization.isResolvable()`
to make it easier/possible to rule out `IResolvable`s from L1
properties.

Included unit tests  validates that the bug reported in #3026 is not systemic.
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 28, 2019

Cannot reproduce this, wrote a unit test in linked PR in which this works as intended.

@strax
Copy link
Contributor Author

strax commented Jun 28, 2019

@rix0rrr I set up a minimal repro at https://github.com/strax/cdk-3122-repro. Note that it is against 0.36, I haven't checked if master has this issue too. I can revisit when the next version is released.

@eladb
Copy link
Contributor

eladb commented Sep 5, 2019

Closing for now. Please reopen if this is still relevant in 1.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment