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: Support custom TF paths which contains spaces #714

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

robinbowes
Copy link
Contributor

@robinbowes robinbowes commented Sep 5, 2024

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

Add quotes around variable containing path to terraform so it works when the path contains a space.

Fixes #713

How can we test changes

@yermulnik
Copy link
Collaborator

@robinbowes Thanks for the contribution. It looks like we'd better quote all instances of $tf_path. Would you be up to update your PR accordingly? See #713 (comment) just in case to avoid race condition =)

> sift -n '[^"]\$tf_path'
hooks/terraform_validate.sh:121:  $tf_path validate "${args[@]}" &> /dev/null && {
hooks/terraform_validate.sh:135:    validate_output=$($tf_path validate "${args[@]}" 2>&1)
hooks/terraform_validate.sh:139:    validate_output=$($tf_path validate -json "${args[@]}" 2>&1)
hooks/terraform_validate.sh:162:      validate_output=$($tf_path validate "${args[@]}" 2>&1)
hooks/terraform_providers_lock.sh:158:  $tf_path providers lock "${args[@]}"
hooks/_common.sh:539:      init_output=$($tf_path init -backend=false "${TF_INIT_ARGS[@]}" 2>&1)
hooks/_common.sh:545:        init_output=$($tf_path init -backend=false "${TF_INIT_ARGS[@]}" 2>&1)
hooks/terraform_fmt.sh:52:  $tf_path fmt "${args[@]}"

@robinbowes
Copy link
Contributor Author

@robinbowes Thanks for the contribution. It looks like we'd better quote all instances of $tf_path. Would you be up to update your PR accordingly? See #713 (comment) just in case to avoid race condition =)

> sift -n '[^"]\$tf_path'
hooks/terraform_validate.sh:121:  $tf_path validate "${args[@]}" &> /dev/null && {
hooks/terraform_validate.sh:135:    validate_output=$($tf_path validate "${args[@]}" 2>&1)
hooks/terraform_validate.sh:139:    validate_output=$($tf_path validate -json "${args[@]}" 2>&1)
hooks/terraform_validate.sh:162:      validate_output=$($tf_path validate "${args[@]}" 2>&1)
hooks/terraform_providers_lock.sh:158:  $tf_path providers lock "${args[@]}"
hooks/_common.sh:539:      init_output=$($tf_path init -backend=false "${TF_INIT_ARGS[@]}" 2>&1)
hooks/_common.sh:545:        init_output=$($tf_path init -backend=false "${TF_INIT_ARGS[@]}" 2>&1)
hooks/terraform_fmt.sh:52:  $tf_path fmt "${args[@]}"

Heh, I fixed all the occurrences you listed before I saw your comment.

I made one additional change in terraform_providers_lock.sh to use $tf_path instead of the text "terraform" to match the style used in other files.

Should be good to go now.

@robinbowes robinbowes changed the title Fix: Quote path to terraform in terraform_fmt hook fix: Quote path to terraform in terraform_fmt hook Sep 5, 2024
Copy link
Collaborator

@yermulnik yermulnik left a comment

Choose a reason for hiding this comment

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

LGTM
Let's wait for @MaxymVlasov to take a look into this change so that we don't miss something non-obvious

@MaxymVlasov MaxymVlasov changed the title fix: Quote path to terraform in terraform_fmt hook fix(terraform_fmt): Support custom TF paths which contains spaces Sep 9, 2024
@MaxymVlasov MaxymVlasov changed the title fix(terraform_fmt): Support custom TF paths which contains spaces fix: Support custom TF paths which contains spaces Sep 9, 2024
@MaxymVlasov MaxymVlasov merged commit 2bca410 into antonbabenko:master Sep 9, 2024
7 of 8 checks passed
antonbabenko pushed a commit that referenced this pull request Sep 9, 2024
## [1.94.2](v1.94.1...v1.94.2) (2024-09-09)

### Bug Fixes

* Support custom TF paths which contains spaces ([#714](#714)) ([2bca410](2bca410))
@antonbabenko
Copy link
Owner

This PR is included in version 1.94.2 🎉

@robinbowes robinbowes deleted the rb-fix_713 branch September 9, 2024 12:30
@pcarn
Copy link

pcarn commented Oct 9, 2024

This caused a breaking change, FYI.

My CI job calls pre-commit run --all-files, and it started failing with 1.94.2.

I found that terraform was not installed on the CI environment. Before 1.94.2, the pre-commit job would pass anyway. Now it fails for terraform being missing.

It did expose that terraform wasn't actually installed, so it was helpful in that way. But I wanted to call out the breaking change.

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Oct 10, 2024

  1. I don't see how changes in this PR could affect your CI

  2. Improving checks which prevents unexpected behavior (from your description, previously it just bypassed checks) is fix by definition, not breaking change.

  3. Also, it should be checked here -

    #######################################################################
    # Get Terraform/OpenTofu binary path
    # Allows user to set the path to custom Terraform or OpenTofu binary
    # Globals (init and populate):
    # HOOK_CONFIG (array) arguments that configure hook behavior
    # PCT_TFPATH (string) user defined env var with path to Terraform/OpenTofu binary
    # TERRAGRUNT_TFPATH (string) user defined env var with path to Terraform/OpenTofu binary
    # Outputs:
    # If failed - exit 1 with error message about missing Terraform/OpenTofu binary
    #######################################################################
    function common::get_tf_binary_path {
    local hook_config_tf_path
    for config in "${HOOK_CONFIG[@]}"; do
    if [[ $config == --tf-path=* ]]; then
    hook_config_tf_path=${config#*=}
    hook_config_tf_path=${hook_config_tf_path%;}
    break
    fi
    done
    # direct hook config, has the highest precedence
    if [[ $hook_config_tf_path ]]; then
    echo "$hook_config_tf_path"
    return
    # environment variable
    elif [[ $PCT_TFPATH ]]; then
    echo "$PCT_TFPATH"
    return
    # Maybe there is a similar setting for Terragrunt already
    elif [[ $TERRAGRUNT_TFPATH ]]; then
    echo "$TERRAGRUNT_TFPATH"
    return
    # check if Terraform binary is available
    elif command -v terraform &> /dev/null; then
    command -v terraform
    return
    # finally, check if Tofu binary is available
    elif command -v tofu &> /dev/null; then
    command -v tofu
    return
    else
    common::colorify "red" "Neither Terraform nor OpenTofu binary could be found. Please either set the \"--tf-path\" hook configuration argument, or set the \"PCT_TFPATH\" environment variable, or set the \"TERRAGRUNT_TFPATH\" environment variable, or install Terraform or OpenTofu globally."
    exit 1
    fi
    }

    @pcarn Please open an issue if you can provide minimum reproduction example via GH workflows etc, as I not sure how it was even possible to out from function above without error, if there is no binary, and you didn't specify path in configuration explicitly

@pcarn
Copy link

pcarn commented Oct 10, 2024

To be clear, I’m glad this change was made. When terraform was not installed, it was not actually doing any checks or doing any formatting. It succeeded silently.

I’m mentioning it because when it started failing with no changes of my own, I looked at the changelog and nothing looked related. I went version by version to find that it was this that changed it.

minimal steps to reproduce:

  • set up a repo with terraform files that are unformatted.
  • do not have terraform installed.
  • use this hook

Before this commit, it would pass, and make no formatting changes.

After this commit, it fails, and says terraform is not installed. (Which is probably the better behavior)

@MaxymVlasov
Copy link
Collaborator

MaxymVlasov commented Oct 10, 2024

Okay, it's actually asked for terraform binary before < 1.90.0.
But somehow check start bypass in 1.90.0 (#670)
And this regression fixed in some magical way in 1.94.2

I'll add this to release notes

And we need tests here -_-

@pcarn thank you, btw

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.

Terraform hook fails when the terraform binary's path contains a space
5 participants