-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: customer aspect cannot add Tags if a BucketNotifications construct is present #33979
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
04adfca
to
4ee1dbf
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33979 +/- ##
==========================================
+ Coverage 82.39% 82.43% +0.03%
==========================================
Files 120 120
Lines 6960 6975 +15
Branches 1175 1178 +3
==========================================
+ Hits 5735 5750 +15
Misses 1120 1120
Partials 105 105
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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. |
The title refers to a specific case in which we discovered this, but the problem was more general.
Now that Aspects have a priority, we want to execute based on priority as the primary sort key. To make sure we don't make mistakes, the new AspectsV2 invocation (enabled with flag
@aws-cdk/core.aspectStabilization: true
) throws an error if it detects a violation of the strict priority execution order.Because of the moment aspects are looked up and sorted by priority, this currently goes wrong if:
The reason is that we used to look up the aspects before iterating, and iterate over them. At that point we have the list
[100, 700]
, and we invoke both. Then, when we go around again, the new list is[100, 500, 700]
with the500
Aspect having not been invoked yet -- but also the Aspect with priority700
already having been invoked. If we invoke500
at that point, we would have inverted the priority order, so we fail instead.That is undesirable: what we should have done is noticed that
500
got inserted between100
and700
and do that next.Currently achieve that with the following:
The system of inserting and recalculating could probably all be made more efficient, but aspects adding aspects should be relatively rare so until we have evidence that it is too expensive, this will do for now.
In the specific case reported for this, a customer Aspect with priority 10 adds a Tag Aspect (priority 200) while the
BucketNotification
construct also adds an Aspect on itself with priority 500. The mere presence of thisBucketNotification
construct with its self-added Aspect prevents any Aspect from adding tags.Closes #33943.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license