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

fix: Fix terraform_wrapper_module_for_each for when resource name contains 'variable' #573

Conversation

nshenry03
Copy link
Contributor

Put an x into the box if that apply:

  • This PR introduces breaking change.
  • This PR fixes a bug.
  • This PR adds new functionality.
  • This PR enhances existing functionality.

Description of your changes

There are some providers with resources and data sources that have "variable" in their name, for example:

When using such resources, terraform_wrapper_module_for_each was adding these as variables to pass in wrappers/main.tf which breaks the wrapper


│ Error: Unsupported argument

│ on main.tf line 14, in module "wrapper":
│ 14: env0_configuration_variable = try(each.value.env0_configuration_variable, var.defaults.env0_configuration_variable)

│ An argument named "env0_configuration_variable" is not expected here.

Validation failed: wrappers

My fix simply improves grep to ensure that module_vars (and module_outputs and module_providers) only find actual variables, outputs, and providers and NOT resources.

How can we test changes

Use the examples provided in the documentation for env0_configuration_variable and try terraform_wrapper_module_for_each as-is today. You'll see that it adds these as variables to pass to the module when it shouldn't.

Now test with my fix and you'll see that it fixes the issue.

@@ -321,14 +321,14 @@ EOF

# Get names of module variables in all terraform files
# shellcheck disable=SC2207
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep variable. | cut -d'.' -f 2 | sort || true; }))
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep "^\s*variable." | cut -d'.' -f 2 | sort || true; }))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep "^\s*variable." | cut -d'.' -f 2 | sort || true; }))
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep "^variable." | cut -d'.' -f 2 | sort || true; }))

This should be enough. I don't think there can/should be any space before the "variable".

Please apply the same fix on other lines in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there can/should be any space before the "variable".

Leading spaces do not produce errors, trigger failures and are not otherwise disallowed explicitly. Hence should be considered allowed imho.
On the other end hcledit block list seems to strip leading spaces, which means Anton's recommendation is correct =)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also would like to recommend to escape dot to not mix it with "any char" in this use case:

Suggested change
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep "^\s*variable." | cut -d'.' -f 2 | sort || true; }))
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep "^variable\." | cut -d'.' -f 2 | sort || true; }))

Copy link
Collaborator

Choose a reason for hiding this comment

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

And as Anton mentioned: please apply the same to other code bits in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Please resolve if this looks sufficient. Thanks!

@nshenry03 nshenry03 force-pushed the fix-terraform-wrapper-module-for-each branch from f2cf876 to c57e68c Compare September 21, 2023 22:51
@antonbabenko antonbabenko merged commit 941177e into antonbabenko:master Sep 22, 2023
5 checks passed
antonbabenko pushed a commit that referenced this pull request Sep 22, 2023
## [1.83.4](v1.83.3...v1.83.4) (2023-09-22)

### Bug Fixes

* Fix terraform_wrapper_module_for_each for when resource name contains 'variable' ([#573](#573)) ([941177e](941177e))
@antonbabenko
Copy link
Owner

This PR is included in version 1.83.4 🎉

Entzerren81 added a commit to Entzerren81/pre-commit-terraform that referenced this pull request Sep 10, 2024
## [1.83.4](antonbabenko/pre-commit-terraform@v1.83.3...v1.83.4) (2023-09-22)

### Bug Fixes

* Fix terraform_wrapper_module_for_each for when resource name contains 'variable' ([#573](antonbabenko/pre-commit-terraform#573)) ([165966c](antonbabenko/pre-commit-terraform@165966c))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants