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

fix(stack): check stack tags for deploy-time values #31457

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Sep 16, 2024

Stack tags are not rendered to the template, but instead are passed via API call.

Verify that stack tags do not contain unresolved values, as they won't work.

Closes #28017.


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

Stack tags are not rendered to the template, but instead are passed via
API call.

Verify that stack tags do not contain unresolved values, as they won't
work.

Closes #28017.
@rix0rrr rix0rrr requested a review from a team September 16, 2024 10:19
@aws-cdk-automation aws-cdk-automation requested a review from a team September 16, 2024 10:19
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Sep 16, 2024
@rix0rrr rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 16, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 16, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 25, 2024
iliapolo
iliapolo previously approved these changes Sep 25, 2024
stack.node.addMetadata(cxschema.ArtifactMetadataEntryType.STACK_TAGS, stack.tags.renderTags());
if (Object.entries(stackTags).length > 0) {
stack.node.addMetadata(
cxschema.ArtifactMetadataEntryType.STACK_TAGS,
Copy link
Contributor

Choose a reason for hiding this comment

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

non blocking: Seems weird to add first and validate later, I would flip this around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An exception is an exception, no? :)

@iliapolo iliapolo added the pr/do-not-merge This PR should not be merged at this time. label Sep 25, 2024
@iliapolo
Copy link
Contributor

Added pr/do-not-merge in case you want to address the comment

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 25, 2024
if (Object.entries(stackTags).length > 0) {
stack.node.addMetadata(
cxschema.ArtifactMetadataEntryType.STACK_TAGS,
Object.entries(stackTags).map(([key, value]) => ({ Key: key, Value: value })));
Copy link
Contributor

@iliapolo iliapolo Sep 25, 2024

Choose a reason for hiding this comment

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

We used to use renderTags and not tagValues here. I think those are the same if we disallow tokens, but wanted to pause on that for a second. Can you elaborate/explain why this is safe?

Its also weird we used to pass renderTags here but tagValues in the artifact itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that we used to use 2 different invocations is exactly the reason why I changed this here.

We know what formats are expected here, so we should do the rendering here, not defer it to some other class that might also be used in different contexts.

@iliapolo iliapolo dismissed their stale review September 25, 2024 12:17

Actually, have a question to clarify

@iliapolo iliapolo removed the pr/do-not-merge This PR should not be merged at this time. label Sep 25, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 25, 2024
Copy link
Contributor

mergify bot commented Sep 25, 2024

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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit abd1768 into main Sep 25, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Sep 25, 2024

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 deleted the huijbers/tags-error branch September 25, 2024 13:22
Copy link

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 Sep 25, 2024
@rix0rrr rix0rrr self-assigned this Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member 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.

Trigger: Token strings not resolving
3 participants