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

Add a 'static_tags' option to disable automatic merging of tags #131

Closed
wants to merge 4 commits into from

Conversation

Makeshift
Copy link

what

  • Adds a new variable static_tags, set to false by default, which stops the module from merging in new tags and instead uses the tags variable in the input as-is, including handing it off to other modules as necessary

why

I have a fork of terraform-null-label that removes tags that interfere with provider default_tags, but this isn't obeyed by downstream cloudposse modules. This PR should make it possible to pass context in and force null_label to not make any tags, or only make a subset of the tags :)

references

@Makeshift
Copy link
Author

/test all

@Makeshift Makeshift marked this pull request as ready for review August 10, 2021 08:52
@Makeshift Makeshift requested a review from a team as a code owner August 10, 2021 08:52
@Nuru Nuru added the do not merge Do not merge this PR, doing so would cause problems label Aug 13, 2021
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

@Makeshift Thank you for this contribution.

I think I understand the problem, though maybe you want to explain it in further detail, because it does not look to me like this PR solves the problem.

As I understand it, the issue is that the current AWS provider does not like it when a resource's tags have the same keys as the provider's default_tags. However, in some cases, modules add tags that are critical to proper functioning of the module, such as tagging resources as managed by an autoscaler or belonging to a Kubernetes cluster. So we cannot allow an option that blocks modules from adding tags.

To me this suggest 2 reasonable solutions:

  1. Tell people not to use the default_tags option to the provider and instead implement default_tags in null-label and have people use null-label's tags output. This has the added advantage of properly allowing tags to be overridden, which the AWS provider seem not to support. Of course the disadvantage is that this does not cover any resources created by modules that do not use null-label. And in fact, we do not even need a new input for this. Simply setting the current tags input with the set of default tags in a base null-label module in the root module would do it.
  2. Add some input like suppress_tags which provides a list of tag keys to filter out of the tags output, so those tags are not set in resources. The trade-off here is that while this ensures all resources get the default tags via the provider, the default tags cannot be overridden.

@Makeshift What do you think of those solutions?

@Makeshift
Copy link
Author

Makeshift commented Aug 18, 2021

You're correct in your assessment - Thank you for pointing out the flaw in disabling tags, it didn't occur to me that they could be integral to the working of certain resources/services. Do you have an example of a module that does this so I can make sure that any changes I make doesn't break it?

  1. Tell people not to use the default_tags option to the provider and instead implement default_tags in null-label and have people use null-label's tags output. This has the added advantage of properly allowing tags to be overridden, which the AWS provider seem not to support. Of course the disadvantage is that this does not cover any resources created by modules that do not use null-label. And in fact, we do not even need a new input for this. Simply setting the current tags input with the set of default tags in a base null-label module in the root module would do it.

That disadvantage is rather a large one, unfortunately, I believe that default_tags is simply too convenient of an option to ignore, even if it's a little broken currently. Being able to generate tags for an entire stack without manually setting them is invaluable for costcentering.

  1. Add some input like suppress_tags which provides a list of tag keys to filter out of the tags output, so those tags are not set in resources. The trade-off here is that while this ensures all resources get the default tags via the provider, the default tags cannot be overridden.

This seems like a reasonable compromise. You're suggesting something along the lines of allowing the user to set suppress_tags = ["environment", "stage", "name"], then tags becomes:

tags = { for k, v in merge(local.generated_tags, local.input.tags): k => v if ! contains([for i in local.suppress_tags: lower(i)], lower(k)) }

?
@Nuru

Edit: I've implemented and committed what I think you were proposing, let me know :)

@Makeshift Makeshift requested a review from Nuru August 18, 2021 19:38
@Makeshift
Copy link
Author

/test all

@Nuru
Copy link
Contributor

Nuru commented Aug 19, 2021

@Makeshift I hope you do not mind, I am going to steal this idea and some of this code and re-implement it in my current PR, rather than ask you to make additional changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this PR, doing so would cause problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants