Skip to content

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

Closed
kaizencc wants to merge 1 commit intomainfrom
revert-31457-huijbers/tags-error
Closed

revert: check stack tags for deploy-time values"#32040
kaizencc wants to merge 1 commit intomainfrom
revert-31457-huijbers/tags-error

Conversation

@kaizencc
Copy link
Copy Markdown
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
Copy Markdown
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
@github-actions
Copy link
Copy Markdown
Contributor

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