search

terraform-google-modules / terraform-example-foundation

Shows how the CFT modules can be composed to build a secure cloud foundation
https://cloud.google.com/architecture/security-foundations
Apache License 2.0
1.18k stars 702 forks source link

fix: #1231 - align 1-org labels #1232

Closed fmichaelobrien closed 1 month ago

fmichaelobrien commented 2 months ago

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

fmichaelobrien commented 1 month ago

Possibly merge after the following and sync this branch https://github.com/terraform-google-modules/terraform-example-foundation/pull/1241#pullrequestreview-2064164125

fmichaelobrien commented 1 month ago

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 commented 1 month ago

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.

fmichaelobrien commented 1 month ago

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