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: #1231 - align 1-org labels #1232

Conversation

fmichaelobrien
Copy link
Contributor

As part of #1231
discovered as part of 387 tef upstream sync

@fmichaelobrien
Copy link
Contributor Author

Possibly merge after the following and sync this branch
#1241 (review)

Copy link
Collaborator

@eeaton eeaton left a comment

Choose a reason for hiding this comment

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

I want to make sure I understand the intent of this PR... Are you saying the value of application_name should match the environment code?

Currently there is not a consistent convention for application-name to match the environment code. A few projects in this stage coincidentally do include the "net" environment code, but I don't think we should try to consistently include the env code in the application name. My reasoning:

  1. For downstream use cases where I want to query project labels for all things in a given environment code, it's more reliable to query on the env label instead of regex matching the application_name label.
  2. In later stages where a given application is deployed across each of d/n/p environments, the same application_name is used across all environments. (example). This is useful if I wanted to query all the projects related to a given app across multiple environments.

@fmichaelobrien
Copy link
Contributor Author

Thank you for the review eeaton, I ran into this minor change when merging upstream into a downstream branch. I'll do another review on the changes that were done in this yaml be eod. I am quoting from the issue #1231 below - essentially this change aligns one 2 of the changes to label naming that were done previously.
I will see if there is something I missed in the alignment and get back to the team.

Quote
tef minor bug for label changes in https://github.com/terraform-google-modules/terraform-example-foundation/pull/1199/files#diff-d6697e7c916ba73d6ae87ff4b1ce67cabc9b9738ab31c9ba582e2a3218982838L279

https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/1-org/envs/shared/projects.tf#L253
match
https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/1-org/envs/shared/projects.tf#L237

-    application_name  = "org-dns-hub"
+    application_name  = "org-net-dns"

and

-net-interconnect

just like in
https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/1-org/envs/shared/projects.tf#L295

@eeaton
Copy link
Collaborator

eeaton commented May 22, 2024

My rationale is that the application_name doesn't really matter, so long as it is intelligible to the end-user. We haven't proposed a convention for application_name, and customers will likely tweak this in their own implementation to something like an ITSM number.

The changes you proposed make a partial match for substrings between application_name and env, but it's not a consistent convention. Most other projects in 1-org/envs/shared/projects.tf do not use the env substring at all in application_name, so the changes just to interconnect and DNS projects actually introduce more inconsistencies.

I'll close this for now but feel free to re-open if you disagree.

@eeaton eeaton closed this May 22, 2024
@fmichaelobrien
Copy link
Contributor Author

Sounds good, understood, yes I would like to keep this file consistent with the naming convention, thanks eeaton for the review

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

Successfully merging this pull request may close these issues.

None yet

2 participants