Changed wording to install external dependency #5154
Conversation
Signed-off-by: Babur Ayanlar <[email protected]>
baa-ableton
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
@baa-ableton is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRenames external-dependency handling from "apply" to "run": function and variable names, prompt text, comments, and related flags updated to use "run" semantics; a test assertion updated to match the new prompt wording. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-08-19T16:05:54.723ZApplied to files:
🔇 Additional comments (1)
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 |
![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)
internal/runner/common/unit_resolver_dependencies.go (2)
23-59: Update variable and comment to match new terminology.While the method call has been correctly updated to
confirmShouldInstallExternalDependency, there are two minor inconsistencies:
- The variable
shouldApply(line 23) still uses the old "apply" terminology- The comment on line 54 says "shouldn't be applied" instead of "shouldn't be installed"
For consistency with the renamed method and updated prompt wording, consider updating these as well.
Apply this diff:
- shouldApply := false + shouldInstall := false if !r.Stack.TerragruntOptions.IgnoreExternalDependencies { // Find a unit that depends on this external dependency for context var dependentUnit *Unit for _, u := range unitsMap { for _, dep := range u.Dependencies { if dep.Path == unit.Path { dependentUnit = u break } } if dependentUnit != nil { break } } // If we found a dependent, ask the user if dependentUnit != nil { var err error - shouldApply, err = r.confirmShouldInstallExternalDependency(ctx, dependentUnit, l, unit, unit.TerragruntOptions) + shouldInstall, err = r.confirmShouldInstallExternalDependency(ctx, dependentUnit, l, unit, unit.TerragruntOptions) if err != nil { return err } } } - unit.AssumeAlreadyApplied = !shouldApply - // Mark external dependencies as excluded if they shouldn't be applied + unit.AssumeAlreadyApplied = !shouldInstall + // Mark external dependencies as excluded if they shouldn't be installed // This ensures they are tracked in the report but not executed - if !shouldApply { + if !shouldInstall { unit.FlagExcluded = true }Note: The field name
AssumeAlreadyAppliedalso uses the old terminology, but renaming it would require broader changes across the codebase and is beyond the scope of this PR.
64-68: Update comment to use consistent terminology.The comment on line 65 still contains "already applied" which should be "already installed" to match the renamed function and updated prompt wording.
Apply this diff:
// Confirm with the user whether they want Terragrunt to assume the given dependency of the given unit is already -// applied. If the user selects "yes", then Terragrunt will install that unit as well. +// installed. If the user selects "yes", then Terragrunt will install that unit as well. // Note that we skip the prompt for `run --all destroy` calls. Given the destructive and irreversible nature of destroy, we don't // want to provide any risk to the user of accidentally destroying an external dependency unless explicitly included // with the --queue-include-external or --queue-include-dir flags.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/runner/common/unit_resolver_dependencies.go(4 hunks)test/integration_destroy_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
test/integration_destroy_test.gointernal/runner/common/unit_resolver_dependencies.go
🧠 Learnings (1)
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
test/integration_destroy_test.go
🧬 Code graph analysis (1)
internal/runner/common/unit_resolver_dependencies.go (4)
options/options.go (1)
TerragruntOptions(99-329)internal/runner/common/unit_resolver.go (1)
UnitResolver(47-53)internal/runner/common/unit.go (1)
Unit(26-36)shell/prompt.go (1)
PromptUserForYesNo(65-77)
🔇 Additional comments (4)
internal/runner/common/unit_resolver_dependencies.go (3)
12-12: LGTM: Comment updated appropriately.The comment correctly reflects the new "install" terminology, which addresses the PR objective of avoiding the misleading "apply" wording.
69-69: LGTM: Function signature updated correctly.The function has been appropriately renamed from
confirmShouldApplyExternalDependencytoconfirmShouldInstallExternalDependency, maintaining the same signature and preserving all functionality.
88-88: Excellent fix for the misleading prompt wording.This change directly addresses issue #4828 by replacing "Should Terragrunt apply the external dependency?" with "Should Terragrunt install the external dependency?". The new wording is more accurate because:
- It doesn't imply that
terraform applywill be executed when runningplanoperations- "Install" is more generic and correctly conveys that the dependency will be processed with whatever command is being run (plan, apply, etc.)
test/integration_destroy_test.go (1)
345-345: LGTM: Test assertion correctly updated.The test assertion has been properly updated to reflect the new prompt wording. This test verifies that the prompt is NOT shown during destroy operations, which is the expected behavior per the logic in
unit_resolver_dependencies.go(lines 80-84).
… installing external dependencies Signed-off-by: Babur Ayanlar <[email protected]>
|
Hey @baa-ableton ! Do you mind changing the verbiage to "run" instead of "install"? That's the standard term we use when Terragrunt is invoking OpenTofu/Terraform on behalf of users. |
Signed-off-by: Babur Ayanlar <[email protected]>
|
@yhakbar I replaced all instances once again, however this also means instances of apply that are meant for developers reading the source code as well as user facing prompt lines have been changed. Pls lmk if you would like the 'apply' verbiage back for the internal instances for source code readability |
|
Looks good @baa-ableton ! Would you be willing to rebase this PR? |
2 participants
Description
Fixes #4828 .
It was reported that the prompt was misleading in its wording. 'Apply' for external deps has been replaced with 'install'.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.