Add terraform and terragrunt testing #581

hoangelos · 2023-10-29T02:49:48Z

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

.pre-commit-hooks.yaml

hooks/terragrunt_test.sh

yermulnik · 2023-10-29T11:54:39Z

@hoangelos Thanks for the contribution 👍🏻
I have several considerations re this change:

I think running terraform test command makes sense only when there are TF Test files in the directory: https://developer.hashicorp.com/terraform/language/tests#syntax — «Each Terraform test lives in a test file. Terraform discovers test files are based on their file extension: .tftest.hcl or .tftest.json». This way we'd also ensure support for TF versions prior to Terraform v1.6.0
I'm not sure about having Terragrunt specific hook for the TF tests since test command doesn't belong in Terragrunt and hence gets forwarded by Terragrunt to Terraform: https://terragrunt.gruntwork.io/docs/reference/cli-options/#all-terraform-built-in-commands — «Terragrunt is a thin wrapper for Terraform, so except for a few of the special commands defined in these docs, Terragrunt forwards all other commands to Terraform». Having standalone Terragrunt Tests hook makes only sense if there might be some use cases when Terrafrunt-specific command line options need to be passed to the hook (I'm not Terragrunt user, thus I'm can say for sure if there are any use cases of such kind).

yermulnik · 2023-10-29T12:14:44Z

.pre-commit-hooks.yaml

+ description: Runs all terraform tests
+ entry: hooks/terraform_test.sh
+ language: script
+ files: (\.tf)$


It's probably worth of triggering TF Tests on tfvars files change also. And maybe even on TF Lock file changes if present 🤔

Yeah

Suggested change

files: (\.tf)$

files: (\.tf(vars)?(\.json)?|\.terraform\.lock\.hcl)$

MaxymVlasov · 2023-10-31T15:42:20Z

Plus add usage examples to README as it done for other hooks

hoangelos · 2023-11-03T21:16:18Z

@hoangelos Thanks for the contribution 👍🏻 I have several considerations re this change:

I think running terraform test command makes sense only when there are TF Test files in the directory: https://developer.hashicorp.com/terraform/language/tests#syntax — «Each Terraform test lives in a test file. Terraform discovers test files are based on their file extension: .tftest.hcl or .tftest.json». This way we'd also ensure support for TF versions prior to Terraform v1.6.0

I'm not sure about having Terragrunt specific hook for the TF tests since test command doesn't belong in Terragrunt and hence gets forwarded by Terragrunt to Terraform: https://terragrunt.gruntwork.io/docs/reference/cli-options/#all-terraform-built-in-commands — «Terragrunt is a thin wrapper for Terraform, so except for a few of the special commands defined in these docs, Terragrunt forwards all other commands to Terraform». Having standalone Terragrunt Tests hook makes only sense if there might be some use cases when Terrafrunt-specific command line options need to be passed to the hook (I'm not Terragrunt user, thus I'm can say for sure if there are any use cases of such kind).

@hoangelos Thanks for the contribution 👍🏻 I have several considerations re this change:

I think running terraform test command makes sense only when there are TF Test files in the directory: https://developer.hashicorp.com/terraform/language/tests#syntax — «Each Terraform test lives in a test file. Terraform discovers test files are based on their file extension: .tftest.hcl or .tftest.json». This way we'd also ensure support for TF versions prior to Terraform v1.6.0

terraform test looks for.tftest.hcl or .tftest.json_». in the test directory, which by default is tests. I do agree that it's likely we don't want to do this per changed file, but per repo.

I'm not sure about having Terragrunt specific hook for the TF tests since test command doesn't belong in Terragrunt and hence gets forwarded by Terragrunt to Terraform: https://terragrunt.gruntwork.io/docs/reference/cli-options/#all-terraform-built-in-commands — «Terragrunt is a thin wrapper for Terraform, so except for a few of the special commands defined in these docs, Terragrunt forwards all other commands to Terraform». Having standalone Terragrunt Tests hook makes only sense if there might be some use cases when Terrafrunt-specific command line options need to be passed to the hook (I'm not Terragrunt user, thus I'm can say for sure if there are any use cases of such kind).

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.

hoangelos · 2023-11-03T21:59:37Z

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.

hoangelos · 2023-11-03T22:00:28Z

Also, should I add a disclaimer in the terraform_test section of the README that this may affect the commit time. Because if the test suite is long.... I'd think that's a give, but you know what they say about assumptions.

yermulnik · 2023-11-03T22:12:09Z

I think running terraform test command makes sense only when there are TF Test files in the directory: developer.hashicorp.com/terraform/language/tests#syntax — «Each Terraform test lives in a test file. Terraform discovers test files are based on their file extension: .tftest.hcl or .tftest.json». This way we'd also ensure support for TF versions prior to Terraform v1.6.0

terraform test looks for.tftest.hcl or .tftest.json_». in the test directory, which by default is tests. I do agree that it's likely we don't want to do this per changed file, but per repo.

What I mean is that running terraform test makes only sense if the commit comprised changes to either TF config files or TF test files only when there are TF test files in current directory, rather than there are just "only TF config files changed", as there's no sense running terraform test command if there are no TF test files in current directory, including TF versions less than 1.6.0, which have no test command for Terraform CLI.

I'm not sure about having Terragrunt specific hook for the TF tests since test command doesn't belong in Terragrunt and hence gets forwarded by Terragrunt to Terraform: terragrunt.gruntwork.io/docs/reference/cli-options/#all-terraform-built-in-commands — «Terragrunt is a thin wrapper for Terraform, so except for a few of the special commands defined in these docs, Terragrunt forwards all other commands to Terraform». Having standalone Terragrunt Tests hook makes only sense if there might be some use cases when Terrafrunt-specific command line options need to be passed to the hook (I'm not Terragrunt user, thus I'm can say for sure if there are any use cases of such kind).

So, your ignorance of Terragrunt has already created a gap in the hooks.

Please don't try and make unreasonable assumptions.

Wrapper is a correct, but not fully accurate.

Yes, I agree, and this is why I commented on this PR.
It looked like there's no sense to require Terragrunt CLI and to relay test command via Terragrunt CLI if terraform test suffices the task. I might be wrong though as I'm not a TG user (which doesn't imply ignorance though).

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.

The TG hclfmt is not a TF command, whilst TG does relay test command directly to TF since it's TF-specific command as opposite to hclfmt being a TG-specific command, which doesn't belong in TF, and hence this means what you're comparing is not of the same kind with what I was talking about.

yermulnik · 2023-11-03T22:21:41Z

Also, should I add a disclaimer in the terraform_test section of the README that this may affect the commit time. Because if the test suite is long.... I'd think that's a give, but you know what they say about assumptions.

Yes, you probably should. It makes sense to let consumers know this hook it compatible with Terraform version >=1.6.0 only and will produce failure for all the folks with Terraform version <1.6.0. Along with that it makes sense to remove this hook from the default pre-commit-terraform configuration to maintain backward compatibility by avoiding failure that requires human interaction to revolve the failure by adjusting default pre-commit-terraform config.
And yes, I'd think that's a give, but you know what they say about OSS breaking backward compatibility w/o argued reason and prior (long prior) notification 😏

yermulnik · 2023-11-03T22:28:27Z

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.

yermulnik · 2023-11-03T22:30:08Z

Simplicity vs flexibility, you know…

hoangelos · 2023-11-03T22:59:24Z

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.

hoangelos · 2023-11-03T23:03:42Z

I do want to point out that terraform test doesn’t fail if it’s run in a directory that doesn’t have tests.

yermulnik · 2023-11-03T23:12:46Z

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'm suggesting to run terraform test command only in those directories that either have "TF config files changed AND there are TF test files in the same directory" or have "TF test files changed".
There's no "look like test files" from what I see in the documentation. There's pretty definitive explanation of which exact files are considered TF test files to be honest.

yermulnik · 2023-11-03T23:23:53Z

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 was talking more of a TF version since TF test command was officially introduced with version 1.6.0 (and was not guaranteed to be supported prior to 1.6.0) rather than about TF failing or not.
But... but I must admit I didn't know TF supports test command since version 0.15.0.
So what I was concerned about doesn't make sense with TF versions >=0.15.0.
To be honest I'm not quire sure about which most lowest version of Terraform pre-commit-terraform supports officially. Hence it may (or may not) make sense to support earlier versions of Terraform.
@MaxymVlasov @antonbabenko What do you folks think?

yermulnik · 2023-11-03T23:30:30Z

.pre-commit-hooks.yaml

+ description: Runs all terraform tests
+ entry: hooks/terraform_test.sh
+ language: script
+ files: (\.tf|\.tfvars|\.terraform\.lock\.hcl||\.terraform\.lock\.hcl||\.terraform\.lock\.json)$


Suggested change

files: (\.tf|\.tfvars|\.terraform\.lock\.hcl||\.terraform\.lock\.hcl||\.terraform\.lock\.json)$

files: \.(tf(vars|test\.(hcl|json))?|terraform\.lock\.hcl)$

Should

yermulnik · 2023-11-03T23:31:03Z

README.md

 | `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 |


yermulnik · 2023-11-03T23:32:49Z

README.md

+ - --arg=-var=variable
+ ```
+
+2. `terraform_test` only runs per repository.


I beg your pardon once again, though is it not possible to run terraform test per directory? 🤔

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.

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.

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.

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.

hoangelos · 2023-11-03T23:41:41Z

The TG hclfmt is not a TF command, whilst TG does relay test command directly to TF since it's TF-specific command as opposite to hclfmt being a TG-specific command, which doesn't belong in TF, and hence this means what you're comparing is not of the same kind with what I was talking about.

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.

yermulnik · 2023-11-03T23:54:27Z

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 pre-commit-terraform as you expected from it. And I hope you'll become a valuable part of this community by bringing in solution(s) for the use cases you've been hitting and that might be a matter for others.

hoangelos · 2023-11-04T00:17:27Z

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 pre-commit-terraform as you expected from it. And I hope you'll become a valuable part of this community by bringing in solution(s) for the use cases you've been hitting and that might be a matter for others.

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.

MaxymVlasov · 2023-11-08T15:10:26Z

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 was talking more of a TF version since TF test command was officially introduced with version 1.6.0 (and was not guaranteed to be supported prior to 1.6.0) rather than about TF failing or not. But... but I must admit I didn't know TF supports test command since version 0.15.0. So what I was concerned about doesn't make sense with TF versions >=0.15.0. To be honest I'm not quire sure about which most lowest version of Terraform pre-commit-terraform supports officially. Hence it may (or may not) make sense to support earlier versions of Terraform. @MaxymVlasov @antonbabenko What do you folks think?

Tests in 1.6.0 have breaking changes compared to tests that were before. So there is no reason to support test before 1.6.0 until someone opens a feature request because of opentofu etc.

hoangelos · 2023-11-08T15:19:36Z

Tests in 1.6.0 have breaking changes compared to tests that were before. So there is no reason to support test before 1.6.0 until someone opens a feature request because of opentofu etc.

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?

MaxymVlasov · 2023-11-08T16:06:08Z

Tests in 1.6.0 have breaking changes compared to tests that were before. So there is no reason to support test before 1.6.0 until someone opens a feature request because of opentofu etc.

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+.
Reason: there are significant changes to how tests are written and executed, based on feedback from the experimental phase

> [!IMPORTANT]
> That hook supports only Terraform 1.6.0+.
> Reason: there are [significant changes](https://github.com/hashicorp/terraform/releases/tag/v1.6.0) to how tests are written and executed, based on feedback from the experimental phase"

Also, you could add a check to hook like this, but for terraform --version -json | grep -E -o '[0-9]+\.[0-9]+'
and compare it with 1.6

pre-commit-terraform/hooks/infracost_breakdown.sh

Lines 43 to 51 in 95fc56f

 local RESULTS 

 RESULTS="$(infracost breakdown "${args[@]}" --format json)" 

 local API_VERSION 

 API_VERSION="$(jq -r .version <<< "$RESULTS")" 

 if [ "$API_VERSION" != "0.2" ]; then 

 common::colorify "yellow" "WARNING: Hook supports Infracost API version \"0.2\", got \"$API_VERSION\"" 

 common::colorify "yellow" " Some things may not work as expected" 

 fi

peter-halliday added 2 commits October 28, 2023 21:39

Add terraform and terragrunt testing

da09736

Add terraform and terragrunt testing

d119e33

hoangelos requested review from MaxymVlasov and yermulnik as code owners October 29, 2023 02:49

yermulnik reviewed Oct 29, 2023

View reviewed changes

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved

hooks/terragrunt_test.sh Outdated Show resolved Hide resolved

yermulnik mentioned this pull request Oct 29, 2023

Support for terraform test #549

Open

yermulnik reviewed Oct 29, 2023

View reviewed changes

MaxymVlasov added the feature New feature or request label Oct 31, 2023

MaxymVlasov added this to the Features implementing and cleanup without breaking changes milestone Oct 31, 2023

update after PR comments

42d470d

yermulnik reviewed Nov 3, 2023

View reviewed changes

This comment was marked as off-topic.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add terraform and terragrunt testing #581

Add terraform and terragrunt testing #581

hoangelos commented Oct 29, 2023 •

edited by MaxymVlasov

yermulnik commented Oct 29, 2023

yermulnik Oct 29, 2023 •

edited

MaxymVlasov Oct 31, 2023

MaxymVlasov commented Oct 31, 2023

hoangelos commented Nov 3, 2023

hoangelos commented Nov 3, 2023

hoangelos commented Nov 3, 2023

yermulnik commented Nov 3, 2023 •

edited

yermulnik commented Nov 3, 2023 •

edited

yermulnik commented Nov 3, 2023

yermulnik commented Nov 3, 2023

hoangelos commented Nov 3, 2023

hoangelos commented Nov 3, 2023

yermulnik commented Nov 3, 2023

yermulnik commented Nov 3, 2023 •

edited

yermulnik Nov 3, 2023

yermulnik Nov 3, 2023

yermulnik Nov 3, 2023

hoangelos Nov 3, 2023

yermulnik Nov 3, 2023

hoangelos Nov 4, 2023

MaxymVlasov Nov 8, 2023 •

edited

hoangelos commented Nov 3, 2023

yermulnik commented Nov 3, 2023

hoangelos commented Nov 4, 2023

MaxymVlasov commented Nov 8, 2023

hoangelos commented Nov 8, 2023

MaxymVlasov commented Nov 8, 2023

This comment was marked as off-topic.

	files: (\.tf)$
	files: (\.tf(vars)?(\.json)?\|\.terraform\.lock\.hcl)$

	files: (\.tf\|\.tfvars\|\.terraform\.lock\.hcl\|\|\.terraform\.lock\.hcl\|\|\.terraform\.lock\.json)$
	files: \.(tf(vars\|test\.(hcl\|json))?\|terraform\.lock\.hcl)$

	# 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

Add terraform and terragrunt testing #581

Are you sure you want to change the base?

Add terraform and terragrunt testing #581

Conversation

hoangelos commented Oct 29, 2023 • edited by MaxymVlasov

Description of your changes

How can we test changes

yermulnik commented Oct 29, 2023

yermulnik Oct 29, 2023 • edited

Choose a reason for hiding this comment

MaxymVlasov Oct 31, 2023

Choose a reason for hiding this comment

MaxymVlasov commented Oct 31, 2023

hoangelos commented Nov 3, 2023

hoangelos commented Nov 3, 2023

hoangelos commented Nov 3, 2023

yermulnik commented Nov 3, 2023 • edited

yermulnik commented Nov 3, 2023 • edited

yermulnik commented Nov 3, 2023

yermulnik commented Nov 3, 2023

hoangelos commented Nov 3, 2023

hoangelos commented Nov 3, 2023

yermulnik commented Nov 3, 2023

yermulnik commented Nov 3, 2023 • edited

yermulnik Nov 3, 2023

Choose a reason for hiding this comment

yermulnik Nov 3, 2023

Choose a reason for hiding this comment

yermulnik Nov 3, 2023

Choose a reason for hiding this comment

hoangelos Nov 3, 2023

Choose a reason for hiding this comment

yermulnik Nov 3, 2023

Choose a reason for hiding this comment

hoangelos Nov 4, 2023

Choose a reason for hiding this comment

MaxymVlasov Nov 8, 2023 • edited

Choose a reason for hiding this comment

hoangelos commented Nov 3, 2023

yermulnik commented Nov 3, 2023

hoangelos commented Nov 4, 2023

MaxymVlasov commented Nov 8, 2023

hoangelos commented Nov 8, 2023

MaxymVlasov commented Nov 8, 2023

This comment was marked as off-topic.

hoangelos commented Oct 29, 2023 •

edited by MaxymVlasov

yermulnik Oct 29, 2023 •

edited

yermulnik commented Nov 3, 2023 •

edited

yermulnik commented Nov 3, 2023 •

edited

yermulnik commented Nov 3, 2023 •

edited

MaxymVlasov Nov 8, 2023 •

edited