fix: Fixing excessive warning for dependency input usage #3879
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces several updates across the project. The CircleCI configuration and various test files update deprecated environment variable names (e.g., replacing Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/CLI
participant PC as ParseConfig()
participant D as detectInputsCtyUsage()
participant SC as StrictControl
U->>PC: Start configuration parsing
PC->>D: Check for dependency input usage
D-->>PC: Return usage flag (found/not found)
PC->>SC: Evaluate strict control settings
SC-->>PC: Return error/warning if necessary
PC-->>U: Return parsed config or error status
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There is no need to change the interface. There are two options:
skipDependenciesInputsControl := &Control{
// TODO: `ErrorFmt` and `WarnFmt` of this control are not displayed anywhere and needs to be reworked.
Name: SkipDependenciesInputs,
Description: "Disable reading of dependency inputs to enhance dependency resolution performance by preventing recursively parsing Terragrunt inputs from dependencies.",
Error: errors.Errorf("Reading inputs from dependencies is no longer supported. To acquire values from dependencies, use outputs."),
Category: stageCategory,
}If
type ControlWithoutWarning struct {
*Control
}
func (ctrl *ControlWithoutWarning) Evaluate(ctx context.Context) error {
// Do what you want.
return nil
} |
config/config_partial.go
Outdated
| suppressable, err := skipDependenciesInputs.(*controls.SuppressableControl) | ||
| if err { |
There was a problem hiding this comment.
| suppressable, err := skipDependenciesInputs.(*controls.SuppressableControl) | |
| if err { | |
| suppressable, ok := skipDependenciesInputs.(*controls.SuppressableControl) | |
| if ok { |
| // Suppress suppresses the warning message of the control. | ||
| func (ctrl *SuppressableControl) Suppress() *SuppressableControl { | ||
| ctrl.suppress = true | ||
|
|
||
| return ctrl | ||
| } |
There was a problem hiding this comment.
| // Suppress suppresses the warning message of the control. | |
| func (ctrl *SuppressableControl) Suppress() *SuppressableControl { | |
| ctrl.suppress = true | |
| return ctrl | |
| } | |
| // Suppress suppresses the warning message of the control. | |
| func (ctrl *Control) Suppress() *Control { | |
| ctrl.suppress = true | |
| return ctrl | |
| } |
I think we can add a suppress field to the already existing Control structure and use it where we need. The default value suppress: false will not affect the behavior of other controls in any way. And in this way you can greatly simplify the code, and if necessary use it in other controls, if such a need arises in the future.
|
The new structure approach is justified when you need to write different logic for |
|
Sorry if I confused you with the options initially. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/config.go (1)
960-991: Looks logically consistent with a straightforward approach.This helper method diligently hunts for
dependency.*.inputsreferences in HCL attributes. In case you need deeper or more dynamic referencing detection beyond three segments, consider refining or generalizing further. Otherwise, this does the trick without overhead.test/integration_test.go (1)
3030-3038: LGTM! Consider adding a comment explaining the warning removal.The removal of the deprecation warning check makes sense if the functionality is no longer deprecated. However, it would be helpful to add a comment explaining why this check was removed to provide context for future maintainers.
_, stderr, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run-all apply -auto-approve --non-interactive --log-level trace --working-dir "+rootPath) require.NoError(t, err) +// Note: Deprecation warning check removed because <reason> assert.NotContains( // Check that we're getting a warning for usage of deprecated functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.circleci/config.yml(11 hunks)config/config.go(3 hunks)config/config_partial.go(1 hunks)docs/_docs/04_reference/04-config-blocks-and-attributes.md(1 hunks)internal/strict/control.go(1 hunks)internal/strict/controls/control.go(3 hunks)internal/strict/controls/controls.go(1 hunks)internal/strict/controls/deprecated_env_var.go(1 hunks)internal/strict/controls/deprecated_flag_name.go(1 hunks)test/fixtures/hooks/error-hooks/tf.sh(1 hunks)test/fixtures/render-json-metadata/attributes/terragrunt.hcl(1 hunks)test/helpers/package.go(1 hunks)test/integration_common_test.go(1 hunks)test/integration_json_test.go(2 hunks)test/integration_serial_test.go(3 hunks)test/integration_strict_test.go(2 hunks)test/integration_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (27)
config/config.go (3)
21-21: Nice addition of the controls import.No issues spotted with bringing in
internal/strict/controls. It's straightforward and aligns with the new usage below.
28-28: Importinghclsyntaxlooks good.Now you can parse HCL files more precisely and detect references with better granularity. This is a solid addition.
873-886: Clarify default behavior if the control is not set.This new block cleanly checks for the
SkipDependenciesInputscontrol. However, an error is thrown if the control is absent. If that’s the intended behavior (strict enforcement), all good. Otherwise, consider a fallback or an optional approach if you’d like looser handling.internal/strict/controls/deprecated_env_var.go (1)
88-88: Wise safeguard for suppressed warnings.Including
!ctrl.Suppressensures logs remain quiet when the control is intentionally muted. This aligns neatly with your new suppression logic.internal/strict/controls/deprecated_flag_name.go (1)
108-108: Good addition of the!ctrl.Suppresscheck.Your warning flow now accurately respects the suppression toggle, preventing unnecessary messages when warnings should be silenced.
internal/strict/controls/control.go (3)
45-46: LGTM! Clear and descriptive field comment.The new
Suppressfield is well-documented and its purpose is clear.
103-106: LGTM! Simple and effective implementation.The
SuppressWarningmethod provides a clean way to control warning visibility.
126-126: LGTM! Thread-safe warning suppression.The warning suppression check is correctly placed before the
OnceWarn.Docall, maintaining thread safety.test/integration_strict_test.go (2)
39-39: LGTM! Consistent error messages.The error messages are clear, consistent, and provide actionable guidance to users.
Also applies to: 46-46
55-55: LGTM! Simplified command-line arguments.Removed the
--terragrunt-prefix from flags, making the commands more concise.internal/strict/controls/controls.go (1)
58-59: LGTM! Clear and actionable messages.The error and warning messages are:
- Direct about the deprecation status
- Clear about what users should do instead
- Consistent with the PR's goal of improving warning messages
internal/strict/control.go (1)
50-52: LGTM! Clean interface extension.The new
SuppressWarningmethod is:
- Well-documented
- Focused on a single responsibility
- Consistent with the interface's style
test/integration_common_test.go (1)
363-363: LGTM! Environment variable name updated for consistency.The change from
TERRAGRUNT_TFPATHtoTG_TF_PATHaligns with the standardization of environment variable naming across the codebase.test/integration_json_test.go (3)
32-32: LGTM! Variable name follows Go naming convention.The change from
terragruntHcltoterragruntHCLfollows Go's standard naming convention for acronyms.
40-40: LGTM! Command flags simplified.Removing the redundant
--terragrunt-prefix from command flags makes the code cleaner while maintaining the same functionality.
117-117: LGTM! Improved error message clarity.Adding expected and actual values in the assertion message will make debugging test failures much easier.
config/config_partial.go (2)
461-464: LGTM! Improved error handling for missing control.The code now properly checks for missing control and returns a clear error message.
466-471: LGTM! Enhanced warning suppression and logging.The changes improve warning control by:
- Suppressing warnings when needed
- Using clearer log message wording
test/helpers/package.go (1)
638-638: LGTM! Environment variable name updated for consistency.The change from
TERRAGRUNT_TFPATHtoTG_TF_PATHaligns with the standardization of environment variable naming across the codebase.test/integration_serial_test.go (3)
235-235: LGTM! Command flag update looks good.The change from
--terragrunt-non-interactiveto--non-interactiveand--terragrunt-working-dirto--working-diraligns with the shorter flag names.
245-245: LGTM! Command flag update is consistent.The flag updates here match the changes in the previous command, maintaining consistency across the test.
258-263: LGTM! Good addition of deprecation warning check.The new assertion verifies that users are properly warned about the deprecated functionality of reading inputs from dependencies. This helps ensure a smooth transition as the feature will be removed in a future version.
test/integration_test.go (1)
3043-3043: LGTM! Command flag updates are consistent.The command flag updates match the changes made above, maintaining consistency throughout the test.
test/fixtures/hooks/error-hooks/tf.sh (1)
3-3:❓ Verification inconclusive
Environment variable and default value updates look good!
The changes standardize the environment variable name to
TG_TF_PATHand update the default value totofu, aligning with the OpenTofu migration.Let's verify if this environment variable is consistently used across the codebase:
🏁 Script executed:
#!/bin/bash # Search for any remaining instances of the old environment variable rg "TERRAGRUNT_TFPATH"Length of output: 355
Environment variable and default value updates confirmed!
The changes in
test/fixtures/hooks/error-hooks/tf.shcorrectly update the variable fromTERRAGRUNT_TFPATHtoTG_TF_PATHand switch the default fromterraformtotofu, aligning with the OpenTofu migration.However, I did notice that our docs still refer to
TERRAGRUNT_TFPATH(e.g., indocs-starlight/src/content/docs/04-reference/01-hcl/03-attributes.mdxanddocs/_docs/04_reference/02-cli-options.md) with a deprecation note. If this is intentional for backward compatibility, that’s fine—but if not, please update the documentation accordingly for consistency.Great job on the update!
test/fixtures/render-json-metadata/attributes/terragrunt.hcl (1)
18-18: Environment variable update is consistent!The change to use
TG_TF_PATHwith default valuetofumatches the pattern established in other files..circleci/config.yml (1)
33-33:❓ Verification inconclusive
CI configuration updates are thorough and consistent!
All CI jobs have been updated to use the new
TG_TF_PATHenvironment variable. The changes are consistent across different test configurations and environments.Let's verify that all CI jobs that need the environment variable have been updated:
Also applies to: 47-47, 99-99, 184-184, 208-208, 269-269, 290-291, 313-314, 336-337, 380-382, 535-536
🏁 Script executed:
#!/bin/bash # Search for any jobs that might still be using the old environment variable rg "TERRAGRUNT_TFPATH" .circleci/Length of output: 33
Everything Looks Updated, But Please Give It a Final Manual Look!
The switch from
TERRAGRUNT_TFPATHtoTG_TF_PATHappears spot on. Our automated search in the.circleci/directory didn’t find any leftover instances of the old variable, which suggests that all relevant CI jobs have been updated. Key points:
- Update at
.circleci/config.yml(e.g., line 33) and across the other noted locations (47, 99, 184, 208, 269, 290-291, 313-314, 336-337, 380-382, 535-536) look consistent.- The
rg "TERRAGRUNT_TFPATH" .circleci/command returned no output, implying no remnants of the old variable exist.That said, since automated checks can occasionally miss context, please manually verify the CI configurations to ensure nothing’s lurking under the hood.
docs/_docs/04_reference/04-config-blocks-and-attributes.md (1)
1177-1187: Documentation updates for mock outputs configuration are clear and comprehensive!The new
mock_outputs_merge_strategy_with_stateattribute is well documented with clear explanations of the available merge strategies (no_merge,shallow,deep_map_only). The deprecation notice formock_outputs_merge_with_stateis properly included.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/config.go (2)
873-885: Consider improving error handling and context consistency.The validation logic for deprecated dependency inputs is solid, but there are two areas for improvement:
- The error message when the control is not found could be more descriptive
- The context variable naming could be more consistent (using
evalCtxin one place andctxin another)Consider applying these changes:
- return nil, errors.New("failed to find control " + controls.SkipDependenciesInputs) + return nil, errors.New(fmt.Sprintf("strict control '%s' not found - this is required for validating dependency inputs", controls.SkipDependenciesInputs)) - evalCtx := log.ContextWithLogger(ctx, ctx.TerragruntOptions.Logger) - if err := skipDependenciesInputs.Evaluate(evalCtx); err != nil { + ctx = log.ContextWithLogger(ctx, ctx.TerragruntOptions.Logger) + if err := skipDependenciesInputs.Evaluate(ctx); err != nil {
959-991: Enhance documentation and constant naming.The implementation is solid, but could benefit from more detailed documentation and clearer constant naming.
Consider these improvements:
-// detectInputsCtyUsage detects if an identifier matching dependency.foo.inputs.bar is used in the given HCL file. -// -// This is deprecated functionality, so we look for this to determine if we should throw an error or warning. +// detectInputsCtyUsage scans the HCL file for deprecated dependency input references. +// +// This function looks for identifiers matching the pattern dependency.foo.inputs.bar, which represents the deprecated +// way of accessing dependency outputs. When found, this triggers strict mode validation to ensure proper migration +// to the new dependency output access pattern. +// +// Example of deprecated usage: +// - dependency.backend.inputs.bucket // deprecated +// - dependency.backend.outputs.bucket // new way func detectInputsCtyUsage(file *hclparse.File) bool { - const dependencyInputsIdentifierMinParts = 3 + const minPartsForDependencyInputs = 3 // minimum parts needed for "dependency.name.inputs"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (1)
config/config.go (1)
21-22: LGTM! Clean import additions.The new imports for
controlsandhclsyntaxpackages are well-organized and necessary for the new functionality.
yhakbar
deleted the
fix/fixing-excessive-warning-for-dependency-input
branch
Description
Fixes #3873
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated strict mode to allow for error suppression.
Summary by CodeRabbit
New Features
Documentation
Tests