Windows Support / Rewrite hooks to Python

[ericfrederich]

What problem are you facing?

I cannot use pre-commit from Windows cmd.exe or PowerShell as it depends on bash. Only way is to use "Git Bash"

How could pre-commit-terraform help solve your problem?

I think the bash dependency should go away. Git is used on Windows outside of Git Bash all the time... either directly by developers using PowerShell or cmd.exe... or indirectly by other tools/IDEs, etc.

pre-commit itself is a Python script, so any environment where pre-commit is running necessarily already has Python available. I'd like to see those shell scripts converted to Python. It could use the subprocess module to invoke the Terraform binary.

I'm willing to do this work if you're open to it.

[yermulnik]

Yep, we're open for contributions. We welcome and encourage them. Especially those of such a brave kind 👍🏻 You may take the https://github.com/antonbabenko/pre-commit-terraform/blob/master/.github/CONTRIBUTING.md as a base point. While it is not quite Windows-aware I guess, it may probably help you with the direction.

[MaxymVlasov]

I'd like to see those shell scripts converted to Python

Well, if you'll provide at least the same execution speed as in the shell, we will check your PR. If it will work slower - from README:

We highly recommend using WSL/WSL2 with Ubuntu and following the Ubuntu installation guide. Or use Docker.

[MaxymVlasov]

I'll recommend building PoC on something simple, like hooks/terraform_fmt.sh, test it execution speed on big repo via pre-commit run -a and compare it with current realization, to understand is it worth it at all.

If you have no big repo - try to test on a tiny one but you'll need aware of fractions of a second and test it many times in a row. And when you're ready - send PoC PR - I have a huge repo where I'll test it :)

[ericfrederich]

Sounds good. Just wanted to make sure you'd be okay with taking on Python as a dependency before attempting a prototype.

To me at least, Python makes more sense of a dependency than Bash does since any environment running pre-commit already has Python but not necessarily Bash.

I'll give it a try.

[ericfrederich]

I'm noticing that the hooks_performance_test.sh seems biased towards hooks of language script.

These times include the overhead of pre-commit itself creating a virtual environment for the python based scripts. This is a one-time event and perhaps shouldn't be counted in the performance testing. It even says:

[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...

I would like to continue, but need to know that you would accept a solution that is performant if you concede the installation time. Unfortunately this is inherent to how pre-commit try-repo works, so maybe I need to do some manual installation and use a command other than try-repo for the hooks_performance_test.sh

no-op tests

I changed both terraform_fmt of language script as well as terraform_docs_replace of language python to immediately exit 0 / sys.exit(0). So they actually do nothing, but Python will be penalized like 1.5 seconds

5 runs 'dummy script'

time command	max	min	mean	median
users seconds	0.24	0.19	0.216	0.22
system seconds	0.08	0.05	0.068	0.07
CPU %	99	94	96.2	96
Total time	0.33	0.27	0.296	0.29

5 runs 'dummy python'

time command	max	min	mean	median
users seconds	2	1.57	1.736	1.72
system seconds	0.35	0.26	0.31	0.32
CPU %	93	92	92.4	92
Total time	2.49	2.06	2.21	2.15

[yermulnik]

Just wanted to make sure you'd be okay with taking on Python as a dependency

As you already mentioned, Python is a hard dependency for pre-commit framework that pre-commit-terraform is based on. So we actually can't decline Python =)

Python makes more sense of a dependency than Bash

If you feel you can replace all the shell related bits with Python, as @MaxymVlasov already mentioned above we'll be keen to see whether this can be viable and feasible solution.
It's not about just Bash as there are other CLI utilities in use also. Not talking about the "generic" CLI tools like terraform, infracost, checkov, terraform-docs, tflint, and others, which you'd probably not rewrite in Python, but which you would need to implement logic for if you're targeting to support not just Linux/macOS, but also Windows.

[ericfrederich]

@yermulnik I'm happy to discuss this here as suggested.

I am confused by asking me to look at man 1 bash because while the hook is implemented in bash, these --env-vars are clearly not interpreted via Bash itself.

Use the following args...

  - id: terraform_fmt
    args:
      - --env-vars=DEBUG_START="start
      - --env-vars=DEBUG_END=end"
      - --env-vars=DEBUG_MID=mi"d
      - --env-vars=DEBUG_BOTH="both"
      - --env-vars=DEBUG_BARE=bare
      - --env-vars=DEBUG_MSG1="This is not how "bash" works"
      - --env-vars=DEBUG_MSG2="this is how \"bash\" works"

... and you'll see the behavior differences between what _common.sh supports and what Bash supports.

yaml string	match	how _common.sh exports it	how Bash would handle it
--env-vars=DEBUG_START="start	NO	"start	unexpected EOF while looking for matching `"'; syntax error: unexpected end of file
--env-vars=DEBUG_END=end"	NO	end"	unexpected EOF while looking for matching `"'; syntax error: unexpected end of file
--env-vars=DEBUG_MID=mi"d	NO	mi"d	unexpected EOF while looking for matching `"'; syntax error: unexpected end of file
--env-vars=DEBUG_BOTH="both"	maybe	"both" ( or both if #651 gets merged )	both
--env-vars=DEBUG_BARE=bare	YES	bare	bare
--env-vars=DEBUG_MSG1="This is not how "bash" works"	NO	This is not how "bash" works	this is not how bash works
--env-vars=DEBUG_MSG2="this is how \"bash\" works"	NO	this is how \"bash\" works	this is how "bash" works

My point is, man 1 bash is not the place to look at what these hooks support.

For the purposes of this feature, I just need to know what to support. I can implement it to work exactly the same. I'd actually prefer if these hooks didn't fully support Bash interpretation because that would make this feature impossible. 2 lines of simple bash should be about 2 lines in any language.

So... finally, I'll just ask my question: should my Python implementation exactly mimic the behavior in the _common.sh column in the above table?

[MaxymVlasov]

So... finally, I'll just ask my question: should my Python implementation exactly mimic the behavior in the _common.sh column in the above table?

For that case, better choose how Bash would handle it interpretation. how _common.sh exports it looks more like a bug than a feature.