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.

Sign up for GitHub

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

Jump to bottom

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
@eladb
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
@rix0rrr
feat(core): add Tokenization.isResolvable for aspects
b7fbd1a 
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 rix0rrr mentioned this issue Jun 28, 2019
Merged
@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 pushed a commit that referenced this issue Jun 30, 2019
@rix0rrr
feat(core): add Tokenization.isResolvable for aspects (#3122)
d3a417e 
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.
@eladb eladb self-assigned this Aug 12, 2019
This was referenced Aug 22, 2019
Open
Open
Open
Open
Open
@eladb
Copy link
Contributor

eladb commented Sep 5, 2019

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

@eladb eladb closed this as completed Sep 5, 2019
This was referenced Dec 12, 2019
Closed
Closed
Closed
Merged
Closed
Open
Open
Open
Open
Open
Open
This was referenced Dec 13, 2019
Open
Open
This was referenced Jan 20, 2020
Closed
Closed
This was referenced Sep 24, 2024
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@NOUIY NOUIY mentioned this issue Oct 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eladb eladb

Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. needs-reproduction This issue needs reproduction. p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Snyk] Upgrade @aws-cdk/aws-iam from 0.22.0 to 0.39.0 MechanicalRock/tech-radar
[Snyk] Upgrade @aws-cdk/aws-codebuild from 0.24.1 to 0.39.0 MechanicalRock/cdk-constructs
[Snyk] Upgrade @aws-cdk/aws-codepipeline from 0.24.1 to 0.39.0 MechanicalRock/cdk-constructs
[Snyk] Upgrade @aws-cdk/aws-s3 from 0.24.1 to 0.39.0 MechanicalRock/cdk-constructs
[Snyk] Upgrade @aws-cdk/aws-cloudfront from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-codebuild from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-codecommit from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-codepipeline from 0.15.2 to 0.39.0 MechanicalRock/cdk-products
[Snyk] Upgrade @aws-cdk/aws-cloudformation from 0.0.0 to 0.39.0 NOUIY/aws-solutions-constructs
[Snyk] Upgrade @aws-cdk/aws-cloudfront from 0.0.0 to 0.39.0 NOUIY/aws-solutions-constructs
5 participants
@fulghum @rix0rrr @strax @eladb @NGL321