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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add new rule to check comparisons with empty list #1359

Merged

Conversation

bmbferreira
Copy link
Contributor

@bmbferreira bmbferreira commented Apr 17, 2022

As explained by @jbardin on this issue "...the preferred way to check for an empty set or series of any type would be to check its length, rather than compare it against a literal value".
I've had a bug on my terraform code that took me almost 2 days to find out the root cause, just to find out that we had a check for [] in our code that was returning always false. 馃槗
Since we are already using tflint, having a check for it would be a really good way to avoid doing the same stupid mistake again! 馃槂 Hopefully, you will also find this new rule useful and everyone can save a lot of headaches with it as well.

More info about why this rule is "needed":
https://discuss.hashicorp.com/t/conditional-for-empty-list/26958/2
hashicorp/terraform#25798 (comment)

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

test.tf Outdated Show resolved Hide resolved
docs/rules/terraform_empty_list_check.md Outdated Show resolved Hide resolved
rules/terraformrules/terraform_empty_list_check_test.go Outdated Show resolved Hide resolved
rules/terraformrules/terraform_empty_list_check.go Outdated Show resolved Hide resolved
Co-authored-by: Ben Drucker <bvdrucker@gmail.com>
@bmbferreira
Copy link
Contributor Author

@bendrucker thank you for reviewing it so quickly! 馃檱

Just fixed all the issues reported.

Have a great day!

@bendrucker
Copy link
Member

Will get back on this tomorrow

@bmbferreira
Copy link
Contributor Author

Will get back on this tomorrow

Thank you!

@bmbferreira
Copy link
Contributor Author

sorry to bother you @bendrucker, but please let me know if there's still something missing on the PR 馃槂

thank you! and have an awesome week!

Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

It looks good to me. If @bendrucker approves, merge this pull request.

@bendrucker bendrucker merged commit f45241c into terraform-linters:master Jun 20, 2022
@sergei-ivanov
Copy link

Thanks for adding this rule, but unfortunately it only picks up the most trivial cases.

For example, it picks up this:

resource "aws_security_group_rule" "outbound_proxy" {
  count =var.proxy_subnets != [] ? 1 : 0

but not this:

resource "aws_security_group_rule" "outbound_proxy" {
  count = var.enabled && var.proxy_subnets != [] ? 1 : 0

nor this:

templatefile("${path.module}/jenkins.yaml.tmpl", {
    JENKINS_INGRESS_ENABLED = var.jenkins_controller_alb_allow_list != []
})

I guess it needs to implement more recursion for parsing into more complex expressions.

@bmbferreira bmbferreira deleted the add-empty-list-check-rule branch June 23, 2022 22:13
@bmbferreira
Copy link
Contributor Author

Thanks for the feedback @sergei-ivanov! Can you report this as an issue so I can tackle it when I have some time please?

馃檱

@sergei-ivanov
Copy link

@bmbferreira I have created #1424 -- thank you very much for your contribution!

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.

None yet

4 participants