Open ericfrederich opened 6 months ago
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.
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 :)
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.
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
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
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 |
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 |
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.
@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?
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.
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.
They are: https://github.com/antonbabenko/pre-commit-terraform/blob/master/hooks/_common.sh#L536 (not specifically interpreted, which makes that function do half of the task apparently, though still those vars are handled by Bash to get them exported into env).
My point is,
man 1 bash
is not the place to look at what these hooks support.
All of the existing hooks rely upon Bash specifically (apart from terraform_docs_replace.py
). Bash is the interpreter of those shell scripts.
For the purposes of this feature, I just need to know what to support.
If I got it right, the gist of this feature is to re-implement hooks in Python. For this goal I'd say you wanted to support the result (altogether with existing user-facing options and features) β if you can replicate what benefits these hooks bring to our users, then the goal is accomplished IMHO.
I'd actually prefer if these hooks didn't fully support Bash interpretation because that would make this feature impossible.
I think the idea must be to re-implement rather than to replicate. Hence you're through the greenfield and have all the options to re-implement the way it is better for users.
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?
I'm more than certain β again β you should be targeting to implement the result, rather than behavior.
PS: Export vars the way they should be exported from within any language. As @MaxymVlasov already mentioned, the _common.sh
way to do it is more of a hacky rather than a proper. We had to had a balance between full interpretation (which would imply eval
which @MaxymVlasov is against of and this makes perfect sense) and the missing feature that our users asked for.
This issue has been automatically marked as stale because it has been open 30 days with no activity. Remove stale label or comment or this issue will be closed in 10 days
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.