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

Jump to bottom

feat: TFLint: Add --hook-config=--delegate-chdir to use tflint -chdir #512

Merged
merged 4 commits into from
May 8, 2023
Merged

feat: TFLint: Add --hook-config=--delegate-chdir to use tflint -chdir #512

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da201ba
feat: hook-config optionally delegate chdir push/pop
lexton Apr 28, 2023
0725e70
fix delegate chdir call
lexton Apr 28, 2023
7e5bda4
Apply suggestions from code review
MaxymVlasov May 2, 2023
5dc58c4
Make chdir extendible and explicitly provide it inside function
MaxymVlasov May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,14 @@ To replicate functionality in `terraform_docs` hook:
- --args=--config=__GIT_WORKING_DIR__/.tflint.hcl
```

3. By default pre-commit-terraform performs directory switching into the terraform modules for you. If you want to delgate the directory changing to the binary - this will allow tflint to determine the full paths for error/warning messages, rather than just module relative paths. *Note: this requires `tflint>=0.44.0`.* For example:

```yaml
- id: terraform_tflint
args:
- --hook-config=--delegate-chdir
```


### terraform_tfsec

Expand Down
29 changes: 27 additions & 2 deletions hooks/_common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,25 @@ function common::per_dir_hook {
((index += 1))
done

# Lookup hook-config for modifiers that impact common behavior
local DELEGATE_CHDIR=false
IFS=";" read -r -a configs <<< "${HOOK_CONFIG[*]}"
for c in "${configs[@]}"; do
IFS="=" read -r -a config <<< "$c"
key=${config[0]}
value=${config[1]}

case $key in
--delegate-chdir)
# this flag will skip pushing and popping directories
# delegating the responsibility to the hooked plugin/binary
if [[ ! $value || $value == true ]]; then
yermulnik marked this conversation as resolved.
Show resolved Hide resolved
DELEGATE_CHDIR=true
MaxymVlasov marked this conversation as resolved.
Show resolved Hide resolved
fi
;;
esac
done
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that it makes common::per_dir_hook more complex.
I thinking about moving it to a separate function but I don't see how it could be done, at least, without introducing a new global env.

The main problem here is pushd/popd IF's,

 if [[ $DELEGATE_CHDIR != true ]]; then
      pushd "$dir_path" > /dev/null || continue
    fi

which, in current realization (internal variable naming?) looks too "tflint-specific"

I suppose CHANGE_DIR_INSIDE_HOOK than DELEGATE_CHDIR will be more universal and easier to understand

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or at least, I have those feelings. Not sure how they could be properly described and, which more importantly, done via code.

Let me think about it for a few days (and test how that functional could works with other hooks), maybe then I can describe it better

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the delegate wording. If DELEGATE_CHDIR is too short for clear understanding, then DELEGATE_CHDIR_TO_HOOK should suffice imho.

Copy link
Collaborator

@yermulnik yermulnik May 3, 2023 •
edited

Choose a reason for hiding this comment

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

I thinking about moving it to a separate function but I don't see how it could be done, at least, without introducing a new global env.

Parse HOOK_CONFIG array outside of the function and define this function with or w/o pushd/popd things based on presence or absence of --delegate-chdir hook arg and its value 😜
This may be expanded later for alike use cases. Though this would add complexity to the code anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd leave this as is (the way it's coded by @lexton in this PR) as a one-off/unique instance, at least until we have more of this kind requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its relatively well encapsulated as is.

If you wanted to change/extend this in the future it wouldn't be too hard to update/change variable wording without major impact.

The biggest thing that would be hard to change would be the exact flag name. --delegate-chdir if you wanted to make this more future proof that makes sense to get correct right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The biggest thing that would be hard to change would be the exact flag name. --delegate-chdir if you wanted to make this more future proof that makes sense to get correct right now.

My opinion for the flag is the same as for the var naming. JFYI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I double-checked #508 and this functional does not help avoid the mentioned issue, so, I'll make one tiny improvement and we are ready to go


# preserve errexit status
shopt -qo errexit && ERREXIT_IS_SET=true
# allow hook to continue if exit_code is greater than 0
Expand All @@ -226,7 +245,10 @@ 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

if [[ $DELEGATE_CHDIR != true ]]; then
pushd "$dir_path" > /dev/null || continue
fi

per_dir_hook_unique_part "$dir_path" "${args[@]}"

Expand All @@ -235,7 +257,10 @@ function common::per_dir_hook {
final_exit_code=$exit_code
fi

popd > /dev/null
if [[ $DELEGATE_CHDIR != true ]]; then
popd > /dev/null
fi

done

# restore errexit if it was set before the "for" loop
Expand Down
11 changes: 8 additions & 3 deletions hooks/terraform_tflint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function main {
} || {
local exit_code=$?
common::colorify "red" "Command 'tflint --init' failed:"
echo "${TFLINT_INIT}"
echo -e "${TFLINT_INIT}"
yermulnik marked this conversation as resolved.
Show resolved Hide resolved
return ${exit_code}
}

Expand All @@ -50,12 +50,17 @@ function per_dir_hook_unique_part {
shift
local -a -r args=("$@")

TFLINT_OUTPUT=$(tflint "${args[@]}" 2>&1)
if [[ $DELEGATE_CHDIR == true ]]; then
local dir_args="--chdir=$dir_path"
fi

# shellcheck disable=SC2086 # we need to remove the arg if its unset
TFLINT_OUTPUT=$(tflint ${dir_args:-} "${args[@]}" 2>&1)
local exit_code=$?

if [ $exit_code -ne 0 ]; then
common::colorify "yellow" "TFLint in $dir_path/:"
echo "$TFLINT_OUTPUT"
echo -e "$TFLINT_OUTPUT"
fi

# return exit code to common::per_dir_hook
Expand Down