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: adds support for optional root_module tag #144

Conversation

gberenice
Copy link

what

  • This adds an optional tag root_module that will be output along with other context tags if the corresponding bool variable is set to true. A similar feature is introduced in introspection.mixin.tf.
  • Example of the changes:
          ~ tags            = {
              "Name"        = "internal-dev-network-vpc"
              "Namespace"   = "internal"
              "Stage"       = "dev"
            + "Root_module" = "network"
          }
    

why

  • In some cases there is a need to quickly identify what Terraform root module manages the resource. The issue becomes more outstanding with the infrastructure growth and/or reorganization of the code structure. Would be great to have it resolved with context.tf and avoid updating additional tags for every child module or resource.

references

  • N/A

@gberenice gberenice requested review from a team as code owners July 5, 2023 10:44
Copy link
Member

@joe-niland joe-niland left a comment

Choose a reason for hiding this comment

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

Hi @gberenice a couple of comments for you!

}

# Remove null values from tags_context. Required due to `root_module` that can be null.
tags_context_clean = { for k, v in local.tags_context : k => v if v != null }
Copy link
Member

Choose a reason for hiding this comment

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

Hi @gberenice just a thought - if you set root_module to "" instead of null (when it's not enabled), you don't need this line. Perhaps this would be more in line with the treatment of the other variables, as done when setting normalized_labels?

@@ -96,6 +96,8 @@ locals {
stage = local.formatted_labels["stage"]
name = local.formatted_labels["name"]

root_module = var.root_module_tag_enabled ? basename(path.cwd) : null
Copy link
Member

Choose a reason for hiding this comment

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

I found that using this with terragrunt doesn't have the intended output. It will output the temporary directory name from within the .terragrunt-cache/ dir. This might be relevant because this module is used in lots of scenarios, not just within the atmos workflow.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @joe-niland , this should not be used in the module b/c it might not work in all scenarios

Copy link
Member

Choose a reason for hiding this comment

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

@joe-niland didn't know you were using Terragrunt! Good to know -- may need to ping you at some point in the coming weeks 😁

@joe-niland joe-niland requested a review from Nuru July 5, 2023 21:21
@Nuru Nuru added the do not merge Do not merge this PR, doing so would cause problems label Jul 5, 2023
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.

@gberenice Thank you for this PR.

As you may know, null-label is central to the Cloud Posse architecture, which unfortunately makes even the tiniest change a big deal. I will leave this PR open for a while for discussion, but I expect to reject it as "won't do". As you pointed out, you can achieve the same effect with the introspection.mixin.tf or by deploying your own, modified context.tf in your own root modules. That path allows you to control your environment without affecting other users of the Cloud Posse Terraform modules.

One concrete problem with your proposal is that basename will not necessarily give you the name of the root module. For example, most of terraform-aws-components's eks modules are under eks/, so we would want the root module name to be something like eks/cluster or eks/alb-controller.

I suggest you work things out with a solution that does not involve a change to null-label and once that solution is proven, you can propose the feature as an enhancement to null-label and we can consider it for the v2 release.

@gberenice
Copy link
Author

@joe-niland @aknysh @Nuru thanks a lot for your reviews, these are valid points. I need to research the case with Terragrunt.
I wonder if an option with a relative path to the repo's root would cover all the other scenarios, what do you think? For example, ./components/eks/alb-controller. Or it could even include the repo name as well.
Also, there might be a variable for the root module name, so a user is able to explicitly set the desired value.

@Gowiem
Copy link
Member

Gowiem commented Jul 6, 2023

@Nuru I get your point on not wanting to merge this as it stands, but if we can I'd like to figure out what we'd want to see here explicitly to move this forward. We see this as being worth a future revision of null-label, and with the many new label based modules that are coming out I personally think this could be a good differentiator. Keeping this information inside of private forks of context.tf will keep it from benefitting the wider community and this is a common tag that both Masterpoint + Cloud Posse have implemented via mixins like introspection.

Couple explicit items I could see as additional functionality here that would serve all cases:

  1. Having the tag's key name ("RootModule" right now) be overridable via a variable, so we could rename to "Component" for atmos projects or other organization specific terminology.
  2. Having the tag's value be overridable via a variable so consumers of this module who are using frameworks that do magic in the background (like Terragrunt) can provide their own value.
  3. Adding a "path traversal count" variable (probably a better name here) that will walk back up the calling module's basename and include the given amount of paths into the value.
    1. Example 1: root_module_path_traversal_count = 1 by default and the value is just the name of the calling root module, so would result in something like aurora-postgres.
    2. Example 2: root_module_path_traversal_count = 2 could result in the desired functionality of grabbing the proper eks/cluster or eks/alb-controller.
    3. Example 3: root_module_path_traversal_count = 4 could result in the full, repo oriented path to the root module such as my-infra-project/components/terraform/aurora-postgres. This could allow tag delineation for projects in a non-monorepo, shared root module organization.

Keeping the default of this functionality as disabled is obviously critical as this likely doesn't make sense in all terraform usecases.

Thoughts on the above? We would be happy to dogfood these ideas internally at Masterpoint and resurface here via this PR at a later date if those sound like potentially useful and mergable enhancements to this functionality.

@Nuru
Copy link
Contributor

Nuru commented Jul 14, 2023

@Gowiem Thank you for your proposal, into which you have obviously put a lot of thought.

I feel you may have overlooked the fact that context.tf gets installed in every one of our Terraform Modules. It seems to me everything you are talking about only applies to root modules (what we have taken to calling "components"), and even then has significant idiosyncrasies. And in the end, all you are talking about is automatically setting a couple of tags. You and @gberenice have other options.

For one, you can create your own mixin that sets your custom tags however you like and then adds them to the AWS provider default_tags. This gets your tags set everywhere without involving null-label.

Alternately, you can create a custom version of context.tf and replace the line

tags                = var.tags

with

tags                = merge(local.introspection_tags, var.tags)

and populate local.introspection_tags however you want. Put this version of context.tf in all of your root modules and you are done. If you are using cloudposse/terraform-aws-components and are worried about your context.tf file getting overwritten when pulling in a new version of the component, name your file context_override.tf and the 2 files can exist side by side, with yours taking precedence.

If you develop your custom context.tf solution and are happy with it and want to share it, I would consider recommending we publish it (maybe under exports/) as an alternative for people to use in their root modules, to benefit the wider community. I just do not think this kind of feature belongs in the null-label we use in every module.

@Nuru
Copy link
Contributor

Nuru commented Jul 23, 2023

@gberenice @Gowiem Here is another way to achieve your goals. In your root modules, in addition to the current context.tf, add this file. Must be have name ending in _override.tf

root-module-tag_override.tf

module "this" {
  tags = merge(var.tags, {
    "Root_module" = basename(abspath(path.module))
  })
}

Let me know what you think. Unless I hear an objection, I plan to close this PR with this as the recommended alternative.

@gberenice
Copy link
Author

@Nuru thank you. I think we'll figure out the way that works best for our needs based on your advice and our analysis. Much appreciate the feedback from all of you.

@Gowiem
Copy link
Member

Gowiem commented Jul 24, 2023

@Nuru yeah, understood across all. I still believe this has value for the overall community if this functionality were included, but I'd probably be belaboring the point in pushing that further. We'll take this internally and go from there.

Thanks for the thoughtful response and solution engineering, it is appreciated! 👍

@Gowiem Gowiem closed this Jul 24, 2023
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.

5 participants