tfutils / tfenv

Terraform version manager
MIT License
4.43k stars 450 forks source link

chore(log): make TFENV_DIR update a debug-level log #343

Closed neilmcgibbon closed 2 years ago

neilmcgibbon commented 2 years ago

After the most recent commit, the new message: Set TFENV_DIR to <x> is always sent to stdout. This means using Terraform outputs in our build scrips fail.

For example, we use the following to look for files that need formatting: terraform fmt --list=true -diff=true -write=false and if the line count is greater than 0 (indicating a file per line needing formatting) our build fails.

After the recent commit we now have the output from terraform fmt polluted with the the Set TFENV_DIR to <x>, which means we can no longer rely on Terraform's (native) output.

I believe this log severity should be debug to avoid having to work around tfenv output when running "native" Terraform commands.

Update: Of course I can change the severity level as a tfenv environment variable, but I still believe that this is a debug level message and should not be a requirement for implementations to change.

OJFord commented 2 years ago

More generally, cf. #310

Zordrak commented 2 years ago

Is this log line even in the right place? Do we want it called for every passed argument?

Zordrak commented 2 years ago

@neilmcgibbon @OJFord

Unless I've missed something about the intention of the original logic.. is this not cleaner - on the basis TFENV_DIR is already set to PWD and doesnt need to be reset to PWD if it hasnt been changed.

https://github.com/tfutils/tfenv/pull/345

jacobdonenfeld commented 2 years ago

+1, also broke my build script.

OJFord commented 2 years ago

@Zordrak Yes no problem as far as I'm concerned - the TFENV_DIR log and setting to PWD were artefacts of its original location before rebasing: https://github.com/tfutils/tfenv/blob/0b34f4a81eb2bec2220603995b387293d8c39755/bin/tfenv#L64-L65

and the break is no big deal, just unnecessary optimisation I suppose. (There'll only be at most one -chdir arg, so exit the loop once found.)

Zordrak commented 2 years ago

Its usual for terraform to support multiple declarations of the same flag with the final declaration taking precedence so it's not wrong to support that.

Zordrak commented 2 years ago

Superseded by #345