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 the terraform_wrapper_module_for_each hook for modules without outputs or variables #552

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

ajax-ryzhyi-r
Copy link
Contributor

…do not have outputs or variables

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

The terraform_wrapper_module_for_each hook always exits with 1 code for modules that do not have outputs or variables defined. This is because grep returns an exit code of 1 when it does not find a match, and pipefail is set for the hook script.

This pull request resolves the issue by setting a 0 exit code for variables and outputs grep executions.

How can we test changes

Run the terraform_wrapper_module_for_each hook against the module without outputs or variables. 🙃

Copy link
Owner

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Looks very good. Thank you for opening this PR!

hooks/terraform_wrapper_module_for_each.sh Show resolved Hide resolved
hooks/terraform_wrapper_module_for_each.sh Show resolved Hide resolved
hooks/terraform_wrapper_module_for_each.sh Outdated Show resolved Hide resolved
hooks/terraform_wrapper_module_for_each.sh Outdated Show resolved Hide resolved
@antonbabenko antonbabenko changed the title fix: Fix the terraform_wrapper_module_for_each hook for modules that … fix: Fix the terraform_wrapper_module_for_each hook for modules that don't have outputs or variables Aug 10, 2023
@antonbabenko antonbabenko changed the title fix: Fix the terraform_wrapper_module_for_each hook for modules that don't have outputs or variables fix: Fix the terraform_wrapper_module_for_each hook for modules without outputs or variables Aug 10, 2023
@antonbabenko antonbabenko merged commit f24b3fa into antonbabenko:master Aug 10, 2023
antonbabenko pushed a commit that referenced this pull request Aug 10, 2023
## [1.81.1](v1.81.0...v1.81.1) (2023-08-10)

### Bug Fixes

* Fix the terraform_wrapper_module_for_each hook for modules without outputs or variables ([#552](#552)) ([f24b3fa](f24b3fa))
@antonbabenko
Copy link
Owner

This PR is included in version 1.81.1 🎉

hooks/terraform_wrapper_module_for_each.sh Show resolved Hide resolved
hooks/terraform_wrapper_module_for_each.sh Show resolved Hide resolved
@@ -323,11 +323,11 @@ 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))
module_vars=($(echo "$all_tf_content" | hcledit block list | { grep variable. || true; } | cut -d'.' -f 2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we put cut to where it belongs in logically?

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


# Get names of module outputs in all terraform files
# shellcheck disable=SC2207
module_outputs=($(echo "$all_tf_content" | hcledit block list | grep output. | cut -d'.' -f 2))
module_outputs=($(echo "$all_tf_content" | hcledit block list | { grep output. || true; } | cut -d'.' -f 2))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we put cut to where it belongs in logically?

Suggested change
module_outputs=($(echo "$all_tf_content" | hcledit block list | { grep output. || true; } | cut -d'.' -f 2))
module_outputs=($(echo "$all_tf_content" | hcledit block list | { grep output. | cut -d'.' -f 2 || true; }))

@antonbabenko
Copy link
Owner

@ajax-ryzhyi-r just discovered the error using this PR:

Terraform wrapper with for_each in module................................Failed
- hook id: terraform_wrapper_module_for_each
- exit code: 1

failed to build expression at the parse phase: failed to parse input: generated_by_buildExpression:21,7-7: Unterminated template string; No closing marker was found for the string.

Could you please work on the fix?

@ajax-ryzhyi-r
Copy link
Contributor Author

@ajax-ryzhyi-r just discovered the error using this PR:

Terraform wrapper with for_each in module................................Failed
- hook id: terraform_wrapper_module_for_each
- exit code: 1

failed to build expression at the parse phase: failed to parse input: generated_by_buildExpression:21,7-7: Unterminated template string; No closing marker was found for the string.

Could you please work on the fix?

I'm not sure if this error is related to the changes made in this pull request. I've tried to reproduce it but with no success. 😞

The error you discovered appears to have originated from one of the hcledit attribute append commands. If I understand correctly, it is related to command input parsing. Could you please provide more details on how to reproduce it? If it is related to one of the public modules, please let me know which one. Thank you 🙃

@antonbabenko
Copy link
Owner

@ajax-ryzhyi-r Try checkout this module locally - https://github.com/terraform-aws-modules/terraform-aws-appsync/ - and add - id: terraform_wrapper_module_for_each into hooks. It works better with version v1.81.0 but not with v1.81.1.

I think the fix you have proposed is relevant and needed.

@ajax-ryzhyi-r
Copy link
Contributor Author

@ajax-ryzhyi-r Try checkout this module locally - https://github.com/terraform-aws-modules/terraform-aws-appsync/ - and add - id: terraform_wrapper_module_for_each into hooks. It works better with version v1.81.0 but not with v1.81.1.

I think the fix you have proposed is relevant and needed.

@antonbabenko The issue also occurs with version v1.81.0. Please take a look at the proposed fix #554.

@antonbabenko
Copy link
Owner

This issue has been resolved in version 1.81.2 🎉

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