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

revert: check stack tags for deploy-time values" #32040

Closed
wants to merge 1 commit into from

Conversation

kaizencc
Copy link
Contributor

@kaizencc kaizencc commented Nov 6, 2024

Reverts #31457

@aws-cdk-automation aws-cdk-automation requested a review from a team November 6, 2024 18:35
@github-actions github-actions bot added the p2 label Nov 6, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Nov 6, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: a3b6d58
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 6, 2024
@kaizencc kaizencc closed this Nov 6, 2024
Copy link

github-actions bot commented Nov 6, 2024

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 Nov 6, 2024
mergify bot pushed a commit that referenced this pull request Nov 6, 2024
In #31457, we introduced a change that made synthesis fail if one of the stack tags was a deploy-time value. Since stack tags are assigned outside a CloudFormation context, deploy-time values cannot be evaluated, so the stack ends up with a tag like `{ Key: "my-tag", Value: "${Token[1234]}" }`, which is probably not what is intended.

Worse, those tags are automatically propagated to all resources in the stack by CloudFormation, and some may validate the tag value and find that `$` or any of the other characters are not valid tag values.

The intent was that customers would be alerted to these kinds of mistakes and apply their tags to resources, or skip stacks when applying tags to large scopes:

```ts
Tags.of(this).add('my-tag', Fn.importValue('SomeExport'), {
  excludeResourceTypes: ['aws:cdk:stack'],
});
```

The previous change was a bit drastic in its attempts.  In this one we ignore the unresolved tags and add a warning instead. That way, synthesis still succeeds.

Closes #32040.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Leo10Gama pushed a commit to Leo10Gama/aws-cdk that referenced this pull request Nov 13, 2024
In aws#31457, we introduced a change that made synthesis fail if one of the stack tags was a deploy-time value. Since stack tags are assigned outside a CloudFormation context, deploy-time values cannot be evaluated, so the stack ends up with a tag like `{ Key: "my-tag", Value: "${Token[1234]}" }`, which is probably not what is intended.

Worse, those tags are automatically propagated to all resources in the stack by CloudFormation, and some may validate the tag value and find that `$` or any of the other characters are not valid tag values.

The intent was that customers would be alerted to these kinds of mistakes and apply their tags to resources, or skip stacks when applying tags to large scopes:

```ts
Tags.of(this).add('my-tag', Fn.importValue('SomeExport'), {
  excludeResourceTypes: ['aws:cdk:stack'],
});
```

The previous change was a bit drastic in its attempts.  In this one we ignore the unresolved tags and add a warning instead. That way, synthesis still succeeds.

Closes aws#32040.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contribution/core This is a PR that came from AWS. p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants