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

lexton · 2023-04-26T13:36:47Z

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 change leverages the tflint -chdir flag to do the nested lookup and provide fully qualified paths to the output.

It unwraps the pushd/popd functionality and delegates the directory changing to the binary process via chdir.

This would be a breaking behavior if a user tried to add a -chdir flag to the args - but I can't think of a valid reason that anyone would do this - or if that functionality would even work with the pre-commit-terraform hooks.

The -chdir flag was added in tflint 0.44.0 - I don't know how to enforce specific versions of the binary or to trivially change the commit hook driver behavior based upon the version. This flag is forwards compatible, while the dir arg to tflint is being deprecated.

Totally happy to revisit the approach or just keep maintaining a fork for my use cases.

Fixes #511

How can we test changes

Locally examine the output when the tflint succeeds vs fails.

MaxymVlasov · 2023-04-27T14:53:37Z

Hmm, that's an interesting solution. I definitely like that you find a way to fix that issue, but not sure about realization and our ability to merge it just as a minor release.

Can you please run some performance tests for your realization and for tag 1.77.3?
https://github.com/antonbabenko/pre-commit-terraform/blob/master/.github/CONTRIBUTING.md#run-hook-performance-test

0.44.0 (2022-12-26) was added not so long time ago and that's the main issue from my perspective - many folks could not updated yet, and I don't like to force them to update outside of breaking change release.

So for now - how it when will be merged and released - as 1.x or part of 2.0 - it's a question.

Maybe I need to find some time to resolve #303 and then I'll be able to move some other hooks to git repo root too, as was asked in #508. With or without breaking changes - will see.

yermulnik · 2023-04-27T19:21:37Z

Hmm, that's an interesting solution. I definitely like that you find a way to fix that issue, but not sure about realization and our ability to merge it just as a minor release.

What is proposed in this PR contradicts with the designed essence of per_dir_hook_unique_part function and attempts to circumvent author's use case by implicitly manipulating dir stack in reverse order. So it's a bit of reversed copy&pasta from the parent function (also continue SHOULD NOT be used outside loops).
This doesn't seem to be a proper and reliable approach as it's more of a circumvention and a quirk. It also doesn't apply any version compatibility checks and hence is backward incompatible (for those who may need to use tflint =< 0.44.0 by whichever reason).

To properly resolve author's use case, parent common::per_dir_hook function should be updated to exclude dir manipulation (the pushd/popd thing) for those hook(s) which underlying tool is capable with managing dir stack on their own.
Along with that it would great to maintain backward compatibility for those who may need to use older versions of tflint IMHO.

lexton · 2023-04-28T05:07:03Z

Thanks for the feedback! This was a bit of a hack - just reversing the caller behavior. It seemed to work well enough for me locally - but see the problems you are describing.

I adjusted to add a hook-config that would bypass the pushd/popd behavior, and then switch on the --chdir flag for tflint.

Does this flag seem reasonable?

I still haven't invested in running the performance tests yet - but doesn't seem like anything is on the hot path. Does this seem ok as is? If so I can get myself setup to run them.

README.md

hooks/_common.sh

README.md

hooks/_common.sh

hooks/terraform_tflint.sh

lexton · 2023-04-28T16:06:53Z

Working on addressing these PR comments now - should have it re-pushed/rebased etc in a few mins.

MaxymVlasov · 2023-04-28T16:07:42Z

And merge master branch too, please

MaxymVlasov · 2023-04-28T16:09:48Z

And you don't need to do force-pushes at all - we will squash it when it will be merged

lexton · 2023-04-28T16:53:54Z

sorry - not super familiar with the best way to work with forked repos - mostly used internal monorepos. Will re-open in a min - I screwed up the rebase.

hooks/terraform_tflint.sh

hooks/_common.sh

yermulnik

@lexton Could you please test this PR once again and if wit works as expected and as intended for all of --delegate-chdir, --delegate-chdir=true, --delegate-chdir=false, and when --delegate-chdir isn't used at all, then I'll be happy to approve.
Thank you

lexton · 2023-04-28T19:33:57Z

Yep - just tested this locally. All these situations worked as expected.


Command 'tflint --init' successfully done:

TFLint in mod/:
1 issue(s) found:

variables.tf:1:1: Warning - variable "unused" is declared but not used (terraform_unused_declarations)

Interestingly when I ran it locally one of the surprising things - it seem to resolve symlinks - since we expect this to only ever be locally relative paths - maybe we just want the relative paths rather than the fully qualified symlink.

../../private/tmp/new/mod/variables.tf:1:1: Warning - variable "unused" is declared but not used (terraform_unused_declarations)

Specifically:
https://apple.stackexchange.com/questions/1043/why-is-tmp-a-symlink-to-private-tmp

yermulnik · 2023-04-28T19:47:39Z

Interestingly when I ran it locally one of the surprising things - it seem to resolve symlinks

This time this is for tflint not pre-commit-terraform 🤣 I'm going to approve PR now. @MaxymVlasov FYI.

yermulnik

@lexton Thank you for the contribution 🥳

hooks/terraform_tflint.sh

hooks/_common.sh

MaxymVlasov · 2023-05-03T14:26:24Z

hooks/_common.sh

+  # 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
+          DELEGATE_CHDIR=true
+        fi
+        ;;
+    esac
+  done


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

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

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

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.

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.

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.

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.

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

MaxymVlasov · 2023-05-08T14:43:55Z

Changes in commit above provide ability to extend different manipulations with dir path.

IE., we could add "--provide-full-path-from-repo-root" etc. which will work for #508 and at the same time, do not need to introduce any extra logic

# [1.79.0](v1.78.0...v1.79.0) (2023-05-08) ### Features * TFLint: Add `--hook-config=--delegate-chdir` to use `tflint -chdir` ([#512](#512)) ([1e9debc](1e9debc))

antonbabenko · 2023-05-08T15:32:54Z

This PR is included in version 1.79.0 🎉

lexton requested review from MaxymVlasov and yermulnik as code owners April 26, 2023 13:36

lexton changed the title ~~tflint: delegate path manipulation to tflint binary~~ fix: tflint: delegate path manipulation to tflint binary Apr 26, 2023

lexton changed the title ~~fix: tflint: delegate path manipulation to tflint binary~~ fix: TFLint: delegate path manipulation to tflint binary Apr 26, 2023

MaxymVlasov added feature New feature or request hook/terraform_tflint Bash hook on-hold Indicates that an issue or PR should not be auto-closed due to staleness. labels Apr 27, 2023

MaxymVlasov changed the title ~~fix: TFLint: delegate path manipulation to tflint binary~~ feat: TFLint: delegate path manipulation to tflint binary Apr 27, 2023

lexton force-pushed the master branch from a177972 to 3e96e3c Compare April 27, 2023 21:15

lexton force-pushed the master branch from 3e96e3c to 61256b5 Compare April 28, 2023 06:14

MaxymVlasov reviewed Apr 28, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

hooks/_common.sh Outdated Show resolved Hide resolved

yermulnik reviewed Apr 28, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

hooks/_common.sh Outdated Show resolved Hide resolved

hooks/_common.sh Outdated Show resolved Hide resolved

hooks/terraform_tflint.sh Outdated Show resolved Hide resolved

hooks/terraform_tflint.sh Outdated Show resolved Hide resolved

lexton closed this Apr 28, 2023

lexton force-pushed the master branch from 61256b5 to 4a0e56f Compare April 28, 2023 16:47

feat: hook-config optionally delegate chdir push/pop

da201ba

lexton reopened this Apr 28, 2023

fix delegate chdir call

0725e70

MaxymVlasov removed the on-hold Indicates that an issue or PR should not be auto-closed due to staleness. label Apr 28, 2023

lexton commented Apr 28, 2023

View reviewed changes

hooks/terraform_tflint.sh Show resolved Hide resolved

lexton commented Apr 28, 2023

View reviewed changes

hooks/_common.sh Show resolved Hide resolved

MaxymVlasov requested a review from yermulnik April 28, 2023 17:08

yermulnik reviewed Apr 28, 2023

View reviewed changes

yermulnik approved these changes Apr 28, 2023

View reviewed changes

MaxymVlasov reviewed May 2, 2023

View reviewed changes

hooks/terraform_tflint.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

7e5bda4

MaxymVlasov self-requested a review May 2, 2023 11:55

MaxymVlasov reviewed May 3, 2023

View reviewed changes

hooks/_common.sh Outdated Show resolved Hide resolved

MaxymVlasov reviewed May 3, 2023

View reviewed changes

Make chdir extendible and explicitly provide it inside function

5dc58c4

MaxymVlasov requested a review from yermulnik May 8, 2023 14:39

yermulnik approved these changes May 8, 2023

View reviewed changes

MaxymVlasov changed the title ~~feat: TFLint: delegate path manipulation to tflint binary~~ feat: TFLint: delegate path manipulation to tflint binary via --hook-config=--delegate-chdir May 8, 2023

MaxymVlasov changed the title ~~feat: TFLint: delegate path manipulation to tflint binary via --hook-config=--delegate-chdir~~ feat: TFLint: Add --hook-config=--delegate-chdir to use tflint -chdir May 8, 2023

MaxymVlasov merged commit 1e9debc into antonbabenko:master May 8, 2023

antonbabenko pushed a commit that referenced this pull request May 8, 2023

chore(release): version 1.79.0 [skip ci]

2e8e13f

# [1.79.0](v1.78.0...v1.79.0) (2023-05-08) ### Features * TFLint: Add `--hook-config=--delegate-chdir` to use `tflint -chdir` ([#512](#512)) ([1e9debc](1e9debc))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lexton commented Apr 26, 2023

MaxymVlasov commented Apr 27, 2023

yermulnik commented Apr 27, 2023 •

edited

Loading

lexton commented Apr 28, 2023 •

edited

Loading

lexton commented Apr 28, 2023

MaxymVlasov commented Apr 28, 2023

MaxymVlasov commented Apr 28, 2023

lexton commented Apr 28, 2023

yermulnik left a comment

lexton commented Apr 28, 2023 •

edited

Loading

yermulnik commented Apr 28, 2023

yermulnik left a comment

MaxymVlasov May 3, 2023

MaxymVlasov May 3, 2023 •

edited

Loading

yermulnik May 3, 2023

yermulnik May 3, 2023 •

edited

Loading

yermulnik May 3, 2023

lexton May 3, 2023

yermulnik May 3, 2023

MaxymVlasov May 8, 2023

MaxymVlasov commented May 8, 2023

antonbabenko commented May 8, 2023

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

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

Conversation

lexton commented Apr 26, 2023

Description of your changes

How can we test changes

MaxymVlasov commented Apr 27, 2023

yermulnik commented Apr 27, 2023 • edited Loading

lexton commented Apr 28, 2023 • edited Loading

lexton commented Apr 28, 2023

MaxymVlasov commented Apr 28, 2023

MaxymVlasov commented Apr 28, 2023

lexton commented Apr 28, 2023

yermulnik left a comment

Choose a reason for hiding this comment

lexton commented Apr 28, 2023 • edited Loading

yermulnik commented Apr 28, 2023

yermulnik left a comment

Choose a reason for hiding this comment

MaxymVlasov May 3, 2023

Choose a reason for hiding this comment

MaxymVlasov May 3, 2023 • edited Loading

Choose a reason for hiding this comment

yermulnik May 3, 2023

Choose a reason for hiding this comment

yermulnik May 3, 2023 • edited Loading

Choose a reason for hiding this comment

yermulnik May 3, 2023

Choose a reason for hiding this comment

lexton May 3, 2023

Choose a reason for hiding this comment

yermulnik May 3, 2023

Choose a reason for hiding this comment

MaxymVlasov May 8, 2023

Choose a reason for hiding this comment

MaxymVlasov commented May 8, 2023

antonbabenko commented May 8, 2023

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

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

yermulnik commented Apr 27, 2023 •

edited

Loading

lexton commented Apr 28, 2023 •

edited

Loading

lexton commented Apr 28, 2023 •

edited

Loading

MaxymVlasov May 3, 2023 •

edited

Loading

yermulnik May 3, 2023 •

edited

Loading