aws_write_only_arguments: recommend write-only arguments where available

[aristosvo]

Fixes #859 partially

A review on the direction of these kind of checks would be really appreciated, I would happily implement all the write-only arguments once we have a direction.

[bendrucker]

I would suggest tying this up into one meta-rule for recommending ephemeral resources and write only. Or two. Versus doing this on a per-resource basis.

Then you can just drive it off configuration/data and quickly handle all of them without all boilerplate. E.g. for write only attributes, a map of resource to a list of attributes with write only alternatives:

type writeOnlyAttribute struct {
    original    string
    alternative string
}

writeOnlyAttributes := map[string][]writeOnlyAttribute{
    "aws_secretsmanager_secret_version": {
        {
            original:    "secret_string",
            alternative: "secret_string_wo",
        },
    },
}

Additionally, you should use EmitIssueWithFix here so that users can automatically convert their code with --fix. There are no existing autofixes in this repository but you can see an example here:

https://github.com/terraform-linters/tflint-ruleset-terraform/blob/c2e1c2e3337034dc475f77d574de64213384c2b4/rules/terraform_deprecated_index.go#L80

Here's the methods you can call to make replacements:

https://pkg.go.dev/github.com/terraform-linters/tflint-plugin-sdk/tflint#Fixer

[aristosvo]

Thanks @bendrucker ! Based on your suggestions it is implemented for the write-only attributes. I also included suggestions for non-writeOnly solutions like managed passwords via SecretsManager.

I’ll have to puzzle a bit how to recommend using an ephemeral resource instead of a data source, but I think that would be a different MR.

I didn’t include the automatic version checking (yet), as that raised a lot of extra questions like:

• if they are on a version lower that 1.11, should I raise this issue at all?
• if raised, should I recommend them to update, maybe even auto fix that?
• can I make check opt-in for versions lower than 1.11, and opt-out for versions 1.11 and later?

Sorry for all the questions, but it seems this rule has some unique patterns to it 😅

[bendrucker]

Nice, few more edits, but getting close.

Also, it'll be annoying, but in reviewing the docs:

https://developer.hashicorp.com/terraform/language/resources/ephemeral/write-only

They decided to call these write-only arguments and not attributes, presumably to emphasize that they cannot be referenced like traditional attributes. I know I called them by the wrong name initially/in my example, but we do need to match Terraform's nomenclature.

[wata727]

Overall good! Only comment left with concern about autofix.

[bendrucker]

Nice work on the auto-fix!

Last thing: this rule isn't an ideal candidate for a generator, at least for the rule code itself. You need a generator when you want to build multiple rules from data. You can't create types in a loop.

But in this case you just have one rule, and could have your generator just write the relevant data for the rule to use (e.g., a JSON file) in its Check method. You'd embed the file. Or, you could generate Go code with a global variable.

Either way, this means you can test your rule for a handful of representative cases rather than having generated tests for every resource.

[aristosvo]

Nice work on the auto-fix!

Last thing: this rule isn't an ideal candidate for a generator, at least for the rule code itself. You need a generator when you want to build multiple rules from data. You can't create types in a loop.

But in this case you just have one rule, and could have your generator just write the relevant data for the rule to use (e.g., a JSON file) in its Check method. You'd embed the file. Or, you could generate Go code with a global variable.

Either way, this means you can test your rule for a handful of representative cases rather than having generated tests for every resource.

Thanks @bendrucker!

• I generated a global variable instead of the complete rule. Embedding a JSON file felt a bit off compared to the global variable, although that might be a taste issue 🙃
• Reduced tests to one type of resource instead of all of them

Last bit might be to arrange the files differently, but I don't know what you'd think of that. For me it is fine, but I can imagine that as a maintainer you have a stricter/more defined/longer term vision on this.

[wata727]

Looks good to me. Will merge after @bendrucker's approval.

[bendrucker]

Nice! Two small comments, neither essential.

[bendrucker]

Thanks again!