-
-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add terraform and terragrunt testing #581
base: master
Are you sure you want to change the base?
Conversation
@hoangelos Thanks for the contribution 👍🏻
|
.pre-commit-hooks.yaml
Outdated
description: Runs all terraform tests | ||
entry: hooks/terraform_test.sh | ||
language: script | ||
files: (\.tf)$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth of triggering TF Tests on tfvars
files change also. And maybe even on TF Lock file changes if present 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah
files: (\.tf)$ | |
files: (\.tf(vars)?(\.json)?|\.terraform\.lock\.hcl)$ |
Plus add usage examples to README as it done for other hooks |
terraform test looks for
So, your ignorance of Terragrunt has already created a gap in the hooks. Wrapper is a correct, but not fully accurate. As a wrapper, it doesn't "otters things" before it passes things along to terraform. So for example, you can "generate" files that get created the same for different environments, like providers or versions or backends. And then you might run a fmt on everything, which would catch the formatting issues in the generated files too.... However, terra grunt fmt hook only does hclfmt, which doesn't actually run any terraform fmt, which leaves these generated files as a gap. That's a long way to say, technically most of the terraform commands will need separate terragrunt commands too. |
I removed the terra grunt test, I do see the need, but this issue was about terraform test. And I'm not actually sure if terra grunt supports the test arguments and even how that would work. I'll research it and open another issue for this once my research is complete. |
Also, should I add a disclaimer in the |
What I mean is that running
Please don't try and make unreasonable assumptions.
Yes, I agree, and this is why I commented on this PR.
The TG |
Yes, you probably should. It makes sense to let consumers know this hook it compatible with Terraform version |
Please don't take my PR comments as ignorance or criticism. I do appreciate the contribution. Though what we'd want to achieve is the tool that can be used by many, which implies a need to maintain backward compatibility and reasonability of changes that are not backward compatible. |
Simplicity vs flexibility, you know… |
So are you suggesting we gate the terraform tests with a check to see if there are files that look like test files in the test directory which defaults to not the current directory but a directory called tests. |
I do want to point out that terraform test doesn’t fail if it’s run in a directory that doesn’t have tests. |
I'm suggesting to run |
I was talking more of a TF version since TF |
description: Runs all terraform tests | ||
entry: hooks/terraform_test.sh | ||
language: script | ||
files: (\.tf|\.tfvars|\.terraform\.lock\.hcl||\.terraform\.lock\.hcl||\.terraform\.lock\.json)$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files: (\.tf|\.tfvars|\.terraform\.lock\.hcl||\.terraform\.lock\.hcl||\.terraform\.lock\.json)$ | |
files: \.(tf(vars|test\.(hcl|json))?|terraform\.lock\.hcl)$ |
Should
| `terraform_tflint` | Validates all Terraform configuration files with [TFLint](https://github.com/terraform-linters/tflint). [Available TFLint rules](https://github.com/terraform-linters/tflint/tree/master/docs/rules#rules). [Hook notes](#terraform_tflint). | `tflint` | | ||
| `terraform_tfsec` | [TFSec](https://github.com/aquasecurity/tfsec) static analysis of terraform templates to spot potential security issues. [Hook notes](#terraform_tfsec) | `tfsec` | | ||
| `terraform_validate` | Validates all Terraform configuration files. [Hook notes](#terraform_validate) | `jq`, only for `--retry-once-with-cleanup` flag | | ||
| `terraform_validate` sd | Validates all Terraform configuration files. [Hook notes](#terraform_validate) | `jq`, only for `--retry-once-with-cleanup` flag | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sd
?
- --arg=-var=variable | ||
``` | ||
|
||
2. `terraform_test` only runs per repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I beg your pardon once again, though is it not possible to run terraform test
per directory? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I beg your pardon once again, though is it not possible to run
terraform test
per directory? 🤔
not sure what that would mean. Is it possible that people litter tests in directories throughout? Yes. I can put the per directory back if you want to support that. Although they’ll have to use the same convention for test-directories for it to work. That makes sense although I can imagine why someone would do that but I’m not a mono repo fan so that’s one use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that we should deliberately and explicitly support both monorepo and multirepo consumers of pre-commit-terraform
instead of making assumptions on how others should better Terraform their infrastructure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that we should deliberately and explicitly support both monorepo and multirepo consumers of
pre-commit-terraform
instead of making assumptions on how others should better Terraform their infrastructure.
I understand why you’d want to do that as your project can enforce opinions. I’m trying to balance the little time to contribute and wanted to contribute and not fork. But in a
Big organization money repo is very unwieldy so we disallow it. I’ll try to find some time to build in support that make sense per dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in a Big organization money repo is very unwieldy so we disallow it.
Well, I have ~320 unique components (modules not included), which are used by about ~2000 stacks. In one monorepo. We switched to monorepo, because it is much better manageable than hundreds of mini-repos.
I previously had "luck" to work with about ~50 repos which were set up on a per-account basis, and that was hell on Earth.
Probably, you'll start to see some issues when achieving thousands of unique components, mostly from a git perspective, which will need some split-up or additional tuning as it is done in big companies like Google or Microsoft, but it is still better than trying to maintain thousands of repos at once.
In any case, the spirit of pre-commit hooks by definition - runs only on changed files a.k.a minimum subset to check that all is OK.
Otherwise, you'll just waste time and resources.
Also, function run_hook_on_whole_repo {
is just an optimization option basically for pre-commit run -a
, because usually, you don't change every file when you run git commit
(and invoking pre-commit run
)
pre-commit-terraform/hooks/_common.sh
Lines 201 to 207 in 95fc56f
# check is (optional) function defined | |
if [ "$(type -t run_hook_on_whole_repo)" == function ] && | |
# check is hook run via `pre-commit run --all` | |
common::is_hook_run_on_whole_repo "$hook_id" "${files[@]}"; then | |
run_hook_on_whole_repo "${args[@]}" | |
exit 0 | |
fi |
At the same time, not providing the per_dir_hook_unique_part
function means a broken hook.
My comment is wn by actual terragrunt usage problem that arose with your hooks. I didn’t look into the hook so I assumed it did both fmt and hclfmt. I used terragrunt_fmt assuming it would at least alert if there were failures in formatting these generated terraform files generated by terragrunt. It didn’t. My CICD caught it. But there should be a terragrunt fmt thank a separate terragrunt_hclfmt. |
I'm a bit confused with your attitude to be honest 😢 It's an open source project and we're welcoming and we do appreciate any contribution. And I'm very much sorry you haven't gained as much satisfaction with |
I’m sorry if you are reading in an attitude. I’m just pointing out the philosophy of skipping some terragrunt commands contributes to some side effects that aren’t good. Happy to file a bug and contribute to that. |
Tests in 1.6.0 have breaking changes compared to tests that were before. So there is no reason to support |
So, to only have this run again 1.6.0 and above how would you recommend me add that. Is there any other hook that does that? |
Add info notice to README like: Important That hook supports only Terraform 1.6.0+.
Also, you could add a check to hook like this, but for pre-commit-terraform/hooks/infracost_breakdown.sh Lines 43 to 51 in 95fc56f
|
This comment was marked as off-topic.
This comment was marked as off-topic.
@hoangelos howdy. Do you plan to continue working on this PR? Do you need any help from our side? |
Put an
x
into the box if that apply:Description of your changes
This runs terraform test or terra grunt test if tf files are updated.
Fixes #549
How can we test changes
Run this on terraform repo that uses testing framework GA in 1.6.x