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.

Sign up for GitHub

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

Jump to bottom

fix(core): a number of resources are not taggable with Tags.of() #28989

Merged
merged 7 commits into from
Feb 23, 2024
Merged

fix(core): a number of resources are not taggable with Tags.of() #28989

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dbe614c
fix(spec2cdk): enable modern tagging style with ITaggableV2
GavinZZ Feb 5, 2024
c134260
Add a comment
GavinZZ Feb 5, 2024
5ff4515
Update code generation
GavinZZ Feb 10, 2024
e805aea
Remove unused boolean HAS_25610
GavinZZ Feb 22, 2024
1c374cd
Update render tags
GavinZZ Feb 22, 2024
07529e6
Give property name in render tags
GavinZZ Feb 23, 2024
5cce6f5
Merge branch 'main' into yuanhaoz/tagv2
mergify[bot] Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions tools/@aws-cdk/spec2cdk/lib/cdk/resource-decider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@ import { TypeConverter } from './type-converter';
import { attributePropertyName, cloudFormationDocLink, propertyNameFromCloudFormation } from '../naming';
import { splitDocumentation } from '../util';

// Depends on https://github.com/aws/aws-cdk/pull/25610
export const HAS_25610 = false;

// This convenience typewriter builder is used all over the place
const $this = $E(expr.this_());

Expand All @@ -21,7 +18,7 @@ export class ResourceDecider {
const taggability = resourceTaggabilityStyle(resource);
return taggability?.style === 'legacy'
? [CDK_CORE.ITaggable]
: taggability?.style === 'modern' && HAS_25610
: taggability?.style === 'modern'
? [CDK_CORE.ITaggableV2]
: [];
}
Expand Down Expand Up @@ -51,10 +48,8 @@ export class ResourceDecider {
this.handleTagPropertyLegacy(name, prop, this.taggability.variant);
continue;
case 'modern':
if (HAS_25610) {
this.handleTagPropertyModern(name, prop, this.taggability.variant);
continue;
}
this.handleTagPropertyModern(name, prop, this.taggability.variant);
continue;
}
} else {
this.handleTypeHistoryTypes(prop);
Expand Down Expand Up @@ -158,15 +153,15 @@ export class ResourceDecider {
summary: 'Tag Manager which manages the tags for this resource',
},
},
initializer: (props: Expression) =>
initializer: (_: Expression) =>
new CDK_CORE.TagManager(
this.tagManagerVariant(variant),
expr.lit(this.resource.cloudFormationType),
HAS_25610 ? expr.UNDEFINED : $E(props)[originalName],
expr.UNDEFINED,
expr.object({ tagPropertyName: expr.lit(originalName) }),
),
cfnValueToRender: {
[originalName]: $this.tags.renderTags(...(HAS_25610 ? [$this[rawTagsPropName]] : [])),
[originalName]: $this.tags.renderTags($this[rawTagsPropName]),
},
},
{
Expand All @@ -184,7 +179,7 @@ export class ResourceDecider {

private handleTagPropertyModern(cfnName: string, prop: Property, variant: TagVariant) {
const originalName = propertyNameFromCloudFormation(cfnName);
const originalType = this.converter.makeTypeResolvable(this.converter.typeFromProperty(prop));
GavinZZ marked this conversation as resolved.
Show resolved Hide resolved
const originalType = this.converter.typeFromProperty(prop);

this.propsProperties.push({
propertySpec: {
Expand Down Expand Up @@ -213,15 +208,15 @@ export class ResourceDecider {
summary: 'Tag Manager which manages the tags for this resource',
},
},
initializer: (props: Expression) =>
initializer: (_: Expression) =>
new CDK_CORE.TagManager(
this.tagManagerVariant(variant),
expr.lit(this.resource.cloudFormationType),
HAS_25610 ? expr.UNDEFINED : $E(props)[originalName],
expr.UNDEFINED,
expr.object({ tagPropertyName: expr.lit(originalName) }),
),
cfnValueToRender: {
[originalName]: $this.tags.renderTags(...(HAS_25610 ? [$this[originalName]] : [])),
GavinZZ marked this conversation as resolved.
Show resolved Hide resolved
[originalName]: $this.cdkTagManager.renderTags($this[originalName]),
},
},
{
Expand Down
Loading