Add range to terraform_required_version errors

[rdimartino]

Fixes #177

I wondered about sorting the blocks / filenames or looking for main.tf, but decided to keep it simple.

A couple questions came up for me:

• Is it ok to use the first block? Should we sort by filename or line number or something?

• Pulling an arbitrary filename from the map of files doesn't feel great. It seems weird for the error to suggest placing the required_version in variables.tf for instance. Maybe we should just show the directory if no "terraform" blocks are found? We could parse that out of the filename.

Let me know what you think.

Thanks!

[bendrucker]

I'd say definitely skip this else logic. Picking the first block is good but a random file is not.

[rdimartino]

Well, in my case at least, completely skipping this logic wouldn't solve the issue I ran into #177. I had several local modules and none of them had a "terraform" block. When I ran tflint for the first time I got a bunch of

Warning: terraform "required_version" attribute is required (terraform_required_version)

  on  line 0:
   (source code not available)

And was very confused about where these were coming from.

I agree that a random file is not ideal. What about pointing to the directory?

[bendrucker]

In theory, Filename should be a filename. If you can show a directory contributes useful output and doesn't break things that sounds fine.

[rdimartino]

Alright, I've updated the else logic. In the case where files are missing a "terraform" block, it now references either the single file in the module or it points to the directory if there are multiple files. The test cases "no terraform block" and "multiple files no terraform blocks" cover these scenarios. Without the else logic, both of those cases would report the confusing range: on line 0:.

In my local example that lead to the creation of the issue, the output now looks like:

❯ tflint --recursive --only terraform_required_version
2 issue(s) found:

Warning: terraform "required_version" attribute is required (terraform_required_version)

  on modules/deployment/ line 0:
   (source code not available)

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

Warning: terraform "required_version" attribute is required (terraform_required_version)

  on modules/dns/ line 0:
   (source code not available)

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

Warning: terraform "required_version" attribute is required (terraform_required_version)

  on modules/email/ line 0:
   (source code not available)

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

[wata727]

As mentioned in terraform-linters/tflint#1790 (comment), I favor a random file over passing directory path as Filename.
I agree that this is not ideal, but I think it's a reasonable choice considering the integration with other tools.

However, we should always attach the same range so that it does not change each time you run it.

[rdimartino]

An arbitrary file may be awkward, sure, but IMHO certainly better than providing no context for the source of the issue at all.

In my case pointing to main.tf would've resolved the issue in #177, so I'm happy to do that, but want to share my 2 cents.

[bendrucker]

Last idea is to report on a main.tf file, whether or not it's there. The source code won't be there but you'll get a range printed with (source unavailable) or similar.

certainly better than providing no context

The other side of this coin is that static analysis tools have a duty to not provide wrong advice. False detection positives will happen but the remediation advice should be clear/specific. Picking an arbitrary file and telling the user "put this config there" seems to risk giving bad advice (more specific than the context allows).

[wata727]

Picking an arbitrary file and telling the user "put this config there" seems to risk giving bad advice (more specific than the context allows).

Agreed with this concern. My proposed policy doesn't allow for the issue to be "create a new file and write to it."

From the perspective of avoiding false positives, maybe there is a way to not report an issue if the "terraform" block is missing. However, I'm not sure if this is the behavior that users want.

On the other hand, any potential misleading behavior may be mitigated by other rules such as terraform_standard_module_structure.

Last idea is to report on a main.tf file, whether or not it's there. The source code won't be there but you'll get a range printed with (source unavailable) or similar.

I've thought about this idea too, but ultimately it brings up issues that aren't well-handled in LSP or Problem Matchers, so I'm not positive about it.

[wata727]

As an aside, instead of main.tf, maybe you could follow the official guidelines and recommend terraform.tf instead.
https://developer.hashicorp.com/terraform/language/style#file-names

[bendrucker]

Oh nice! Pretty recent, let's do that:

hashicorp/terraform#34876

[bendrucker]

Per L49 this rule only runs in root modules. So it should be sufficient to just take the module path and not do all this work around inferring from the list of files, no?

[rdimartino]

That sounds reasonable to me, but I'm admittedly new to terraform and definitely new to this code base, so I'm not sure what assumptions are safe to make.

It was also unclear to me a direct way to get the module path. runner.Ctx.ModulePath looked promising, but I wasn't sure how to handle a addrs.ModuleInstance.

If it's straight forward to do then, yeah, I think this could be simplified a lot too to something like:

...
} else {
    if len(files) == 1 {
        for filename := range files {
            missingRange = hcl.Range{ Filename: filename }
        }
    } else {
        missingRange = hcl.Range{ Filename: < module path > }
    }
}

[bendrucker]

Hmm I guess there's no official interface for getting the path on disk, just the path in Terraform (module.*). That said, each runner should only operate in a single module (directory). So I think it should be enough to just do filepath.Dir(files[0])?

[rdimartino]

Sounds great to me. I've updated it, so I pull a single file from the map. If it's the only file, use it for the hcl.Range otherwise if there are multiple files, use filepath.Dir to reference the directory of the module.

[rdimartino]

@bendrucker @wata727 Please take a look. I tried to incorporate the feedback you both provided

[wata727]

We may need to specify the End as well.

[rdimartino]

Should that also be hcl.InitialPos?

[wata727]

Yes, a range where the End is less than the Start may not be valid.

[rdimartino]

Thanks, done!

[bendrucker]

LGTM, will let @wata727 take a final look

[wata727]

Perfect. Thanks!