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

[lexton]

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]

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]

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]

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.

[lexton]

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

[MaxymVlasov]

And merge master branch too, please

[MaxymVlasov]

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

[lexton]

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.

[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]

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]

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 🥳

[MaxymVlasov]

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

[MaxymVlasov]

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

[yermulnik]

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

[yermulnik]

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.

[yermulnik]

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.

[lexton]

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.

[yermulnik]

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.

[MaxymVlasov]

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]

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

[antonbabenko]

This PR is included in version 1.79.0 🎉