Skip to content

Commit 0b9ffeb

Browse files
authored
fix(core): wrong priority for tag aspects (#33460)
### ❗Important❗ This change is to fix behavior that was always wrong, starting in this [commit](#32097) 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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent f9252ab commit 0b9ffeb

File tree

3 files changed

+40
-6
lines changed

3 files changed

+40
-6
lines changed

packages/aws-cdk-lib/core/lib/tag-aspect.ts

+7-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Construct, IConstruct } from 'constructs';
22
import { Annotations } from './annotations';
3-
import { IAspect, Aspects, AspectPriority } from './aspect';
3+
import { IAspect, Aspects, AspectPriority, AspectOptions } from './aspect';
44
import { ITaggable, ITaggableV2, TagManager } from './tag-manager';
55

66
/**
@@ -159,14 +159,18 @@ export class Tags {
159159
* add tags to the node of a construct and all its the taggable children
160160
*/
161161
public add(key: string, value: string, props: TagProps = {}) {
162-
Aspects.of(this.scope).add(new Tag(key, value, props)), { priority: AspectPriority.MUTATING };
162+
const tag = new Tag(key, value, props);
163+
const options: AspectOptions = { priority: AspectPriority.MUTATING };
164+
Aspects.of(this.scope).add(tag, options);
163165
}
164166

165167
/**
166168
* remove tags to the node of a construct and all its the taggable children
167169
*/
168170
public remove(key: string, props: TagProps = {}) {
169-
Aspects.of(this.scope).add(new RemoveTag(key, props), { priority: AspectPriority.MUTATING });
171+
const removeTag = new RemoveTag(key, props);
172+
const options: AspectOptions = { priority: AspectPriority.MUTATING };
173+
Aspects.of(this.scope).add(removeTag, options);
170174
}
171175
}
172176

packages/aws-cdk-lib/core/test/aspect.test.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -226,10 +226,10 @@ describe('aspect', () => {
226226
Aspects.of(stack).add(new AddLoggingBucketAspect(), { priority: 0 });
227227
Tags.of(stack).add('TestKey', 'TestValue');
228228

229-
// THEN - check that Tags Aspect is applied to stack with default priority
229+
// THEN - check that Tags Aspect is applied to stack with mutating priority
230230
let aspectApplications = Aspects.of(stack).applied;
231231
expect(aspectApplications.length).toEqual(2);
232-
expect(aspectApplications[1].priority).toEqual(AspectPriority.DEFAULT);
232+
expect(aspectApplications[1].priority).toEqual(AspectPriority.MUTATING);
233233

234234
// THEN - both Aspects are successfully applied, new logging bucket is added with versioning enabled
235235
Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', {

packages/aws-cdk-lib/core/test/tag-aspect.test.ts

+31-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,19 @@
11
import { Construct } from 'constructs';
22
import { toCloudFormation } from './util';
3-
import { CfnResource, CfnResourceProps, RemoveTag, Stack, Tag, TagManager, TagType, Aspects, Tags, ITaggable, ITaggableV2 } from '../lib';
3+
import {
4+
CfnResource,
5+
CfnResourceProps,
6+
RemoveTag,
7+
Stack,
8+
Tag,
9+
TagManager,
10+
TagType,
11+
Aspects,
12+
Tags,
13+
ITaggable,
14+
ITaggableV2,
15+
AspectPriority,
16+
} from '../lib';
417
import { synthesize } from '../lib/private/synthesis';
518

619
class TaggableResource extends CfnResource implements ITaggable {
@@ -131,6 +144,23 @@ describe('tag aspect', () => {
131144
expect(res2.tags.renderTags()).toEqual([{ key: 'first', value: 'there is only 1' }]);
132145
});
133146

147+
test('Tags applied without priority get mutating priority value', () => {
148+
const root = new Stack();
149+
const res = new TaggableResource(root, 'FakeResource', {
150+
type: 'AWS::Fake::Thing',
151+
});
152+
153+
Tags.of(root).add('root', 'was here');
154+
Tags.of(res).add('first', 'there is only 1');
155+
Tags.of(res).remove('root');
156+
157+
const rootAspectApplications = Aspects.of(root).applied;
158+
expect(rootAspectApplications[0].priority).toEqual(AspectPriority.MUTATING);
159+
const resAspectApplications = Aspects.of(res).applied;
160+
expect(resAspectApplications[0].priority).toEqual(AspectPriority.MUTATING);
161+
expect(resAspectApplications[1].priority).toEqual(AspectPriority.MUTATING);
162+
});
163+
134164
test('add will add a tag and remove will remove a tag if it exists', () => {
135165
const root = new Stack();
136166
const res = new TaggableResource(root, 'FakeResource', {

0 commit comments

Comments
 (0)