feat: improve disabled dependency management #5155
Conversation
loispostula
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
@loispostula is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis pull request adds guards to skip empty string and "." dependency paths in both the discovery and runner phases of dependency resolution, and implements logic to skip disabled dependencies. These changes prevent invalid or placeholder paths from being processed during Terragrunt configuration dependency extraction and unit graph construction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.go (1)
241-252: Consider canonicalizing paths for more robust comparison.The current implementation compares
dependencyPath(fromDependencies.Paths) directly withdep.ConfigPath.AsString()(fromTerragruntDependencies). If the same dependency is specified in different path formats (e.g.,"../foo"vs"${get_terragrunt_dir()}/../foo"), the comparison might fail to match.While the test cases suggest same-format usage is expected, canonicalizing both paths before comparison would make this more robust:
func (unit *Unit) isDisabledDependency(dependencyPath string) bool { + canonicalDepPath, err := util.CanonicalPath(dependencyPath, unit.Path) + if err != nil { + return false + } for _, dep := range unit.Config.TerragruntDependencies { if dep.Enabled != nil && !*dep.Enabled { depPath := dep.ConfigPath.AsString() + canonicalConfigPath, err := util.CanonicalPath(depPath, unit.Path) + if err != nil { + continue + } - if depPath == dependencyPath { + if canonicalConfigPath == canonicalDepPath { return true } } } return false }internal/runner/common/unit_test.go (1)
150-172: LGTM! Test correctly validates disabled dependency skipping.The test properly exercises the fix by creating a scenario where a unit has both an enabled dependency (fooPath) and a disabled dependency (disabledDepPath), then verifying that only the enabled dependency is included after conversion.
Optional enhancement: Consider adding more specific assertions to make test failures clearer:
units, err := m.ConvertDiscoveryToRunner([]string{fooPath, barPath}) require.NoError(t, err) - assert.Len(t, units[0].Dependencies, 1) + require.Len(t, units, 2) + // units[0] is barPath (alphabetically first) + assert.Len(t, units[0].Dependencies, 1, "bar should have only 1 dependency") + assert.Equal(t, fooPath, units[0].Dependencies[0].Path, "bar should depend on foo only")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/discovery/discovery.go(2 hunks)internal/discovery/discovery_test.go(2 hunks)internal/runner/common/unit.go(3 hunks)internal/runner/common/unit_test.go(2 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:
internal/discovery/discovery_test.gointernal/discovery/discovery.gointernal/runner/common/unit_test.gointernal/runner/common/unit.go
🧠 Learnings (3)
📚 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:
internal/discovery/discovery.gointernal/runner/common/unit_test.go
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
internal/runner/common/unit_test.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
internal/runner/common/unit_test.go
🧬 Code graph analysis (2)
internal/discovery/discovery_test.go (4)
internal/errors/export.go (1)
Join(17-19)options/options.go (1)
NewTerragruntOptions(380-382)test/helpers/logger/logger.go (1)
CreateLogger(9-14)internal/discovery/constructor.go (1)
NewDiscovery(153-172)
internal/runner/common/unit_test.go (2)
internal/runner/common/unit.go (2)
UnitsMap(57-57)Unit(26-36)config/config.go (2)
TerragruntConfig(140-166)ModuleDependencies(796-798)
🔇 Additional comments (6)
internal/runner/common/unit.go (2)
207-210: LGTM! Good defensive check for conditional dependencies.The guard correctly handles empty or current-directory paths that can occur when conditional dependencies are disabled or resolve to non-existent paths. This aligns with the similar checks added in discovery.go.
222-225: Correctly implements the disabled dependency check.This properly addresses the PR objective by checking if a missing dependency is disabled before returning an error. The logic correctly skips disabled dependencies instead of throwing UnrecognizedDependencyError.
internal/discovery/discovery.go (1)
1390-1392: LGTM! Consistent guards prevent invalid paths from being processed.The guards correctly filter out empty or current-directory paths for both
TerragruntDependenciesandDependencies.Paths, ensuring only valid dependency paths enter the deduped set. This prevents issues during subsequent dependency resolution and aligns with the similar guards added in the runner layer.Also applies to: 1402-1404
internal/runner/common/unit_test.go (1)
9-9: LGTM! Import is necessary for test setup.The
ctyimport is required to constructConfigPathvalues usingcty.StringValin the new test case.internal/discovery/discovery_test.go (2)
1003-1010: LGTM! More realistic test setup using HCL locals.The change to compute the
enabledflag via a local variable makes the test more realistic and better exercises the HCL evaluation path. The test still correctly validates that disabled dependencies don't create cycles.
1045-1128: LGTM! Comprehensive test for conditional dependency paths.This test thoroughly validates the core PR functionality with a realistic multi-environment setup:
- Uses
fileexists()to conditionally enable dependencies based on whether the dependency exists- Tests that empty or non-existent
config_pathvalues are properly skipped during discovery- Verifies that conditional dependencies don't cause false cycle detection
- Covers multiple environment-specific components (prod/dev) with both shallow and deep merge strategies
The test provides strong coverage for the empty path skipping logic introduced in this PR.
|
@denis256 I'm not sure what's missing on this pr? |
|
Hey @loispostula , I'm not sure this is the right solution for this. Should dependencies with There are also conflicts in your branch that need to be resolved before review. |
2 participants
Description
Fixes #2734
Fixes #2927
Fix runner validation to skip disabled dependencies when resolving unit dependencies. Previously, when a dependency block had
enabled = falsebut a valid config_path, the path was still validated against discovered units, causingUnrecognizedDependencyError. Now getDependenciesForUnit checks TerragruntDependencies for the enabled flag before erroring on missing paths.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated disabled dependency management
Migration Guide
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.