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

Allow duplicate file names in project hierarchy #2661

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

robrankin
Copy link
Contributor

@robrankin robrankin commented Nov 5, 2024

The Project Factory module doesn't allow duplicate filenames in the project hierarchy folder structure, as the filenames are currently used as keys in TF maps, and duplicates lead to duplicate key errors.

This change moves to building the map keys from either:

  • The full path + filename with the separator of / replaced with - (in the abscence of an explicit name: key in the yaml)
  • The name: key explicitly set in the project yaml definition file.

I've included two example project files, both named ce-0.yaml, but in different parts of the project hierarchy.

The first file, team-a/dev/ce-0.yaml has an explicitly set name: key, and produces the following resource:

  # module.projects.module.projects["ta-d-ce-0"].google_project.project[0] will be created
  + resource "google_project" "project" {
      + name                = "main-ta-d-ce-0"
      + project_id          = "main-ta-d-ce-0"
    }

The second file, team-b/dev/ce-0.yaml does not have an explicitly set name: key, and produces the following resource with the project name, ID and TF map key built from the path + filename:

  # module.projects.module.projects["team-b-dev-ce-0"].google_project.project[0] will be created
  + resource "google_project" "project" {
      + name                = "main-team-b-dev-ce-0"
      + project_id          = "main-team-b-dev-ce-0"
    }

Checklist

If applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

Copy link

google-cla bot commented Nov 5, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@robrankin robrankin force-pushed the robrankin/allow_dupe_project_files_in_hierarchy branch 2 times, most recently from a901065 to eed3933 Compare November 5, 2024 13:57
@robrankin robrankin force-pushed the robrankin/allow_dupe_project_files_in_hierarchy branch from eed3933 to c9220f1 Compare November 5, 2024 14:54
@juliocc
Copy link
Collaborator

juliocc commented Nov 16, 2024

Not a bad idea but if we do this for PF we should probably do it for all other factories too.

I actually like the idea and the fix is fairly simple.

@ludoo wdyt?

Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

Forgot to send comments lol

@@ -20,7 +20,7 @@ locals {
_hierarchy_projects = (
{
for f in try(fileset(local._folders_path, "**/*.yaml"), []) :
basename(trimsuffix(f, ".yaml")) => merge(
replace(trimsuffix(f, ".yaml"), "/", "-") => merge(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer having the / being part of the key, to make it clear which part is the hierarchy and which is the project. Keep in mind the primary use case is using the key in substitutions (e.g. automation.project = "foo/bar"). Let's keep the path intact, and then find a way to map that to project ids. Probably by moving the replace there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you comment on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the problem (I think) is that '/' is invalid in project ids

Copy link
Collaborator

@ludoo ludoo Nov 16, 2024

Choose a reason for hiding this comment

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

We need to keep the path for keys, not project ids

modules/project-factory/factory-projects.tf Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants