fix(core): wrong priority for tag aspects #33460
Conversation
github-actions
bot
added
beginning-contributor
kaizencc
changed the title
kaizencc
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
Hi @kaizencc, sorry, i missed that. I added a unit test. |
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
aws-cdk-automation
added
the
pr/needs-community-review
kaizencc
changed the title
There was a problem hiding this comment.
Just my thoughts because @kaizencc sent me the PR and I read it. Figured I'd throw in my $0.02. Please feel free to ignore.
| // THEN - check that Tags Aspect is applied to stack with mutating priority | ||
| let aspectApplications = Aspects.of(stack).applied; | ||
| expect(aspectApplications.length).toEqual(2); | ||
| expect(aspectApplications[1].priority).toEqual(AspectPriority.DEFAULT); | ||
| expect(aspectApplications[1].priority).toEqual(AspectPriority.MUTATING); |
There was a problem hiding this comment.
Interesting. It looks like the test was written to the code instead of the code to the test. Otherwise the test would have called out the priority as mutating, and the code with the typeo would have failed.
There was a problem hiding this comment.
I get the general your suggestion, but I’d like to understand it more clearly. Could you share a code snippet to illustrate your point?
There was a problem hiding this comment.
I think in this case it was an error on the original authors part to simply get the tests to pass, and so its ok to change it here.
@dsilbergleithcu-godaddy do you have any suggestions for how to make sure this kind of human error doesn't happen in the future? otherwise i'm just kind of chalking it up to an oversight.
There was a problem hiding this comment.
Thanks @hwwi. I'm approving and this PR should be included in CDK v2.181.0. We've decided against adding a notice since it's unlikely that many users are affected and such a notice will be more confusing than helpful. Instead I've documented the PR description to alert anyone who stumbles upon this PR that there is a small chance that changing your CDK code is necessary, if you are depending on the previously wrong order of aspect execution.
aws-cdk-automation
removed
the
pr/needs-community-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
❗Important❗
This change is to fix behavior that was always wrong, starting in this commit released in CDK v2.172.0. In doing so, the order of your aspect execution may change. If you are inadvertently depending on an aspect ordering that was previously wrong (tagging was previously not prioritized as a mutating aspect), you could need to change your CDK code. We are not treating this as a breaking change because the previous order was always wrong.
Reason for this change
Priority was not applied in #32333
Description of changes
Fix missing priority
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license