fix: Print out the full path in tflint when run through pre-commit #357

thomaslast · 2022-04-07T13:54:38Z

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

This PR aims to fix the issue here: #54

The issue for tflint is resolved by running the pre-commit at the repo level and the full path is then displayed on an error.

How can we test changes

repos:
- repo: https://github.com/thomaslast/pre-commit-terraform
  rev: afde258978090e4d2382fd90bb230347d0b349ac

✅ Work as expected
☑️ - Able to check only passed, not fail scenario
✖️ - work, but not print full path
❌ - failed to start
Not checked

✅ checkov (python, not affected hook)
✅ infracost_breakdown
✅ terraform_docs_replace (deprecated, python, not affected hook)
✖️ terraform_docs_without_aggregate_type_defaults
✖️ terraform_docs
✅ terraform_fmt
❌ terraform_providers_lock
❌ terraform_tflint
✅ terraform_tfsec
☑️ terraform_validate
✖️ terragrunt_fmt
☑️ terragrunt_validate
❌ terrascan

yermulnik · 2022-04-07T14:05:12Z

hooks/_common.sh

@@ -138,7 +138,7 @@ function common::per_dir_hook {
  # run hook for each path
  for dir_path in $(echo "${dir_paths[*]}" | tr ' ' '\n' | sort -u); do
    dir_path="${dir_path//__REPLACED__SPACE__/ }"
-    pushd "$dir_path" > /dev/null || continue
+    [ -f "$dir_path" ] || continue


the [ -f "$dir_path" ] || continue should be [ ! -d "$dir_path" ] && continue

The pushd/popd commands add/remove directories to/from stack, which allows command in between pushd/popd execute inside the directory. By removing pushd/popd you introduce breakage of essential logic of the common::per_dir_hook function.

Fair enough I can change

I have tried manually with tflint and this would work with a full path passed as an argument. Which hooks would be a problem? Could we make this configurable instead?

I took tflint hook as this seems to be what you're trying to amend and you can see that per_dir_hook_unique_part function doesn't take into account the dir path and doesn't change working directory to the dest dir: https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/terraform_tflint.sh#L33-L49
The same function in other hooks seem to behave the same, which means your change would break all those functions.
Please feel free to take a look and try and re-work these per_dir_hook_unique_part functions in all hooks, which have it, to respect the dir_path (the 2nd) parameter passed to them instead of changing directory in parent function.

I've added in the dir_path so that it should run against the file for each hook

yermulnik

I added some more comments re the new portion of changes — please take a look.
Also could you please test-run all hooks you're requesting changes too and provide the output, so that we are sure there's no breakage of existing functionality which may impact other consumers of pre-commit-terraform. Thank you.

@MaxymVlasov Could you please take a look into this PR when you've got a chance so that we all agree upon this kind of changes? Thanks.

yermulnik · 2022-04-11T11:58:53Z

hooks/terraform_tfsec.sh

@@ -36,7 +36,7 @@ function per_dir_hook_unique_part {

  # pass the arguments to hook
  # shellcheck disable=SC2068 # hook fails when quoting is used ("$arg[@]")
-  tfsec ${args[@]}
+  tfsec ${args[@]} "$dir_path"


I'm not quite sure since I don't use tfsec often, though its help page states that directory argument should come first:

> tfsec -h | head -5 tfsec is a simple tool to detect potential security vulnerabilities in your terraformed infrastructure. Usage: tfsec [directory] [flags]

Could you please verify this hook and put dir arg before the rest of args to align with tfsec usage? Thanks.

yermulnik · 2022-04-11T12:08:20Z

hooks/terragrunt_fmt.sh

@@ -33,7 +33,7 @@ function per_dir_hook_unique_part {

  # pass the arguments to hook
  # shellcheck disable=SC2068 # hook fails when quoting is used ("$arg[@]")
-  terragrunt hclfmt ${args[@]}
+  terragrunt hclfmt ${args[@]} "$dir_path"


Based on https://terragrunt.gruntwork.io/docs/reference/cli-options/#hclfmt this wouldn't work for terragrunt. The hclfmt recursively searches current working dir and isn't compatible with providing target working dir as a command line argument, hence the terragrunt hclfmt hook need an explicit pushd/popd workaround.

yermulnik · 2022-04-11T12:13:00Z

hooks/terrascan.sh

@@ -33,7 +33,7 @@ function per_dir_hook_unique_part {

  # pass the arguments to hook
  # shellcheck disable=SC2068 # hook fails when quoting is used ("$arg[@]")
-  terrascan scan -i terraform ${args[@]}
+  terrascan scan -i terraform ${args[@]} "$dir_path"


To provide target working directory to terrascan the -d, --iac-dir string command line flag must be provided:

> terrascan scan -h | fgrep -- --iac-dir -d, --iac-dir string path to a directory containing one or more IaC files (default ".")

Please fix.

MaxymVlasov

Good try, but that will not work, because you are already in $dir_path. Usage of "$(pwd)" will not help too:

Oslo/environment/qa ➜ terraform fmt "$(pwd)"      
main.tf

You need to show an error msg like "that was changed in next dir" but not show it if exit code == 0

MaxymVlasov · 2022-04-13T19:22:55Z

hooks/_common.sh

@@ -138,7 +138,7 @@ function common::per_dir_hook {
  # run hook for each path
  for dir_path in $(echo "${dir_paths[*]}" | tr ' ' '\n' | sort -u); do
    dir_path="${dir_path//__REPLACED__SPACE__/ }"
-    pushd "$dir_path" > /dev/null || continue
+    [ ! -d "$dir_path" ] && continue


Hmm, not mention that. Lmc

Yeah, with that change it can work, but need to check that all hooks will work properly

TFLint can't find TF modules by relative source without pushd

repos: - repo: https://github.com/thomaslast/pre-commit-terraform rev: afde258978090e4d2382fd90bb230347d0b349ac

✅ Work as expected
☑️ - Able to check only passed, not fail scenario
✖️ - work, but not print full path
❌ - failed to start
Not checked

✅ checkov (python, not affected hook)
✅ infracost_breakdown
✅ terraform_docs_replace (deprecated, python, not affected hook)
✖️ terraform_docs_without_aggregate_type_defaults
✖️ terraform_docs
✅ terraform_fmt
❌ terraform_providers_lock
❌ terraform_tflint
✅ terraform_tfsec
☑️ terraform_validate
✖️ terragrunt_fmt
☑️ terragrunt_validate
❌ terrascan

MaxymVlasov · 2022-04-13T19:46:11Z

Hmm, it does not print changed files at all, interesting

➜ pre-commit run
Terraform docs...........................................................Failed
- hook id: terraform_docs
- files were modified by this hook

repos:
- repo: https://github.com/thomaslast/pre-commit-terraform
  rev: afde258978090e4d2382fd90bb230347d0b349ac
  hooks:
    - id: terraform_docs

thomaslast · 2022-04-27T10:17:19Z

Sorry I've gone AWOL on this, thanks @MaxymVlasov and @yermulnik for your help. This seems to be a breaking change for something that could be quite simple. Is it worth making another version of common::per_dir_hook which will not use pushd & popd that I make use of just for tflint for the moment...?

MaxymVlasov · 2022-04-27T15:29:34Z

common::
just for tflint

it's called uncommon :)

We need a solution that will be DRY, easy to maintain, and fix #54 at the same time.

for the moment

Here is no place for a "quick fix", because who will do as it should be after you got what you ask?

github-actions · 2022-05-28T00:15:21Z

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

github-actions · 2022-06-08T00:13:11Z

This PR was automatically closed because of stale in 10 days

Fix attempt for tflint issue

784fe49

thomaslast requested review from MaxymVlasov and yermulnik as code owners April 7, 2022 13:54

thomaslast changed the title ~~Fix attempt for tflint issue~~ fix: print out the full path in tflint when run through pre-commit Apr 7, 2022

thomaslast changed the title ~~fix: print out the full path in tflint when run through pre-commit~~ fix: Print out the full path in tflint when run through pre-commit Apr 7, 2022

yermulnik reviewed Apr 7, 2022

View reviewed changes

Review comment

b35425d

thomaslast requested a review from yermulnik April 7, 2022 14:54

Add in the dir_paths to the calls

89bca81

yermulnik reviewed Apr 11, 2022

View reviewed changes

MaxymVlasov requested changes Apr 13, 2022

View reviewed changes

MaxymVlasov reviewed Apr 13, 2022

View reviewed changes

Fix terraform_fmt

afde258

MaxymVlasov marked this pull request as draft April 27, 2022 16:12

github-actions bot added the stale Denotes an issue or PR has remained open with no activity and has become stale. label May 28, 2022

github-actions bot closed this Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Print out the full path in tflint when run through pre-commit #357

fix: Print out the full path in tflint when run through pre-commit #357

thomaslast commented Apr 7, 2022 •

edited by MaxymVlasov

Loading

yermulnik Apr 7, 2022

thomaslast Apr 7, 2022

yermulnik Apr 7, 2022

thomaslast Apr 8, 2022

yermulnik left a comment

yermulnik Apr 11, 2022

yermulnik Apr 11, 2022

yermulnik Apr 11, 2022

MaxymVlasov left a comment

MaxymVlasov Apr 13, 2022

MaxymVlasov Apr 13, 2022

MaxymVlasov Apr 13, 2022

MaxymVlasov Apr 13, 2022 •

edited

Loading

MaxymVlasov commented Apr 13, 2022 •

edited

Loading

thomaslast commented Apr 27, 2022 •

edited

Loading

MaxymVlasov commented Apr 27, 2022

github-actions bot commented May 28, 2022

github-actions bot commented Jun 8, 2022

fix: Print out the full path in tflint when run through pre-commit #357

fix: Print out the full path in tflint when run through pre-commit #357

Conversation

thomaslast commented Apr 7, 2022 • edited by MaxymVlasov Loading

Description of your changes

How can we test changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yermulnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxymVlasov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxymVlasov Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

MaxymVlasov commented Apr 13, 2022 • edited Loading

thomaslast commented Apr 27, 2022 • edited Loading

MaxymVlasov commented Apr 27, 2022

github-actions bot commented May 28, 2022

github-actions bot commented Jun 8, 2022

thomaslast commented Apr 7, 2022 •

edited by MaxymVlasov

Loading

MaxymVlasov Apr 13, 2022 •

edited

Loading

MaxymVlasov commented Apr 13, 2022 •

edited

Loading

thomaslast commented Apr 27, 2022 •

edited

Loading