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

Stop using the "prehistoric" terraform naming for modules. #138

Closed
wants to merge 2 commits into from

Conversation

gillg
Copy link

@gillg gillg commented Jan 26, 2023

To prevent bad practices by people downloading this file as a bootstrap, it will be a lot better to have a module name not meaningless.

what

  • rename the module and exemples in the file exports/context.tf

why

  • Avoid the name this which is meaningless, confusing, and was a kind of bad habbit in first ages of terraform.
  • Explicit the name of the module by naming so you can use it like module.naming.tags

To prevent bad practices by people downloading this file as a bootstrap, it will be a lot better to have a module name not meaningless.
@gillg gillg requested a review from a team as a code owner January 26, 2023 15:10
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@gillg thanks for the PR.

This is a complicated topic, and we will not make the change easily b/c it will affect everything else (all other terraform modules and components).

  1. Since context.tf is a mixin, this is supposed to mean not the null-label module, but the parent/root module itself where you use the context. So module.this.xxx means the module where you are now, and naming it this makes perfect sense
  2. naming is not a good name either, since it's confusing as well. Maybe context would be a better name
  3. Changing label will affect everything else, and we, and all other people, will have to update almost all TF code they have now

All of this is not a simple decision, and def needs to be discussed.

@osterman @Nuru @nitrocode @mcalhoun @goruha

@gillg
Copy link
Author

gillg commented Jan 26, 2023

@aknysh thanks for your very interesting answer !

I don't get your point about

Since context.tf is a mixin, this is supposed to mean not the null-label module, but the parent/root module itself where you use the context. So module.this.xxx means the module where you are now, and naming it this makes perfect sense

The module label is named this, so it's necessarly the null-label module itself ? In term of TF path it will be module.parent.module.this.xxxx so it's still meaningless when you read your plan or your state resources no ? You never know (except by bad experice) that this is related to the naming.

But I easily understand all your other points about complexity of migration. Maybe a complex but meainingfull step ? Maybe doable in multiple steps, but that should triggers zero change in all the TF stacks, and I would say there is no need for users to necessarly change their modules names... The can continue to use this if they prefer. That will just affect new deployments in theory no ?

Copy link
Sponsor 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.

The export context.tf is used in all (or nearly all) Cloud Posse Terraform modules. Any change would have a huge ripple effect, causing hundreds of modules to need to be manually updated. Any such change would have to provide a highly compelling benefit. Changing this to naming does not meet this standard.

Anyone is free to not use context.tf or to use their own version, so that is one way for you to gain the benefit you seek.

Personally, since common uses are module.this.id and module.this.tags I do not see any benefit to changing that to module.naming.id, and while reasonable people may differ, finding a name that everyone likes is one of the hardest challenges in computer science, so I doubt making the change would improve the proportion of users who are happy with the name.

I acknowledge that module.this.context, which is by far the most common use, can be confusing and could be better named; perhaps module.this.label_context would have been a better choice. But again, that change would have a huge ripple effect and would not be worth the effort.

We are considering a major redesign of this module, to drop the idea of predefined special label names (like stage and environment) in favor of a more flexible map. You could, if you want to, open an issue with suggestions for naming changes and/or other improvements and we will consider them for the next version, but be aware that this next version has no timetable associated with it.

@Nuru Nuru added the wontfix This will not be worked on label Aug 1, 2023
@Nuru
Copy link
Sponsor Contributor

Nuru commented Aug 1, 2023

Closing as this is not something we will accept for the reasons outlined above.

TL;DR Changing the name will have a huge impact, and, as names are largely a matter of preference, does not guarantee any overall benefit. It is plausible that fewer people will like the new naming convention than the current one.

@Nuru Nuru closed this Aug 1, 2023
@osterman
Copy link
Member

osterman commented Aug 1, 2023

Avoid the name this which is meaningless, confusing, and was a kind of bad habbit in first ages of terraform.

I think some context may be missing. The terms this and self are pretty common in languages like C++, Python, and Perl. While Terraform does not support object-oriented programming, the terms were inspired by conventions in other languages.

Explicit the name of the module by naming so you can use it like module.naming.tags

I'm not familiar with the naming variable convention; I've not seen the convention used elsewhere, but please share documentation to the convention.

Keep in mind that terraform-null-label is the first module of its kind in the Terraform ecosystem, and now many other projects have adopted this "label" convention introduced by Cloud Posse. This module is now 8+ years old, and there are many things we would do differently now that Terraform has evolved; however, due to the limitations of Terraform at the time we first implemented it, and a general commitment to reducing breaking changes for our community, we've elected to reduce volatile changes to null label.

We have plans to evolve null-label without breaking backward compatibility, such as allowing for arbitrary taxonomies, but changing our naming convention of this is not something we've considered and would require significant discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants