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

Avoid map-related casting errors in project factory #1836

Merged
merged 8 commits into from
Nov 2, 2023
Merged

Conversation

ludoo
Copy link
Collaborator

@ludoo ludoo commented Nov 1, 2023

Using a map in the for loop that reads and decodes the project YAML data files can trigger unpredictable errors, as Terraform tries to enforce the same type for every map value. This cannot work in our case since most of the project attributes are optional, or accept a variable number of values (e.g. variable number of project services).

The solution implemented here is to replace the map with two lists: one for the YAML file names, and one for the actual raw data decoded from each YAML file. They are then recombined as map keys and values when the final project data structure is assembled for each element.

This PR also simplifies the variable used for the data path by removing the option to pass a structure with project data. This has never been needed so far and only complicates the module.

An additional test is added to the README to replicate an actual use case that is fixed by this change.

@ludoo ludoo changed the title Try to repro Roberto's pf example error Avoid map-related casting errors in project factory Nov 1, 2023
@ludoo ludoo marked this pull request as ready for review November 1, 2023 19:55
@ludoo ludoo requested a review from sruffilli November 1, 2023 19:55
@ludoo ludoo enabled auto-merge (squash) November 1, 2023 20:08
@ludoo ludoo added the incompatible change Pull request that breaks compatibility with previous version label Nov 1, 2023
Copy link
Collaborator

@wiktorn wiktorn left a comment

Choose a reason for hiding this comment

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

Looks good.

I did some experimentation here just to better understand Terraform internals. It looks like TF is verifying the type within the map only in trenary operator. So the minimal fix for added example can look like that:

diff --git a/blueprints/factories/project-factory/factory.tf b/blueprints/factories/project-factory/factory.tf
index e0351a0a..53017266 100644
--- a/blueprints/factories/project-factory/factory.tf
+++ b/blueprints/factories/project-factory/factory.tf
@@ -16,10 +16,8 @@
 
 locals {
   _data = (
-    var.factory_data.data != null
-    ? var.factory_data.data
-    : {
-      for f in fileset("${local._data_path}", "**/*.yaml") :
+    {
+      for f in fileset(local._data_path, "**/*.yaml") :
       trimsuffix(f, ".yaml") => yamldecode(file("${local._data_path}/${f}"))
     }
   )

That is, just refactoring the variable and getting rid of factory.data is sufficient. 🤯

@ludoo ludoo merged commit de0325b into master Nov 2, 2023
13 checks passed
@ludoo ludoo deleted the ludo/pf-tomap branch November 2, 2023 07:24
@ludoo
Copy link
Collaborator Author

ludoo commented Nov 2, 2023

Looks good.

I did some experimentation here just to better understand Terraform internals. It looks like TF is verifying the type within the map only in trenary operator. So the minimal fix for added example can look like that:

diff --git a/blueprints/factories/project-factory/factory.tf b/blueprints/factories/project-factory/factory.tf
index e0351a0a..53017266 100644
--- a/blueprints/factories/project-factory/factory.tf
+++ b/blueprints/factories/project-factory/factory.tf
@@ -16,10 +16,8 @@
 
 locals {
   _data = (
-    var.factory_data.data != null
-    ? var.factory_data.data
-    : {
-      for f in fileset("${local._data_path}", "**/*.yaml") :
+    {
+      for f in fileset(local._data_path, "**/*.yaml") :
       trimsuffix(f, ".yaml") => yamldecode(file("${local._data_path}/${f}"))
     }
   )

That is, just refactoring the variable and getting rid of factory.data is sufficient. 🤯

Ach, I had set automerge. Can you send a patch to implement yours, which is much more readable?

wiktorn added a commit that referenced this pull request Nov 2, 2023
…tory-fix

Simplify #1836 fix, avoid map-related casting errors in project factory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible change Pull request that breaks compatibility with previous version on:blueprints on:FAST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants