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

feat: Optional appName for tag #1497

Merged
merged 3 commits into from
Nov 14, 2022
Merged

feat: Optional appName for tag #1497

merged 3 commits into from
Nov 14, 2022

Conversation

tomrf1
Copy link
Member

@tomrf1 tomrf1 commented Sep 19, 2022

It's useful to have all resources in a stack tagged with App, specifically for monitoring costs by app

@tomrf1 tomrf1 changed the title Optional appName for tag feat: Optional appName for tag Sep 19, 2022
@tomrf1 tomrf1 marked this pull request as ready for review September 19, 2022 11:19
@tomrf1 tomrf1 requested a review from a team as a code owner September 19, 2022 11:19
Copy link
Member

@akash1810 akash1810 left a comment

Choose a reason for hiding this comment

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

A really sensible change!

I wonder if we should rename the property to app, and consequently rename app in the constructor to scope? scope is a fairly typical name within the AWS CDK for this type of argument.

IIUC the patterns will add the app tag to resources too, so I'd be curious to see what would happen if multiple GuEc2App patterns are added to the same GuStack and each set the app tag. Maybe that's worth a test and/or some validation logic?

@nicl
Copy link
Contributor

nicl commented Oct 21, 2022

The docstring for GuStack already suggests the app tag should be set based on the constructor app arg:

* GuStack will add the Stack, Stage and App tags to all resources.

But, as this PR suggests, for some reason this isn't actually done.

cc @akash1810 @tomrf1 I wonder if we should just require app as part of GuStackProps and then stop adding the app tag via our patterns? It's not clear to me that patterns should be adding app tags to things themselves - feels like the stack should have this responsibilty.

@akash1810
Copy link
Member

The docstring for GuStack already suggests the app tag should be set based on the constructor app arg

I think the docs are out of date; we correct that description!

cc @akash1810 @tomrf1 I wonder if we should just require app as part of GuStackProps and then stop adding the app tag via our patterns? It's not clear to me that patterns should be adding app tags to things themselves - feels like the stack should have this responsibilty.

I don't think we can do this. Tags at the GuStack level are applied to all resources. If the stack is simple, this is fine. However where we have multiple apps, for example two lambdas, it'll be incorrect to apply App at GuStack as each lambda will get the same value.

@tomrf1
Copy link
Member Author

tomrf1 commented Nov 14, 2022

@akash1810 @nicl I've pushed up the rename from appName to app

Copy link
Contributor

@nicl nicl left a comment

Choose a reason for hiding this comment

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

Nice :shipit:

Copy link
Member

@akash1810 akash1810 left a comment

Choose a reason for hiding this comment

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

🏷️

@tomrf1 tomrf1 merged commit db51447 into main Nov 14, 2022
@tomrf1 tomrf1 deleted the tf-appName branch November 14, 2022 10:05
@github-actions
Copy link
Contributor

🎉 This PR is included in version 48.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@akash1810
Copy link
Member

I don't think this is working, as we're never assigning this._app from the props 😓 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants