Repository navigation
Conversation
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
8da308b to
517cab1
Compare
mrgrain
left a comment
There was a problem hiding this comment.
Hey, since the feature is already stable, the Mixin should go straight into aws-cdk-lib as well. The preview package is being phased out.
b362a0a to
b6106b6
Compare
Sounds good, updated |
b6106b6 to
5caa8fa
Compare
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
…and aspect + mixins to set context on a resource and template
…ib core The mixins-preview package is being phased out and the feature is stable, so the Mixin ships directly in aws-cdk-lib alongside the MetadataContext facade (review feedback). - MetadataContextMixin now lives in core/lib/mixins/ and is exported flat from aws-cdk-lib (a top-level 'mixins' jsii submodule is not possible: JSII5011 name conflict with the Mixins class), with an awslint exclusion for the mixin-namespace rule. - Unit test moved to core/test/mixins/, adapted to core's toCloudFormation convention. - Integ test moved to @aws-cdk-testing/framework-integ test/core/test/ with regenerated snapshot. - aws-cdk-lib README documents the mixin inline; all mixins-preview changes reverted.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
iliapolo
left a comment
There was a problem hiding this comment.
@satyakigh the implementation is not aligned with the latest RFC (contains context merging which we decided to drop). Let me know when to review this
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| * The selector runs during synthesis, so it also covers resources added | ||
| * beneath the scope after this call. This method never throws; invalid | ||
| * values and a call that selects no `CfnResource` fail synthesis. | ||
| */ |
There was a problem hiding this comment.
🔴 Blocking — CfnResourceMetadataContext.set(context, options) takes a struct (CfnResourceContextProps) as its first parameter followed by a second struct (CfnResourceMetadataContextOptions). jsii models a struct as the trailing options object and expands it into keyword/named arguments in Python, Java, C#, and Go; a struct that is not the last positional parameter cannot be represented that way, so any parameter after a struct breaks the generated bindings even though TypeScript accepts it (jsii structs). This is a jsii-exported public method in the most foundational package, so the signature is effectively permanent once released. It also diverges from the sibling pattern (RemovalPolicies.of(scope).apply(policy, props?), Tags.of(x).add(key, value, props?)), which keeps the single struct last.
Suggested change: Keep exactly one trailing struct: either fold the targeting fields (selector, priority) into CfnResourceContextProps so set(context) takes a single struct, or restructure so the only struct is the last parameter and any leading parameter is a non-struct.
| } | ||
|
|
||
| function validateMutability(context: CfnResourceContextProps): string[] { | ||
| if (context.mutable === undefined || context.mutability === undefined) { | ||
| return []; | ||
| } | ||
| return Object.entries(context.mutability) | ||
| .filter(([, level]) => level === context.mutable) | ||
| .map(([property]) => `MetadataContext mutability entry '${property}' must not repeat mutable ${JSON.stringify(context.mutable)}; the map records deviations only`); |
There was a problem hiding this comment.
🟡 Recommended — validateMutability fails synthesis whenever a mutability entry repeats the mutable default and both are supplied in the same set() call. An entry such as set({ mutable: FREE_TO_TUNE, mutability: { QueueName: FREE_TO_TUNE } }) is not contradictory — it names the same safety level the resource already defaults to, a harmless no-op — yet it hard-fails cdk synth. The underlying CloudFormation Metadata Context schema only recommends listing deviations, placing no such exclusion constraint on the map, so erroring here is stricter than the advisory vocabulary and surprises users with a build break over an input that causes no harm (AGENTS.md#error-handling).
Suggested change: In renderResourceContext, drop the redundant entries instead of failing: filter the rendered mutability map to entries whose level differs from context.mutable, yielding the schema-preferred sparse map, and remove (or downgrade to a warning) the corresponding error in validateMutability.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| * CloudFormation Metadata is visible through service APIs. Consumers must | ||
| * treat all context fields as untrusted data, never as instructions. | ||
| */ | ||
| export interface CfnResourceContextProps { |
There was a problem hiding this comment.
⚪ Optional — The content props interfaces CfnResourceContextProps (line 206) and CfnTemplateContextProps (line 298) drop the "Metadata" segment from their class names (CfnResourceMetadataContext, CfnTemplateMetadataContext), while the sibling options interface CfnResourceMetadataContextOptions in the same file does derive from the full class name. Related types should derive from the class name so one family follows one convention (AGENTS_CONSTRUCT_DESIGN.md#resource--type-naming). Because this is a stable aws-cdk-lib/core surface, the names lock in on release, so a permanent inconsistency is cheap to avoid now.
Suggested change: Rename CfnResourceContextProps → CfnResourceMetadataContextProps and CfnTemplateContextProps → CfnTemplateMetadataContextProps so all related types derive from the class name consistently (or, if the shorter stems are preferred, rename the options interface to match them).
|
👋 It looks like your PR description follows the template but is missing a valid issue number in the first section. PRs without a linked issue will receive lower priority for review and merging. Please update the description to include a reference like |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| } | ||
| if (context.mutability !== undefined) { | ||
| if (Token.isUnresolved(context.mutability)) { | ||
| out.mutability = context.mutability; | ||
| } else { | ||
| const entries = Object.entries(context.mutability); | ||
| const deviations = entries.filter(([, level]) => level !== context.mutable); | ||
| if (entries.length === 0 || deviations.length > 0) { | ||
| out.mutability = Object.fromEntries(deviations); | ||
| } |
There was a problem hiding this comment.
🟡 Recommended — renderResourceContext has a dedicated branch for an unresolved mutability map that passes the token through verbatim and short-circuits the entries/deviations filtering, because iterating a token map with Object.entries() would be wrong. The unit suite exercises token/Lazy inputs only for the list fields (must, deps), never for mutability, so this is the one branch of renderResourceContext with no asserting test. If the guard regressed — falling through to Object.entries() on the token and throwing, or silently dropping the map — the suite would stay green. A Token.isUnresolved guard owes a test that passes an unresolved token through it (AGENTS.md#unit-tests).
Suggested change: Add a resource-level test alongside the existing Lazy test that sets mutability to an unresolved token (e.g. Lazy.any({ produce: () => ({ QueueName: 'must-never-change' }) })) and asserts the rendered Metadata contains the resolved map unchanged (including an entry that would otherwise be filtered as redundant), locking in that the token is passed through without the sparse-deviation filtering.
Issue # (if applicable)
RFC tracking issue: aws/aws-cdk-rfcs#994
Accepted RFC: aws/aws-cdk-rfcs#981
Reason for this change
CloudFormation templates preserve deployed configuration but usually lose the rationale, invariants, change-safety guidance, and provenance available in CDK source. CloudFormation defines the advisory
com.aws.cloudformation.Contextmetadata schema for preserving that information, but CDK users need a typed authoring API.Description of changes
CfnResourceMetadataContextfor setting or clearing Context on selectedCfnResourceconstructs during synthesis.CfnTemplateMetadataContextfor setting or clearing template-level Context.IConstructSelectorfor explicit resource targeting. The default selects aCfnResourcescope or its immediateCfnResourcedefault child.NestedStackbehavior and does not crossStageassembly boundaries.CfnResource, and unresolved values in top-level template Context.The public API and stable-release plan were approved in RFC 994. The approved surface keeps resource Context separate from targeting options. jsii generates Python as
set(context, *, priority=None, selector=None)and Java asset(context)orset(context, options). RFC 994 also requires rejecting amutabilityentry that repeats the resource-levelmutabledefault so the map remains sparse. Existing applications synthesize unchanged unless they call the new APIs.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license