Skip to content

Conversation

@reedham-aws
Copy link
Contributor

Issue # (if applicable)

N/a

Reason for this change

Current validation of fields for recent capacity provider additions in aws-lambda does not take into account unresolved tokens.

Description of changes

Added Token.isUnresolved() checks before all new validations. This ensures that validation only occurs when concrete values are provided, while still allowing tokens to pass through for CloudFormation resolution.

Describe any new or updated permissions being added

N/a

Description of how you validated changes

Changes should be only internal, validated that unit and integration tests continued to work.

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the p2 label Dec 2, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team December 2, 2025 21:32
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Dec 2, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

(This review is outdated)

@kumvprat
Copy link
Contributor

kumvprat commented Dec 3, 2025

@reedham-aws Can we have some unit tests that cover the edge of these properties being provided as tokens instead of exact values ?

new lambda.CapacityProvider(stack, 'MyCapacityProvider', {
capacityProviderName: tokenName,
maxVCpuCount: cdk.Token.asNumber(tokenMaxVCpu),
subnets: tokenSubnets.map((id, i) => ec2.Subnet.fromSubnetId(stack, `TokenSubnet${i}`, id)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have assertions on subnets and securityGroups values in the checks below ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added extra assertions for this in the tests

@reedham-aws
Copy link
Contributor Author

I'm not sure if the integration tests need to be updated here, it looks like other Lambda integ tests don't test token resolution.

Copy link
Contributor

@kumvprat kumvprat left a comment

Choose a reason for hiding this comment

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

LGTM

@kumvprat kumvprat self-assigned this Dec 18, 2025
@kumvprat kumvprat added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Dec 22, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 22, 2025 15:36

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mergify
Copy link
Contributor

mergify bot commented Dec 22, 2025

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit c5fbd97 into aws:main Dec 22, 2025
19 of 20 checks passed
@mergify
Copy link
Contributor

mergify bot commented Dec 22, 2025

Merge Queue Status

✅ The pull request has been merged at 684286e

This pull request spent 6 seconds in the queue, with no time running CI.
The checks were run in-place.

Required conditions to merge

@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2025
@reedham-aws reedham-aws deleted the capacity-provider-token-res branch December 22, 2025 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants