feat: New Python-based terraform_fmt hook to better support Windows

[ericfrederich]

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

Fixes #648

How can we test changes

From a windows PowerShell or cmd.exe (i.e. outside of Git Bash, or any other Bash) in a repo with a terraform_fmt hook, run pre-commit run -a and it will fail with something like /bin/bash: .... : No such file or directory.

Change the hook from terraform_fmt to terraform_fmt_v2 and it'll work.

I've tested on Linux and Windows. I've made formatting changes to terraform code and verified that it updates the files in place properly. I have verified that with --args=-check the files are not updated and the hook fails as expected while showing on stdout the offending files.

[MaxymVlasov]

What .pre-commit-config.yaml configuration do you recommend to test it on the current iteration?

repos:
- repo: https://github.com/antonbabenko/pre-commit-terraform
  rev: e12fae3faeaf2ffcf65b1d08af91d7b64d349b31
  hooks:
    - id: terraform_fmt_v2

This one?

[ericfrederich]

This one?

yes, that one works for me on Windows.

[ericfrederich]

Up for discussion is the naming of this hook.

Should we _v2 this new one, or should we _legacy the old one?.... or not have 2 of them and simply replace it?

[yermulnik]

Overall looks neat 👍🏻

For the sake of visual comparison, could you please attach:

• screenshots of the output of both bash and python hooks running on the same code and formatting it
• timings for the above (it makes sense to run this over several dirs with a dozen files in each that need formatting).

Thanks

[yermulnik]

Should we _v2 this new one, or should we _legacy the old one?.... or not have 2 of them and simply replace it?

I'd keep the new hook with _py suffix during the solution evaluation period, and replace old one with new one if we decide to go with Py version.

It would also be great if you have doc-strings added to each function. Thanks in advance.

[MaxymVlasov]

➜ time pre-commit run -a
[INFO] Installing environment for https://github.com/antonbabenko/pre-commit-terraform.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/python3', '/home/vm/.cache/pre-commit-zipapp/foYifN81FLsOKwa_AbWJjbHnCx9JWWVug44854ojl8o/python', '-mvirtualenv', '/home/vm/.cache/pre-commit/repow3wud7r1/py_env-python3.10')
return code: 1
stdout: (none)
stderr:
    Traceback (most recent call last):
      File "/home/vm/.cache/pre-commit-zipapp/foYifN81FLsOKwa_AbWJjbHnCx9JWWVug44854ojl8o/python", line 47, in <module>
        raise SystemExit(main())
      File "/home/vm/.cache/pre-commit-zipapp/foYifN81FLsOKwa_AbWJjbHnCx9JWWVug44854ojl8o/python", line 34, in main
        runpy.run_module(args.m, run_name='__main__', alter_sys=True)
      File "/usr/lib/python3.10/runpy.py", line 220, in run_module
        mod_name, mod_spec, code = _get_module_details(mod_name)
      File "/usr/lib/python3.10/runpy.py", line 140, in _get_module_details
        raise error("No module named %s" % mod_name)
    ImportError: No module named virtualenv
Check the log at /home/vm/.cache/pre-commit/pre-commit.log
pre-commit run -a  0,41s user 0,09s system 105% cpu 0,473 total

Looks like it includes external dependencies which not comes with pre-commit

Or does it not work with Python 3.10, I dunno

✘3 ➜ virtualenv
usage: virtualenv [--version] [--with-traceback] [-v | -q] [--read-only-app-data] [--app-data APP_DATA] [--reset-app-data] [--upgrade-embed-wheels] [--discovery {builtin}] [-p py] [--try-first-with py_exe]
                  [--creator {builtin,cpython3-posix,venv}] [--seeder {app-data,pip}] [--no-seed] [--activators comma_sep_list] [--clear] [--no-vcs-ignore] [--system-site-packages] [--symlinks | --copies] [--no-download | --download]
                  [--extra-search-dir d [d ...]] [--pip version] [--setuptools version] [--wheel version] [--no-pip] [--no-setuptools] [--no-wheel] [--no-periodic-update] [--symlink-app-data] [--prompt prompt] [-h]
                  dest
virtualenv: error: the following arguments are required: dest
SystemExit: 2

I suppose that we should support everything starting at least from Python 3.8.2, to support Ubuntu 20.04 (Standard EOL - April 2025, Extended EOL - April 2030)
If we'd like to support 18.04 (Standard EOL - June 2023, Extended EOL - April 2030) - Python 3.6.8

https://wiki.ubuntu.com/Releases

Or we should pack somehow Python update/venv based on pre-installed tools

[ericfrederich]

I suppose that we should support everything starting at least from Python 3.8.2, to support Ubuntu 20.04 (Standard EOL - April 2025, Extended EOL - April 2030) If we'd like to support 18.04 (Standard EOL - June 2023, Extended EOL - April 2030) - Python 3.6.8

https://wiki.ubuntu.com/Releases

Or we should pack somehow Python update/venv based on pre-installed tools

I'd recommend supporting whatever the current version of pre-commit itself supports (currently >=3.9)

I don't know why you've got an error.

I run with this which is the current commit on my PR branch:

repos:
- repo: https://github.com/ericfrederich/pre-commit-terraform.git
  rev: c78797feb5463a975a19b69dbd3624b9219fe0ab
  hooks:
  - id: terraform_fmt_py
    args:
      - --env-vars=FOO="bar"
      - --env-vars=SPAM=eggs

[ericfrederich]

I believe all review items are now addressed.

I think the error @MaxymVlasov had is unrelated to this PR as it was complaining about virtualenv which is what pre-commit uses to create isolated environments for various Python based hooks.

I have a feeling there would also be an issue running hooks from the pre-commit-hooks repo on that same system as well.

[MaxymVlasov]

I have a feeling there would also be an issue running hooks from the pre-commit-hooks repo on that same system as well.

Have no problem with them, only with this Python hook
Nwm, that was an issue with pre-commit installation via asdf

[MaxymVlasov]

I don't get how it is possible, but this hook flies like a rocket compared to bash-one. 🔥

It makes sense to reimplement all these as Python. But please, @ericfrederich add tests. Pytest whatever, currently it's a pain in the ass to try to implement anything without breaking stuff... #303

And these checks.
If you find some checks too strict or outdated - please let me know, we definitely can adjust them

[MaxymVlasov]

Let's start a PR scope thread here:

How much do we want to be implemented in this PR?

1.1. Current common functional "as is"
1.2. Full common functional parity

2.1. Just terraform_fmt
2.2. All hooks fully which fully based on common functional

Choose one from 1. and 2.
The current state is 1.1. + 2.1.

[yermulnik]

1.1 + 2.1 (+ each hook to be re-implemented with Python in scope of a separate PR)

ps: I don't get the difference between 1.1 and 1.2 to be honest... 🤷🏻

[MaxymVlasov]

1.2 Means that

https://github.com/ericfrederich/pre-commit-terraform/blob/18291bd1e3105046592d221426704c2418c34b7c/hooks/common.py#L55-L59

should be implemented too in this PR

[yermulnik]

I don't get how it is possible, but this hook flies like a rocket compared to bash-one. 🔥

What's the approximate difference?
If the diff is really notable, then we might just want to find what's super slow in the bash implementation. Even adding time to each command may surface the culprit I hope.

[ericfrederich]

What's the approximate difference?

Logic

Python version

pre-commit itself is already sending over batches of files to the hook. The python version simply calls terraform fmt on the files given to it by pre-commit. So each .tf file that should be formatted is formatted exactly once (and none of the excluded ones). This is simple and fast.

Bash version

The bash version from what I can tell is using common::per_dir_hook.

From what I understand, this is taking the same list of files from pre-commit, but then doing computation to figure out unique directories, and running terraform fmt against each directory.

So I think if given a large directory of .tf files, say (infra/foo_000.tf, infra/foo_001.tf, ... infra/foo_999.tf)... if pre-commit splits this up into 50 chunks of 20 files the per_dir_hook logic ends up running it against all 1,000 files 50 times?

side note: In addition to performance implications, I believe it is in fact a bug because it'll end up runningterraform fmt against files which are explicitly excluded in your config file. I reported this 4 years ago: #121

Architecture

Disclosure: I have not verified this.

Pre-commit is written in Python, so when it uses a Python based hook it may be done without invoking another subprocess (but I could be wrong)
I think this would be negligible anyway and the logic difference is likely where most the performance difference is coming from.

Python version

pre-commit process is a Python process which runs the hook within the same process which calls terraform as a subprocess.

Bash version

pre-commit is a Python process which runs a bash script as a subprocess which runs terraform as a subprocess.

[yermulnik]

Does this mean, that if we re-work existing shell-hook to run only on changed files, then we will rectify the "slowness" of the shell-hook but introduce behavioral change that Py-based hook implements by default? The fmt all files in each changed dir vs check only changed files. If yes, then @MaxymVlasov why do we need the common::per_dir_hook feature? I'm sure it was added on purpose... 🤔

[MaxymVlasov]

Hmm, I am not sure that it changes only modified files, need to test it. Otherwise, I don't get how it was able to run on pre-commit run --all-files

All this "per-dir" stuff was introduced to avoid 3-8 times the same runs in one dir by removing filenames and then deduplication of patches.

We need to test it for sure. Ideally, by pytest (or similar), not manually

[ericfrederich]

There should be no code that runs against an entire directory. It should only run against the files given to it by pre-commit itself as pre-commit honors the exclude section of the config.

Here's a repo which demonstrates the example: https://github.com/ericfrederich/pct-bug-example

Just follow the README.

I think if you fix the bug (i.e. remove all the unique directory stuff), you'll get the performance for free.

[yermulnik]

There should be no code that runs against an entire directory. It should only run against the files given to it by pre-commit itself […]

I do agree. Though... though there's a trade off between running terraform fmt once per dir with 250 files and running 250 times for each file in that dir if all of them were updated.
I suppose that the common::per_dir_hook was implemented since most of the time (and this is important) the users wanted to run it for all files in the dir to ensure the entire code layer was formatted properly. Which is, well, a drawback in speed and time if there are too many files in the dir, which is not the most cases I suspect (I have no knowledge on why that function was implemented and hence I'm guesstimating sort of "if that exists, it probably was requested").
It's probably worth to re-visit the logic (if it indeed is like that) to provide an option for users to choose between the two approaches.

Here's a repo which demonstrates the example: ericfrederich/pct-bug-example

That example demonstrates not a bug, but an option, which you think isn't worth to exist and which (if it indeed exists) you simply cut off without backward compatibility in the mind.

ps: I might be completely messing up things, though I don't believe that the spoken drastic speedup with Py-based implementation is related to the language switch. It most probably is related to the change in logic. Again: I might be wrong, though I have no playground to make different tests to prove or to disprove 🤷🏻

[yermulnik]

You will notice that even though ec2.tf should be skipped terraform fmt was definitely ran against it.

This is indeed a bug that we need to rectify: if the ec2.tf file was the only change in that dir (and it was in the list of pre-commit exclusions), the tf fmt hook must have not got triggered for sure.
Is that worth of rewriting the hook in Python? I'm not entirely sure to be honest, because we can't say for sure that our users prefer Python over Bash when they want to contribute.
From my point of view the pre-commit-terraform initially was oriented to Ops rather than Dev. Maybe it's time to re-consider this point? 🤔

[ericfrederich]

I do agree. Though... though there's a trade off between running terraform fmt once per dir with 250 files and running 250 times for each file in that dir if all of them were updated.

Maybe I don't understand this statement or perhaps you don't understand the way the Python code is working. The python version does not call terraform fmt once per file, it would never call it 250 times for 250 files. In fact it only ever calls terraform fmt once per invocation. If there is a big enough list of files, then pre-commit will split them and invoke the hook multiple times, but the Python hook calls terraform just once. There is no loops in the code. It's very direct, simple, and correct.

This all works because terraform fmt itself already accepts a list of files, this hook simply passes them from pre-commit to terraform.

It could be perhaps that terraform didn't always support multiple arguments. Perhaps it would either accept a single file or a single directory. Perhaps that was the reason these were originally written the way they are.

I suppose that the common::per_dir_hook was implemented since most of the time (and this is important) the users wanted to run it for all files in the dir to ensure the entire code layer was formatted properly.

I apologize, but if a user wants to format an entire directory they can simply call terraform fmt path/to/some/directory. This behavior should be outside the scope of these hooks and pre-commit in general.

In all cases a pre-commit hook should only run on the files which pre-commit determines it should run against... nothing more, nothing less. Anything else is a bug.

If a user wants to run it against every non-excluded file in the entire project, they can run pre-commit run -a.

Is that worth of rewriting the hook in Python?

Let's not conflate all these different topics. The only reason to rewrite into Python is to support environments which don't have Bash (Windows cmd.exe, Power Shell, PyCharm on Windows, VSCode, etc). The fact that the Python version is also more performant and doesn't re-create a bug is beside the point.

[yermulnik]

perhaps you don't understand the way the Python code is working.

Did I miss a detailed description of the changes this PR brings or you just didn't bother to provide it and let others dive into details on their own? 🤔

In fact it only ever calls terraform fmt once per invocation.

So it never passes a chdir to TF and hence TF is run from a repo root (?) and hence presumably it never takes into account anything that existing hook would, right?

There is no loops in the code. It's very direct, simple, and correct.

Correct from which point of view: your or those who already rely upon existing logic?

I suppose that the common::per_dir_hook was implemented since most of the time (and this is important) the users wanted to run it for all files in the dir to ensure the entire code layer was formatted properly.

I apologize, but if a user wants to format an entire directory they can simply call terraform fmt path/to/some/directory. This behavior should be outside the scope of these hooks and pre-commit in general.

I see your point, though your reasoning is, well, biased and opinionated.
You've come saying sorta you wanted to improve existing hook by re-writing it in Python. Though eventually you introduced "quite another but kind of similar hook".
Let's put things straight: you're not replacing existing functionality, you're adding new behavior. I'm not against what you're doing. I'm all for the hooks to run solely against only changed files. But... but it's thousands of users and automations across the world that already rely upon existing logic, which you want to cut off and throw away just because you don't like it.

Is that worth of rewriting the hook in Python?

Let's not conflate all these different topics. The only reason to rewrite into Python is to support environments which don't have Bash (Windows cmd.exe, Power Shell, PyCharm on Windows, VSCode, etc). The fact that the Python version is also more performant and doesn't re-create a bug is beside the point.

Okay, let's not conflate and refrain from naming a hook with different implementation and different behavior a "version" of an existing one.
I agree that some systems don't have Bash, though they also may be missing all the other "external" tools (the prerequisites) that are the gist of pre-commit-terraform — following your words, these tools should be re-written in Python too.
What you call "a bug" — and I repeat it again and again — is an existing functionality which most probably was requested and which most probably is relied upon. And I'm against cutting it off just like that.
So what I actually am suggesting is we add (vs replace) this re-implementation of fmt hook to the pool and give our users an option to choose between existing functionality and the speed-wise hook with a bit different behavior.
Later on we can try and either fix the shell-based hook or decommission it in favour of the Py-based one (which is still not finalized as I can see) or, well, keep it to support existing behavior and let users choose speed vs "bells-and-whistles" (like identifying issues with TF configuration in the specific directory with TF files).

[ericfrederich]

Did I miss a detailed description of the changes this PR brings or you just didn't bother to provide it and let others dive into details on their own? 🤔

No detailed description needed as it's not over-complicated. It simply calls terraform fmt on the files that pre-commit tells it to. Period the end.

Okay, let's not conflate and refrain from naming a hook with different implementation and different behavior a "version" of an existing one.

I named it as was suggested, I asked how it should be handled and followed instructions.

It's currently suffixed with _py but feel free to change it to _faster_and_without_bugs.

---

I just need a way to run terraform fmt via pre-commit on Windows.

I'll use this which:

• is fast (doesn't process the same file multiple times like the Bash version does)
• is correct (doesn't process files explicitly excluded in the config like the Bash version does)
• works on non-bash environments like Windows (which the Bash version does not)
• can be placed directly in the repo using it without referring to this repo or any other one.

- repo: local
  hooks:
  - id: terraform_fmt
    name: Terraform fmt
    description: Rewrites all Terraform configuration files to a canonical format.
    entry: terraform fmt
    language: system
    files: (\.tf|\.tfvars)$
    exclude: \.terraform/.*$

In the future if I need some of the advanced features provided by this repo like --env-vars I'll check back and see if this has been merged.

I tried to help add Windows support, but I'm ready to move on.

[yermulnik]

I tried to help add Windows support

Appreciate your intent, time and effort, though this way of collaboration and implementation "user-wise" isn't what I personally am keen to be part of in this project.
I'll leave it to @MaxymVlasov to decide whether to proceed with this piece of opinionated work, whilst I will be happy to help re-visiting existing hook to see whether it's safe to re-implement current logic to gain speed altogether with keeping existing functionality to the extent necessary.

I guess If we onboard more Pythonists, who are open for collab, are user-oriented and are long-term project support oriented, we may look into re-implementing existing hooks in Python to provide more convenience for Windows users (as opposed to easier maintenance by vast majority of Windows users as well).

[ericfrederich]

For what it's worth, on the project I need Windows support on with just 113 .tf files I switched to the pre-commit-native solution via language: system.

It used to take 2.41 seconds, now it takes 0.241.

It was never about Python or performance though. It was about adding Windows support.

[yermulnik]

It was never about Python or performance though. It was about adding Windows support.

I absolutely understand your point. My point is of another kind.
I personally will be sincerely glad to see your hook added (under some conditions though 🤷🏻). And I already said it:

I guess If we onboard more Pythonists, who are open for collab, are user-oriented and are long-term project support oriented, we may look into re-implementing existing hooks in Python to provide more convenience for Windows users (as opposed to easier maintenance by vast majority of Windows users as well).

I also would like to emphasize once more: this is my very own opinion and I'm not the person in charge.

[MaxymVlasov]

I finally read everything that you discussed here :)

Okay, my opinion: If we can make something in Python and it will work faster than in bash - let's do it in Python.
For me, there is no major difference in whether Windows is supported or not (I'm okay with dropping Mac support too, tbh :), but performance, ability to use normal functions and tests, and many more restrictive linters on top of it - make a major difference for me.

I see that this PR little-bit stale, so I'll take care of it.

[yermulnik]

If we can make something in Python and it will work faster than in bash

There's no evidence (there's no even info on the comparison of execution speed) and hence I believe the Py-based hook is just doing something differently and hence with a different result (or there's a bug in Bash-based hook which we may need to rectify because it may also impact other hooks).

[…] - make a major difference for me.

While I'm not opposing to switch to Python (why not Golang then for example? 🤪), I'd like to suggest to consider thoroughly all the benefits and drawbacks before making such decision.
I see a bunch of benefits, though I also see some drawbacks at a quick glance like a way bigger sprawl between Py versions and hence we'll have to support at least all of the non-EOL'ed versions and will have to conclude a decision to drop support for older versions and users (given we still support Bash 3, this decision may look a bit rough for pre-commit-terraform users in spite of comparison to Bash 3).
There may be other drawbacks and hitches though and this has to be considered from all angles I guess.

so I'll take care of it.

I'm all for this and will try and help reviewing 🤝

[ericfrederich]

Yes Bash version does a bug which also impacts performance.
It incorrectly ignores runs against directories instead of the files fed into it by pre-commit.
I've explained it before and even created a repo to demonstrate the bug.
The performance is also easily explained because the Bash bug causes entire directories to be formatted multiple times when pre-commit itself creates batches and invokes the hook multiple times.

There is evidence.

Yes Py-based hook is doing something differently... It's not re-implementing the bug present in the Bash version. It's really that simple. I don't know how to explain it differently.

[yermulnik]

This had been already discussed and you keep repeating yourself without listening to other person.

[yermulnik]

If @MaxymVlasov is up to pick up on getting the job done, we can add this hook as a different one (since it implements different behavior) and let users switch step by step if they decide for speed vs regular, sort of conventional behavior.
Additionally we can also take a look into implementing the same into existing hook as an option and — again — let users decide to switch or keep things as they used to be.
IMHO.

[ericfrederich]

different behavior

since it implements different behavior

We're in agreement that they implement different behavior. The behavior of the Python version is that it's bug free and performs well. The behavior of the Bash one is the opposite. This is though no fault of Bash itself, just the over-engineered buggy implementation. This isn't a language war, I could easily write one just as bad in Python.

Other than the Python one being bug-free and faster, can you please articulate the difference?

conventional behavior

let users switch step by step if they decide for speed vs regular, sort of conventional behavior

Is "conventional behavior" that if a user explicitly tells pre-commit in their config to exclude a file... that this Bash hook still formats the file anyway ignoring the user's wishes?

Is "conventional behavior" that you end up formatting the same file 20 times causing performance issues?

This is where there needs to be agreement:

• files explicitly excluded by the user in their config should not be touched.
• processing only the files that pre-commit passes into the hook is the only way to properly implement a hook.
• files should only be processed once per hook.

Do you disagree with any of those 3 points?

[yermulnik]

So you keep repeating yourself over and over again without listening to other person.

Let me cite myself (please read thoroughly the statement inside parenthesis at the end of the paragraph):

I suppose that the common::per_dir_hook was implemented since most of the time (and this is important) the users wanted to run it for all files in the dir to ensure the entire code layer was formatted properly. Which is, well, a drawback in speed and time if there are too many files in the dir, which is not the most cases I suspect (I have no knowledge on why that function was implemented and hence I'm guesstimating sort of "if that exists, it probably was requested").

Re the "conventional" term since we're taking this differently in this context: I mean traditional, established behavior that is relied upon by lots of users already. Maybe not because they wanted it, but rather because they used to it. Who knows... You've spoken out. Others — haven't. We can't judge for all of them based on your mileage only.
Your take is "switch behavior" (while I do agree that this actually along the way rips off a bug from current hook's implementation, but still switches the behavior). My take is "add this new hook as an option (and let users choose whichever is best for them) and fix the bug in existing hook".

PS: Apart from this fascinating conversation, do you have any plans on completing the work on this PR to make it fit the ecosystem?

[ericfrederich]

I read your citation the first time and even quoted it and responded to it.

If a user wants to run it against every non-excluded file in the entire project, they can run pre-commit run -a.

"supposing" or "guestimating" why something was implemented doesn't make it true. It also doesn't matter because even if it were true it just means the user requesting it was wrong and it was a mistake to implement the behavior.

There already exist ways for users to run against everything in a directory that doesn't involve pre-commit.

I had "supposed" and "guestimated" that the common::per_dir_hook was implemented for performance reasons years ago when the terraform command itself didn't support multiple files being passed to it.

In the end it doesn't matter whose supposition is correct (maybe neither). The fact remains it's still the wrong thing to do in 2024.

PS: Apart from this fascinating conversation, do you have any plans on completing the work on this PR to make it fit the ecosystem?

What is left to complete? I see 7 resolved threads.

[yermulnik]

"supposing" or "guestimating" why something was implemented doesn't make it true.

We're looking at this from different points of view. I already explained that:

Note

Your take is "switch behavior" (while I do agree that this actually along the way rips off a bug from current hook's implementation, but still switches the behavior). My take is "add this new hook as an option (and let users choose whichever is best for them) and fix the bug in existing hook".

I had "supposed" and "guestimated" that the common::per_dir_hook was implemented for performance reasons years ago when the terraform command itself didn't support multiple files being passed to it.

Maybe. TBH I don't understand what you're arguing for. What's your goal?

PS: Apart from this fascinating conversation, do you have any plans on completing the work on this PR to make it fit the ecosystem?

What is left to complete? I see 7 resolved threads.

I see two TODO's.

[ericfrederich]

I fixed the mypy and pylint issues. But now on GitHub I don't see the results any more... maybe only once it's approved it'll run the checks again?

In any case, the pylint stuff was over-aggressive and complaining about existing code which is not part of this PR.