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.

Sign up for GitHub

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: Print out the full path in tflint when run through pre-commit #357

Closed
wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
784fe49
Fix attempt for tflint issue
Apr 7, 2022
b35425d
Review comment
Apr 7, 2022
89bca81
Add in the dir_paths to the calls
Apr 8, 2022
afde258
Fix terraform_fmt
MaxymVlasov Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions hooks/_common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. the [ -f "$dir_path" ] || continue should be [ ! -d "$dir_path" ] && continue
  2. 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.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Fair enough I can change
  2. 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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


per_dir_hook_unique_part "$args" "$dir_path"

Expand All @@ -147,7 +147,6 @@ function common::per_dir_hook {
final_exit_code=$exit_code
fi

popd > /dev/null
done

# restore errexit if it was set before the "for" loop
Expand Down