-
Notifications
You must be signed in to change notification settings - Fork 889
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
base: master
Are you sure you want to change the base?
Allow duplicate file names in project hierarchy #2661
Conversation
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. |
a901065
to
eed3933
Compare
eed3933
to
c9220f1
Compare
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? |
There was a problem hiding this 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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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:
/
replaced with-
(in the abscence of an explicitname:
key in the yaml)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 setname:
key, and produces the following resource:The second file,
team-b/dev/ce-0.yaml
does not have an explicitly setname:
key, and produces the following resource with the project name, ID and TF map key built from the path + filename:Checklist
If applicable, I acknowledge that I have:
terraform fmt
on all modified filestools/tfdoc.py