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

feat: Hook terraform_wrapper_module_for_each should use versions.tf from the module if it exists #657

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

Rather than having hooks/terraform_wrapper_module_for_each.sh always create versions.tf for us, if the module has versions.tf, use that instead.

We want to do this because if your module uses a non-Hashicorp provider and versions.tf doesn't have a the required required_providers config, then Terraform will try to download the provider from Hashicorp (i.e., for the env0 provider, it would try to download hashicorp/env0 which doesn't exist and NOT env0/env0)... We COULD set this in the deployment code; however, if you're using Terragrunt and you configure the provider with the required_providers config, then if you use a root module (with proper versions.tf) with the provider, then you get a conflict because you have required_providers configured in the module AND required_providers configured in the deployment code.

How can we test changes

When you run pre-commit, wrappers/versions.tf should be copied from / the same as /versions.tf.

yermulnik
yermulnik previously approved these changes Apr 10, 2024
@MaxymVlasov MaxymVlasov requested review from antonbabenko and removed request for MaxymVlasov April 12, 2024 14:11
@MaxymVlasov MaxymVlasov added bug Something isn't working hook/terraform_wrapper_module_for_each Bash hook labels Apr 12, 2024
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.

I'm not sure that I agree or fully understand the reasoning for this PR. Let me think a bit and come back to it on Monday.

@yermulnik
Copy link
Collaborator

yermulnik commented Apr 12, 2024

I'm not sure that I agree or fully understand the reasoning for this PR. Let me think a bit and come back to it on Monday.

The context from how I understood author's idea: it either pre-defined quite permissive constraint (see screenshot) or the user can provide it's own configuration.
image

While it looks more or less safe, I'd support @antonbabenko hesitaion as that user-provided file may contain something irrelevant and hence user can inadvertently shoot their leg.
So it's probably may make sense to allow this usage by an explicit argument passed to this hook, so that user sort of confirms they understand what they do.
The arg can rely on a pre-defined file name, or may require an explicit file name (the latter seems safer).

Just thoughts aloud.

ps: I'll take by approval back to prevent accidental merge to allow Anton to has his "veto" take actual effect.

@yermulnik yermulnik dismissed their stale review April 12, 2024 18:22

Let Anton review this change

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.

I now understand the reason for this PR - if the module requires something (e.g. providers of certain versions or the specific range of the Terraform versions), so should the wrapper do, too.

This is a new feature, not a small fix.

@antonbabenko antonbabenko changed the title fix: Use root module versions.tf if it exists feat: Hook terraform_wrapper_module_for_each should use versions.tf from the module if it exists Apr 15, 2024
@antonbabenko antonbabenko merged commit b127601 into antonbabenko:master Apr 15, 2024
7 checks passed
antonbabenko pushed a commit that referenced this pull request Apr 15, 2024
# [1.89.0](v1.88.4...v1.89.0) (2024-04-15)

### Features

* Hook terraform_wrapper_module_for_each should use versions.tf from the module if it exists ([#657](#657)) ([b127601](b127601))
@antonbabenko
Copy link
Owner

This PR is included in version 1.89.0 🎉

@MaxymVlasov MaxymVlasov added feature New feature or request and removed bug Something isn't working labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants