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: Fixed ordering issue in terraform_wrapper_module_for_each hook #565

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

ajax-ryzhyi-r
Copy link
Contributor

@ajax-ryzhyi-r ajax-ryzhyi-r commented Sep 4, 2023

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

I have a module with variables defined in two .tf files. When I run the terraform_wrapper_module_for_each hook locally on macOS, no changes are shown. However, when the same hooks run against the same module by GitHub Actions on Ubuntu, a difference in the order of the wrapper module input variables is shown. After researching the root cause of this behavior, I discovered that the find utility returns files in a different order on macOS and Ubuntu because these OS use different virtual file systems:
https://unix.stackexchange.com/questions/13451/what-is-the-directory-order-of-files-in-a-directory-used-by-ls-u

To solve this issue, I added sorting for the outputs and variables before appending them to the wrapper module.

Note: These changes could cause updates to already generated wrappers.

How can we test changes

Run the hooks against module with the next structure in MacOS and Ubuntu:

main.tf
variables.tf             # Some variables are defined here
additional_variables.tf  # Some variables are defined here


# 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 || true; }))
module_outputs=($(echo "$all_tf_content" | hcledit block list | { grep output. | cut -d'.' -f 2 | sort || true; }))

# Get names of module providers in all terraform files
module_providers=$(echo "$all_tf_content" | hcledit block list | { grep provider. || true; })
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ajax-ryzhyi-r Would it make sense to add sorting to this var definition as well for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajax-ryzhyi-r Would it make sense to add sorting to this var definition as well for consistency?

This variable is only used to check if the module contains provider definitions, so the ordering does not matter. However, for the sake of code consistency, sorting can be added here as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@MaxymVlasov What do you think? I'd add sorting for this var value too.

Copy link
Owner

Choose a reason for hiding this comment

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

Good idea to add sort, but it is enough to have it just in module_vars as it is the only variable used in a loop when making a file.

Also, tflint may help with this. It would complain if variable blocks were defined in various places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea to add sort, but it is enough to have it just in module_vars as it is the only variable used in a loop when making a file.

Also, tflint may help with this. It would complain if variable blocks were defined in various places.

Yes, you're right. The order of outputs does not matter too. Thank you for pointing that out.

I am aware that tflint can detect such cases, but we still have some legacy modules where this structure is expected 😢

@antonbabenko antonbabenko changed the title fix: Fix ordering issue in terraform_wrapper_module_for_each hook fix: Fixed ordering issue in terraform_wrapper_module_for_each hook Sep 4, 2023
@antonbabenko antonbabenko merged commit dc12be1 into antonbabenko:master Sep 4, 2023
4 checks passed
antonbabenko pushed a commit that referenced this pull request Sep 4, 2023
## [1.83.1](v1.83.0...v1.83.1) (2023-09-04)

### Bug Fixes

* Fixed ordering issue in terraform_wrapper_module_for_each hook ([#565](#565)) ([dc12be1](dc12be1))
@antonbabenko
Copy link
Owner

This PR is included in version 1.83.1 🎉

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