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

plugin: Drop support for bundled plugins #1286

Merged
merged 1 commit into from Mar 20, 2022
Merged

Conversation

wata727
Copy link
Member

@wata727 wata727 commented Dec 26, 2021

See also #1160

Bundled plugins have been deprecated in v0.31.0. We believe that it is well known because 5 months passed since then. This PR drops support for bundled plugins according to the deprecation.

After the end of support, AWS resources will not be inspected unless you explicitly enable the AWS ruleset. If you have already checked the warning in your workspace, follow the instructions in #1160 to add a step to install the plugin.

Copy link
Contributor

@PatMyron PatMyron left a comment

Choose a reason for hiding this comment

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

Feels like a large breaking change and a more tedious end-user process explicitly configuring AWS ruleset versions and potentially dealing with Github rate limiting, not sure I understand the benefit. Especially since we want users on the latest AWS ruleset and not being forced to specify specific versions, right?

Dependabot handles bumping the bundled AWS plugin right? #1278

Was actually thinking we should also consider bundling https://github.com/terraform-linters/tflint-ruleset-google and https://github.com/terraform-linters/tflint-ruleset-azurerm too


agree with @Vlaaaaaaad's concern in #1181 (+ #1262 + terraform-linters/tflint-ruleset-azurerm#30?):

this will lead to a lot of manual version bumping. I, like a lot of other users, want to always run with the latest and full ruleset available

@wata727
Copy link
Member Author

wata727 commented Dec 27, 2021

Thank you for your feedback. There are several benefits.

One is the reduction of binary size. When bundled with the AWS ruleset, the binary size is around 55~56MB, but this change makes the size will be reduced to 28~29MB.

https://github.com/terraform-linters/tflint/actions/runs/1625263210#artifacts
https://github.com/terraform-linters/tflint/actions/runs/1624209586#artifacts

This makes sense for GCP/Azure ruleset users. The rules associated with unneeded providers are not downloaded and you can only install the ruleset you need. Large binary not only increases network transfer overhead during downloading, but also slows down compilation and test speeds. This is one of the reasons why I didn't bundle the GCP/Azure plugins. Another reason is that I was worried that the plugins to be bundled would increase endlessly.

With the above background in mind, TFLint aims to be easier to maintain and more extensible by focusing on a neutral framework without handling any particular plugin specially.

I understand that it requires additional steps and is inconvenient for the current AWS ruleset users. However, I believe that this change is in the same direction that the Terraform separated the AWS provider from the core, and is the right direction in terms of supporting a wider variety of inspections.

I also understand the concerns of #1181, but the proposal at #1181 (comment) hasn't been discussed further and no good way has been found. Therefore, it is left as it is.

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.

Noting my firm agreement with dropping bundled plugins.

There's some ambiguity with how the terraform rules should be handled. Terraform has a built-in provider that provides terraform_remote_state which was at one time handled as a plugin IIRC. But it's effectively a single resource provider and only consumes data (state) generated by an earlier run of the same binary (terraform). The case for keeping Terraform style rules together with TFLint itself is a lot weaker. But that's not changing here.

That this change is less convenient for some users is not sufficient reason to halt it. Like Terraform, TFLint is a tool with deliberately diverse usage and there's been a lot of work to make it flexible even if those choices aren't maximally convenient for users who just care about AWS.

Dependabot doesn't really address the problem. Bumping versions across multiple repos to execute a release is not a viable workflow with many plugins. Raising the PRs automatically is a marginal efficiency gain but doesn't address the problem directly. Bundling plugins via Go modules only makes sense for offering temporary backward compatibility. If bundling was the rule rather than the exception, moving code to dedicated plugin repos isn't justified.

There's certainly more work to do to improve the plugin system. Like Terraform, we need to take things in the architecturally correct direction to support diverse usage (independent plugin binaries).

If you're concerned about receiving version updates, I'd encourage you to find ways to contribute to truly solving that problem instead of hiding from it.

@PatMyron
Copy link
Contributor

PatMyron commented Dec 27, 2021

Didn't realize plugins contributed that much binary size. Any thoughts on feasibility of meaningfully reducing plugin size? And can we continue bundling if plugin binary size was reduced meaningfully?

worried that the plugins to be bundled would increase endlessly

Agree we shouldn't bundle endlessly. Beyond the 3 cloud providers already mentioned, Kubernetes seems like the only other provider that could be worth bundling in my opinion: https://www.scalr.com/blog/top-20-terraform-providers


If you're concerned about receiving version updates, I'd encourage you to find ways to contribute to truly solving that problem instead of hiding from it.

If we do decide we must stop bundling, would like some time to look into #1181 in advance

@bendrucker
Copy link
Member

The fact that binaries are already somewhat large speaks to the general problem. Optimizing is again a marginal gain rather than a fix. If accidental bloat in an Azure plugin affects all users, that is a design problem in the surrounding system.

There are no clear criteria that could be chosen for bundling. Downloads are a good proxy for prioritizing plugin work. But choosing a "top n" or threshold, however, is inherently arbitrary.

Part of the goal here is to be consistent rather than arbitrary. To follow the same workflow regardless of provider.

Regarding complimentary features, we could certainly consider releasing other enhancements with this PR. However, this may be merged and released in the mean time. This deprecation warning has been printed for a while now. The breaking change that aws users must enable the plugin is intentional. I understand that you're arguing the opposite (that google/azure should be implicitly enabled too), but we're committed to following Terraform's lead here. The discussion in #1181 surrounds additional flexibility on version. Its inclusion does not make this less of a breaking change for users currently ignoring the deprecation warning.

@wata727
Copy link
Member Author

wata727 commented Dec 28, 2021

Any thoughts on feasibility of meaningfully reducing plugin size? And can we continue bundling if plugin binary size was reduced meaningfully?

I haven't done much research on reducing the binary size. As @bendrucker says, reducing the binary size is just one of the benefits, and the important thing is to indicate that TFLint is in a neutral position.

@PatMyron
Copy link
Contributor

choosing a "top n" or threshold, however, is inherently arbitrary

the important thing is to indicate that TFLint is in a neutral position.

follow these reasons far less than binary size; tflint is already maintaining rulesets for top providers, why not also bundle them for convenience? Sure, the exact cutoff may be arbitrary, but that doesn't detract from the major benefits


Unbundling doesn't seem urgent, especially while it's still a pain to always grab the latest plugin version

@bendrucker
Copy link
Member

Ok, I think this conversation has been pretty much exhausted at this point. Your opinion is noted but the decision is not one we plan to reverse.

@bendrucker
Copy link
Member

A central reason to avoid opinions here is to avoid having these debates, which become a drain on maintainer time and happiness. Why this plugin and not another. Why haven't you bumped this bundled plugin to incorporate some bug fix. Etc. This decision is about finding a healthy way for plugins to grow independently. We are prioritizing that flexibility over up-front convenience.

for addr := range reqs {
if addr.Type == "aws" {
log.Print("[INFO] AWS provider requirements found. Enable the plugin `aws` automatically")
fmt.Fprintln(cli.errStream, "WARNING: The plugin `aws` is not explicitly enabled. The bundled plugin will be enabled instead, but it is deprecated and will be removed in a future version. Please see https://github.com/terraform-linters/tflint/pull/1160 for details.")
Copy link
Contributor

@PatMyron PatMyron Dec 28, 2021

Choose a reason for hiding this comment

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

Thoughts on keeping warning added in #1160 with new wording?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. I thought about that, but I stopped displaying warnings because it would be an obvious noise for users who are only using Terraform rules.

@wata727
Copy link
Member Author

wata727 commented Dec 29, 2021

In the short term, bundling some major plugins will certainly help. However, I still want to support this decision to improve the independence of plugins and the TFLint core and eliminate the cost of maintaining implementations for bundled plugins.

I feel sorry for the users who are inconvenienced by this change, but I believe that this change will be easier to maintain and help develop better features.

@wata727 wata727 merged commit 3d4cb42 into master Mar 20, 2022
@wata727 wata727 deleted the drop_bundled_plugin branch March 20, 2022 18:54
@yermulnik
Copy link

yermulnik commented Mar 28, 2022

It looks like the README didn't get this change reflected (nor did https://github.com/terraform-linters/tflint-ruleset-aws#installation), hence what's the new process to install plugins as of the change in this PR?

> tflint --version
TFLint version 0.35.0

> tflint --init
# empty output

> tflint
Failed to initialize plugins; Plugin `aws` not found in /home/giermulnik/.tflint.d/plugins

What am I doing wrong?

@bendrucker
Copy link
Member

bendrucker commented Mar 28, 2022

Readme were updated

terraform-linters/tflint-ruleset-aws@31233ac

If you think there's a bug or an error in the documentation you can open an issue or PR. Full reproduction is required, which means TFLint and Terraform config.

Edit: Had the same plugin page open in two tabs. The aws plugin changing to match all other plugins is covered thoroughly in the changelog:

https://github.com/terraform-linters/tflint/blob/master/CHANGELOG.md

@terraform-linters terraform-linters locked as off-topic and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants