tfutils / tfenv

Terraform version manager
MIT License
4.48k stars 454 forks source link

Include support of min-required syntax in TFENV_TERRAFORM_VERSION #305

Closed jramosf closed 2 years ago

jramosf commented 2 years ago

This PR adds support of min-required in the TFENV_TERRAFORM_VERSION environment variable.

This way, the .terraform-version files can be skipped as the source of truth for the Terraform version can be inferred from the Terraform Block directly.

The feature is opt-in.

OJFord commented 2 years ago

I like this a lot! In a way this is the main thing I want from tfenv - to be able to hop around between tf projects and have an appropriate version, and with this not even require anything (a .terraform-version file) from the project itself.

I wonder if it's right to give precedence to the env var over a .terraform-version file if one does exist though? If the project is saying 'use this version', isn't that stronger than my user environment having a general 'eh, use the min-required' (aside: I'd love there to be a 'latest-allowed' option!) wish?

jramosf commented 2 years ago

I like this a lot! In a way this is the main thing I want from tfenv - to be able to hop around between tf projects and have an appropriate version, and with this not even require anything (a .terraform-version file) from the project itself.

I wonder if it's right to give precedence to the env var over a .terraform-version file if one does exist though? If the project is saying 'use this version', isn't that stronger than my user environment having a general 'eh, use the min-required' (aside: I'd love there to be a 'latest-allowed' option!) wish?

Thanks for the feedback!

The environment variable takes precedence over the dotfiles.

Regarding latest-allowed, I believe there's no such feature in tfenv at the moment.

Cheers

OJFord commented 2 years ago

The environment variable takes precedence over the dotfiles.

Yes, sorry I don't think I phrased very clearly - I was thinking maybe it's better if the .terraform-version file takes precedence - since it seems stronger to me if the project is specifying a version than a more general variable from the environment.

Regarding latest-allowed, I believe there's no such feature in tfenv at the moment.

Indeed, was just an idle semi-related wish. I subsequently created an issue & PR for that in #309. If this is merged (as I hope it is) then I'll come back to that and add support for it in the env var too, to match min-required.

jramosf commented 2 years ago

The environment variable takes precedence over the dotfiles.

Yes, sorry I don't think I phrased very clearly - I was thinking maybe it's better if the .terraform-version file takes precedence - since it seems stronger to me if the project is specifying a version than a more general variable from the environment.

I believe environment variables should take precedence over configuration files. If I'm not mistaken this is is also the behaviour for Terraform itself. Other HashiCorp products such as Vault follow the same pattern (for instance VAULT_TOKEN env var over .vault-token files).

Regarding latest-allowed, I believe there's no such feature in tfenv at the moment.

Indeed, was just an idle semi-related wish. I subsequently created an issue & PR for that in #309. If this is merged (as I hope it is) then I'll come back to that and add support for it in the env var too, to match min-required.

It's an interesting feature to have, I'd point out that this however might trigger unwanted changes to future versions in the statefile.

JeanFred commented 2 years ago

I gave this a quick try (as this would solve my problem too) − just running ./xyz/.tfenv/bin/terraform results in

.tfenv/libexec/tfenv-exec: line 75: TFENV_TERRAFORM_VERSION: unbound variable

Otherwise, when setting TFENV_TERRAFORM_VERSION, this works as expected.

Hope that helps!

jramosf commented 2 years ago

I gave this a quick try (as this would solve my problem too) − just running ./xyz/.tfenv/bin/terraform results in

.tfenv/libexec/tfenv-exec: line 75: TFENV_TERRAFORM_VERSION: unbound variable

Otherwise, when setting TFENV_TERRAFORM_VERSION, this works as expected.

Hope that helps!

Thanks for the tip! Pushed 41effc0 to address this.

JeanFred commented 2 years ago

Thanks for addressing this :)

I tested this further and realized this would not completely solve my use-case, as I would need to not fail when detection fail, and instead fallback on whatever tfenv would have done. But my use-case is a bit peculiar and perhaps not worth supporting − nevertheless let me describe it :)

We have a large codebase using Terragrunt − “a thin wrapper for Terraform that provides extra tools for working with multiple environments ” − the layout is something like :

.
├── environments
│  ├── prod
│  │ ├── app
|  | |   └── terragrunt.hcl
│  │ ├── base
|  | |   └── terragrunt.hcl
│  │ └── persist
|  | |   └── terragrunt.hcl
│  └── test
│    ├── app
|    |   └── terragrunt.hcl
│    ├── base
|    |   └── terragrunt.hcl
│    └── persist
|        └── terragrunt.hcl
├── modules
    ├── app
    │ └── main.tf  # TF13: required_version = "= 0.13.x"
    ├── base
    │ └── main.tf  # TF12: required_version = "= 0.12.31"
    └── persist
      └── main.tf  # TF12: required_version = "= 0.12.31"

The various environments (prod/app, test/app) are just reference to the Terraform code in modules. These modules are a mix of TF12 or TF13 code. So when we call terragrunt, we have some scripting to figure out which terraform version it should use and pass it through. Alternatively, we could store that information in the terragrunt.hcl files − but then we would be duplicating the information N times (prod/app is the same version as staging/app and test/app etc.)

As outlined in How to manage multiple versions of Terragrunt and Terraform as a team in your IaC project, we realized we could leverage on tfenv to do that for us − as terragrunt first cds into the module directory. First we thought of adding .terraform-versions to each module (which works perfectly in theory, although not in practice because of a small bug/missing feature in terragrunt). Then we realized the information was already in the required_version block − and that it would be great if tfenv could just use that. Hence me finding this PR :) With this − all good − when terragrunt invokes terraform at the module level with TFENV_TERRAFORM_VERSION=min-required, then tfenv finds eg 0.12.31 and everything works.

But there’s a catch: before running terraform at the module level, terragrunt does a first “version check” terraform --version wherever it first runs − for example, in prod/app. And as there is no tf file there, the min-required check fails, rightfully, with [1].

Hence my original request: if there would be a graceful fail and move on to the next possible inference method, then my use case would work − the version check would run with whatever default, and the actual plan/apply would later pick up the correct version.

Do you think it’s something that could be accommodated?

[1]

TFENV_TERRAFORM_VERSION set to "min-required". Detecting minimum required version...
Error: Could not determine required_version based on your terraform sources.
Make sure at least one of your *tf  or *.tf.json files includes a required version section like
terraform {
  required_version = ">= 0.0.0"
}
OJFord commented 2 years ago

I think that's not so niche as you suggest - if I have TFENV_TERRAFORM_VERSION set in my environment system wide to min-required, terragrunt or not it's probably not my intention that I can't run terraform in a project (which could be a regular terraform project) that doesn't specify a required_version.

In some sense the min-required would be the oldest version available... but I think 'move on to the next possible inference method' (.terraform-version file, then use?) as you suggest would be more sensible.

jramosf commented 2 years ago

Pushed 09eb7f8 with the graceful fail for this feature.

JeanFred commented 2 years ago

Pushed 09eb7f8 with the graceful fail for this feature.

Yay! With this my setup works perfectly. Thanks!

jramosf commented 2 years ago

@Zordrak any chance this can get merged? Thanks!

jramosf commented 2 years ago

I'm closing the PR as it's been several months without activity. Please feel free to reopen if this has a chance to get merged. Thanks!

JeanFred commented 2 years ago

It is unfortunate this could not move forward ; it was solving my use case very elegantly :-(