Skip to content
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 --recursive option #1622

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Add --recursive option #1622

merged 3 commits into from
Dec 20, 2022

Conversation

wata727
Copy link
Member

@wata727 wata727 commented Dec 18, 2022

Fixes #527
Fixes #1355
Closes #841

This PR adds --recursive option for inspection. This is useful in monorepo with multiple Terraform modules:

$ tree
.
├── dir1
│   └── main.tf
└── dir2
    └── main.tf

2 directories, 2 files

$ tflint --recursive
2 issue(s) found:

Warning: variable "foo" is declared but not used (terraform_unused_declarations)

  on dir1/main.tf line 5:
   5: variable "foo" {

Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.2.1/docs/rules/terraform_unused_declarations.md

Warning: variable "foo" is declared but not used (terraform_unused_declarations)

  on dir2/main.tf line 5:
   5: variable "foo" {

Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.2.1/docs/rules/terraform_unused_declarations.md

In recursive mode, it walks each directory with --chdir. This means that it is roughly equivalent to the following shell script:

#!/bin/bash

for dir in $(find . -type d -not -path "*/.*"; do
    if ls *.tf 1> /dev/null 2>&1; then
        tflint --chdir=dir
    fi
done

See also #1612 for behavior regarding --chdir. In addition, there are some operational notes:

  • If you want to apply a different config for each module, you can put a .tflint.hcl in each module.
    • On the other hand, if you want to apply the same config, you need to pass the absolute path using something like realpath(1). e.g. tflint --recursive --config=$(realpath .tflint.hcl)
    • Redundant config issue may be solved by Git config like .tflint.hcl files combination #1536.
  • It is possible to run module inspections recursively, but there is no way to run terraform init recursively.
    • In other words, to run recursive module inspection, you need to use a shell script that runs terraform init recursively.
  • There is currently no way to run tflint --init recursively.
    • This will be implemented in another pull request.
  • You can pass the inspection configuration with environment variables or CLI flags, but you cannot set a different configuration for each module. In this case, you should use config files.
  • Hidden directories and directories that do not contain *.tf files are excluded from recursive inspection.
  • The format and force in the config file are ignored in recursive mode. CLI flags are always respected.
  • Directory and file arguments are invalid in recursive mode.
  • Recursive inspection is performed sequentially for each module. It can be very slow for large projects.

Finally, this recursive mode is not suitable for all projects. This is an experimental feature and the behavior may change frequently in future releases. If this is unacceptable, it is safe to implement a shell script using --chdir.

@wata727
Copy link
Member Author

wata727 commented Dec 18, 2022

@bendrucker Any thoughts on this?

@bendrucker
Copy link
Member

Will review either today or tomorrow

Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Few small comments.

While reviewing this, it occurred to me that TFLint could potentially run faster in large repositories by inspecting modules in parallel. Using Chdir will make that difficult. Not sure there's any viable alternative given that some Terraform modules depend on the working directory.

cmd/inspect.go Outdated
}

return ExitCodeOK
if err := proc(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If proc() fails, the working directory is never reset to its original value. It should probably be reset regardless of the inner result. Since modules are processed in series I don't think this would have any impact right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Initially, I thought that if an error occurred, it should be returned to the caller without any changes, but It's obviously counterintuitive that the directory is changed outside of withinChangedDir.

docs/user-guide/config.md Outdated Show resolved Hide resolved
docs/user-guide/config.md Outdated Show resolved Hide resolved
@wata727
Copy link
Member Author

wata727 commented Dec 20, 2022

Thank you for your review!

it occurred to me that TFLint could potentially run faster in large repositories by inspecting modules in parallel. Using Chdir will make that difficult. Not sure there's any viable alternative given that some Terraform modules depend on the working directory.

Yes, this would be difficult because it doesn't allow you to assign a different working directory to each goroutine. I would like to implement it naively first, and consider countermeasures if this becomes a problem in many projects.

One possible idea is to reimplement the terraform.Loader and terraform.Evaluator to be independent of the working directory and achieve goroutine-based parallelization. There is a concern that this will diverge the implementation from the Terraform codebase and make it difficult to follow upstream.

Another idea is to spawn a new process with exec.Command for each module. This is the same approach as Terragrunt:
https://github.com/gruntwork-io/terragrunt/blob/v0.42.5/configstack/running_module.go#L170-L173
https://github.com/gruntwork-io/terragrunt/blob/v0.42.5/shell/run_shell_cmd.go#L69

This works well in high-CPU machines but has an issue of inconvenient aggregation of errors and results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Recursive inspection (again) Linting repo with multiple folders
2 participants