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.

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

fix(core): wrong priority for tag aspects #33460

Merged
merged 11 commits into from
Feb 25, 2025
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);
Comment on lines +229 to +232
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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