chore: add internal-tflint strict control#5361
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a legacy strict control Changes
Sequence DiagramsequenceDiagram
participant Hook as Hook Executor
participant Strict as Strict Controls
participant TFLint as TFLint Runner
Hook->>Hook: Inspect actionParams for TfExternalTFLint
alt TfExternalTFLint present
Hook->>TFLint: executeTFLint(..., externalTfLint=true)
else TfExternalTFLint absent
Hook->>Strict: Query InternalTFLint control
Strict-->>Hook: control value (true/false)
Hook->>TFLint: executeTFLint(..., externalTfLint=derived)
end
TFLint->>TFLint: RunTflintWithOpts(..., externalTfLint)
TFLint-->>Hook: return result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
97fd836 to
f57d944
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/runner/run/hook.go`:
- Around line 190-194: The code calls opts.StrictControls.Find(...) without
guarding against opts.StrictControls being nil; add a nil check for
opts.StrictControls before calling StrictControls.Find to avoid a panic (e.g.,
only call opts.StrictControls.Find(controls.InternalTFLint) when
opts.StrictControls != nil), keep the existing check that strictControl != nil
before calling strictControl.Evaluate(ctx), and ensure forceExternalTFLint is
set based on the guarded result; references: opts.StrictControls,
StrictControls.Find, controls.InternalTFLint, strictControl.Evaluate.
|
I've discovered that our internal running of I'm refactoring to still do the init automatically. |
fe693c1 to
89478e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/tflint/tflint.go`:
- Around line 86-94: Replace the accidental redeclaration of args by creating a
new execArgs slice and use it for the command construction: create execArgs :=
make([]string, 0, 5+len(variables)+len(tfVariables)+len(tflintArgs)), then
append the same items ( "tflint", "--config", configFile, "--chdir",
opts.WorkingDir, variables..., tfVariables..., tflintArgs...) into execArgs and
update all subsequent uses of args in this function to use execArgs (references
around where args is later consumed). This avoids the := redeclaration error
with the existing args identifier and keeps the assembled command in execArgs.
♻️ Duplicate comments (1)
internal/runner/run/hook.go (1)
187-192: Add nil check foropts.StrictControlsto prevent potential panic.If
opts.StrictControlsis nil, callingFindon it will cause a nil pointer dereference. This was identified in a previous review and the author acknowledged they would add a nil check.🛡️ Proposed defensive check
} else { - strictControl := opts.StrictControls.Find(controls.InternalTFLint) - forceExternalTFLint := false - if strictControl != nil && strictControl.Evaluate(ctx) != nil { - forceExternalTFLint = true + if opts.StrictControls != nil { + strictControl := opts.StrictControls.Find(controls.InternalTFLint) + if strictControl != nil && strictControl.Evaluate(ctx) != nil { + forceExternalTFLint = true + } } return executeTFLint(ctx, l, opts, cfg, curHook, workingDir, forceExternalTFLint) }
🧹 Nitpick comments (2)
mise.toml (1)
8-8: Version format inconsistency with other Go tools.Other Go tool entries use the
vprefix (e.g.,v0.9.0,v0.6.0,v0.20.0), but this entry uses0.38.0without the prefix. For consistency and to match the actual release tags on golang.org/x/tools, consider adding thevprefix.♻️ Suggested fix
-"go:golang.org/x/tools/cmd/goimports" = "0.38.0" +"go:golang.org/x/tools/cmd/goimports" = "v0.38.0"docs-starlight/src/content/docs/03-features/10-hooks.mdx (1)
300-300: Minor: Extra whitespace in example.There's an extra space after
"tflint"before the comma that's inconsistent with other examples in the file.📝 Suggested fix
- execute = ["tflint" , "--terragrunt-external-tflint", "--minimum-failure-severity=error", "--config", "custom.tflint.hcl"] + execute = ["tflint", "--terragrunt-external-tflint", "--minimum-failure-severity=error", "--config", "custom.tflint.hcl"]
c9f7bdc to
b1e36b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs-starlight/src/content/docs/03-features/10-hooks.mdx`:
- Around line 276-279: In the <Aside type="warning"> block remove the duplicated
phrase "due to the adoption of a BUSL license" and rephrase the clause that
currently reads "due to the adoption of a BUSL license in `tflint` (due to the
adoption of a BUSL license in Terraform)" into a single concise explanation (for
example: "after Terraform adopted a BUSL license, `tflint` did as well"), so the
sentence reads clearly and without repetition while keeping the same meaning.
| <Aside type="warning"> | ||
| Previous versions of Terragrunt shipped with a version of `tflint` compiled into the binary to allow for more convenient usage without installing `tflint` directly. Due to the adoption of a BUSL license in `tflint` (due to the adoption of a BUSL license in Terraform), the version included in Terragrunt was frozen. | ||
|
|
||
| Use of the internal `tflint` is deprecated, and will be removed in the future. You may opt in ahead of time by |
There was a problem hiding this comment.
Tighten duplicated clause in the BUSL explanation.
The sentence repeats “due to the adoption…” twice. Consider tightening for clarity.
📝 Proposed fix
- Previous versions of Terragrunt shipped with a version of `tflint` compiled into the binary to allow for more convenient usage without installing `tflint` directly. Due to the adoption of a BUSL license in `tflint` (due to the adoption of a BUSL license in Terraform), the version included in Terragrunt was frozen.
+ Previous versions of Terragrunt shipped with a version of `tflint` compiled into the binary to allow for more convenient usage without installing `tflint` directly. Due to the adoption of a BUSL license in `tflint` (following Terraform’s move), the version included in Terragrunt was frozen.🤖 Prompt for AI Agents
In `@docs-starlight/src/content/docs/03-features/10-hooks.mdx` around lines 276 -
279, In the <Aside type="warning"> block remove the duplicated phrase "due to
the adoption of a BUSL license" and rephrase the clause that currently reads
"due to the adoption of a BUSL license in `tflint` (due to the adoption of a
BUSL license in Terraform)" into a single concise explanation (for example:
"after Terraform adopted a BUSL license, `tflint` did as well"), so the sentence
reads clearly and without repetition while keeping the same meaning.
Description
Starts the deprecation process for the embeded version of
tflint.Tflint is tainted by the BUSL license in Terraform; this prevents us from being able to keep the embedded version updated; fortunately our hooks system makes running a local version of tflint pretty easy!
This PR creates a new strict control that forces the use of external tflint, maintaining the automatic
tflint initand passing of variables.TODOs
Release Notes (draft)
Add strict control for deprecation of embedded tflint, our ability to embed having been hampered by the BUSL license change in Terraform.
Migration Guide
No breaking changes right now. But if you use the embedded
tflintright now you can ensure no breakages when we later remove it by adding the--terragrunt-external-tflintflag:Or by enabling the
legacy-internal-tflintstrict control.When the embedded
tflintis later removed, we will still strip this flag, perform automatictflint init, and pass in variables when running the external tool.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.