Skip to content

Commit

Permalink
fix(core): wrong priority for tag aspects (#33460)
Browse files Browse the repository at this point in the history
### ❗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*
  • Loading branch information
hwwi authored Feb 25, 2025
1 parent f9252ab commit 0b9ffeb
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 6 deletions.
10 changes: 7 additions & 3 deletions packages/aws-cdk-lib/core/lib/tag-aspect.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Construct, IConstruct } from 'constructs';
import { Annotations } from './annotations';
import { IAspect, Aspects, AspectPriority } from './aspect';
import { IAspect, Aspects, AspectPriority, AspectOptions } from './aspect';
import { ITaggable, ITaggableV2, TagManager } from './tag-manager';

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

/**
* remove tags to the node of a construct and all its the taggable children
*/
public remove(key: string, props: TagProps = {}) {
Aspects.of(this.scope).add(new RemoveTag(key, props), { priority: AspectPriority.MUTATING });
const removeTag = new RemoveTag(key, props);
const options: AspectOptions = { priority: AspectPriority.MUTATING };
Aspects.of(this.scope).add(removeTag, options);
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/core/test/aspect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,10 @@ describe('aspect', () => {
Aspects.of(stack).add(new AddLoggingBucketAspect(), { priority: 0 });
Tags.of(stack).add('TestKey', 'TestValue');

// THEN - check that Tags Aspect is applied to stack with default priority
// 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);

// THEN - both Aspects are successfully applied, new logging bucket is added with versioning enabled
Template.fromStack(stack).hasResourceProperties('AWS::S3::Bucket', {
Expand Down
32 changes: 31 additions & 1 deletion packages/aws-cdk-lib/core/test/tag-aspect.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,19 @@
import { Construct } from 'constructs';
import { toCloudFormation } from './util';
import { CfnResource, CfnResourceProps, RemoveTag, Stack, Tag, TagManager, TagType, Aspects, Tags, ITaggable, ITaggableV2 } from '../lib';
import {
CfnResource,
CfnResourceProps,
RemoveTag,
Stack,
Tag,
TagManager,
TagType,
Aspects,
Tags,
ITaggable,
ITaggableV2,
AspectPriority,
} from '../lib';
import { synthesize } from '../lib/private/synthesis';

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

test('Tags applied without priority get mutating priority value', () => {
const root = new Stack();
const res = new TaggableResource(root, 'FakeResource', {
type: 'AWS::Fake::Thing',
});

Tags.of(root).add('root', 'was here');
Tags.of(res).add('first', 'there is only 1');
Tags.of(res).remove('root');

const rootAspectApplications = Aspects.of(root).applied;
expect(rootAspectApplications[0].priority).toEqual(AspectPriority.MUTATING);
const resAspectApplications = Aspects.of(res).applied;
expect(resAspectApplications[0].priority).toEqual(AspectPriority.MUTATING);
expect(resAspectApplications[1].priority).toEqual(AspectPriority.MUTATING);
});

test('add will add a tag and remove will remove a tag if it exists', () => {
const root = new Stack();
const res = new TaggableResource(root, 'FakeResource', {
Expand Down

0 comments on commit 0b9ffeb

Please sign in to comment.