Key inconsistency in project-factory

[V0idC0de]

This Pull Request addresses two issues I stumbled upon, while working with your nice and shiny framework.

Inconsistent Map Keys for Project Files

Projects configured in folders_data_path have their filename as key (see code, do note the use of basename() to get only the filename), whereas those from projects_data_path have their entire path, relative to projects_data_path, as key (see code, do note the absence of basename()).

This causes project files from the two sources to be key'ed differently, which has some implications down the line.

1. When local.projects is eventually constructed here, the name attribute each YAML file is looked up. If not set, the function resorts to the map-key, which is the filename for project files from folders_data_path and the full relative path for project files from projects_data_path.
2. Moving project files around in the folder hierarchy causes the path of the file to change, which would change the key for projects files from projects_data_path, unless name was specified in the file. As local.projects is used as map for invoking module.projects, this key-change destroys and re-creates projects. This does not happen, if the project file resides anywhere in folders_data_path, since their key is only the filename (basename() was used).
3. Invoking module.projects on a project YAML from projects_data_path without specified name attribute will cause a mis-construction of the project_id. Module project will be handed the map-key as name for the project, which is used together with prefix as project_id. But if the project YAML file happens to reside in a sub-directory, the path will not only contain its filename, but also its parent-folder separated by a /. This causes an error (admittedly in the tf plan stage) which outputs that the project_id is invalid (it may look like this prefix-subdir/projectname). However, this error is not very verbose and unintuitive in this case, since omitting name in project files from folders_data_path works perfectly fine, as the key is always just the filename and a perfectly fine name/project_id.

Solution

The PR uses basename() for files from projects_data_path too, causing the behavior to be identical and name to be an optional setting, just like with files from folders_data_path, which is consistent, in my opinion.

One implication is, that files from folders_data_path can now overwrite entries fetched from projects_data_path, if they happen to use the same name (or filename, in case no name is specified), due to the merge-order, see this merge()-call.

Null when decoding empty YAML files

Another minor detail I noticed was that Terraform returns null when using yamldecode() on a file that only contains --- to indicate the start of the YAML document. An empty project YAML is a perfectly fine configuration, since the filename is used as name and everything else may be added by default values or overrides.

However, parsing an empty YAML document will place null as value in factory-projects.tf:25 and factory-projects.tf:34. This causes an error, when this value is used in lookup() at factory-projects.tf:50, since lookup() does not accept null as parameter InputMap.

Interestingly enough, this also happens only to project files from projects_data_path, since those from folders_data_path are merge()d with {parent = dirname(f) ...} at factory-projects.tf:24. merge() actually accepts null as argument, treating it as { }, so there is always at least a map/object present as value for those project files. This is inconsistent behavior, in my opinion and YAML objects from project files should be sanitized to be at least { } instead of null, allowing empty project files for both folders_data_path and projects_data_path.

Solution

I added a second commit to my PR, which wraps yamldecode() in coalesce() resorting to { } should the yamldecode() invocation return null, making the behavior consistent for both cases.

---

Checklist

I 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

[ludoo]

Great debugging work on non-trivial project factory use cases. :) Let me read this and approve, if tests fail just ping here if you end up struggling with our framework and I'll try to help or fix those myself.

[ludoo]

TFlint does not like coalesce there, probably with good reason. You might try catching the nulls in the next stage in the pipeline, but given the amount of issues we had with ternary operators and type checking on maps, I would be very wary of doing it there.

What if you set a harmless attribute in the empty YAML files instead? Something like labels: {} which should make yamldecode produce a map?

[V0idC0de]

Wow, I'm actually impressed by that response speed 😀

The Linter doesn't seem to like my coalesce() call since "all arguments must have the same type".

However, when verifying in terraform console, the command works out perfectly and looks a lot like coalesce() is intended to be used.

> yamldecode("---")
null
> coalesce(yamldecode("---"), {})
{}

[ludoo]

Wow, I'm actually impressed by that response speed 😀

The Linter doesn't seem to like my coalesce() call since "all arguments must have the same type".

However, when verifying in terraform console, the command works out perfectly and looks a lot like coalesce() is intended to be used.

> yamldecode("---")
null
> coalesce(yamldecode("---"), {})
{}

Heh, FAST tests also fail with the same error. The tf type checker behaves badly with maps in ternaries and other situations, reading the map as an object and enforcing the same type on both sides which clearly does not work.

What if you skipped the fix for null files, and just used a workaround in that case? A bit suboptimal but better than risking breaking the project factory, which has a very fragile balance in its locals that cost us a lot of trouble.

[V0idC0de]

Hm, evaluating with type() returns the following:

> type(yamldecode("{'test': 42}"))
object({
    test: number,
})

> type({})
object({})

> type(yamldecode(file("path/to/file.yaml")))
dynamic

So apparently it's both object, however, yamldecode(file(...)) returns dynamic, since TF cannot be sure, probably. Maybe explicit type-casting can be added? My TF knowledge is limited for these type of linter settings, to be honest.

Practically speaking, most projects files in projects_data_path will at least specify parent, since these do not resort to the special default parent folder by themselves (this only happens for projects in folders_data_path, as I found out - is this difference intended?). However, setting parent with data_defaults or data_overrides is still a valid use-case, which also make empty project files a viable option, so it'd be great to have that. Especially since this seems to be only a Linter issue.

To me it seems, that having type object | null for these map-values is more complex than having a guaranteed object. The latter is a more specific version of the format and makes more sense to me (especially because null is a guaranteed crash, since lookup() won't accept null).

[ludoo]

It still seems like a lot of trouble for a tiny corner case: as you write above projects will almost always have a parent set.

Why don't you split changes so the key fix can go in, and we then take the time to figure out if a solid approach for null can be found?

[V0idC0de]

It still seems like a lot of trouble for a tiny corner case: as you write above projects will almost always have a parent set.

Why don't you split changes so the key fix can go in, and we then take the time to figure out if a solid approach for null can be found?

Fair enough - I removed the commit for coalesce() 👍