chore: autoinclude merge fixes - #6248
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds stack-kind autoinclude validation and typed errors; merges sibling autoincludes into unit/stack parsing (full and partial), folds autoinclude dependencies into dependency decoding, rebuilds per-stack-dir HCL function maps for discovery with recursion/cycle guards, and expands tests/fixtures and changelog. ChangesStack-dependencies autoinclude parsing and discovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
"""
Which would you prefer? ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
1397-1410:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReturn early when config conversion failed.
Line 1404 assumes
configis non-nil, butconvertToTerragruntConfighas severalreturn nil, errpaths in this file. If one of those fires and a sibling autoinclude exists, this turns a normal parse error into a nil-pointer panic incfg.Merge(...). Bail out before the autoinclude/include merge phases when conversion produced no config.Proposed fix
config, err := convertToTerragruntConfig(ctx, pctx, file.ConfigPath, terragruntConfigFile) if err != nil { errs = append(errs, err) + if config == nil { + return nil, errors.Join(errs...) + } } // Auto-merge the unit-level terragrunt.autoinclude.hcl if present in the same directory; stack-level terragrunt.autoinclude.stack.hcl is handled by the stack parser path. // Only replace config on success; the merge helper returns nil on failure and handleInclude below would nil-deref it. merged, autoMergeErr := mergeAutoIncludeIfPresent(ctx, pctx, l, config, file.ConfigPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config.go` around lines 1397 - 1410, The code assumes convertToTerragruntConfig returned a non-nil config and proceeds to call mergeAutoIncludeIfPresent/ cfg.Merge, which can nil-deref when convertToTerragruntConfig returned (nil, err); add an early return/continue when err != nil or config == nil: after calling convertToTerragruntConfig (symbol: convertToTerragruntConfig) if err != nil or config == nil, append the error(s) as already done and skip the autoinclude/merge steps (do not call mergeAutoIncludeIfPresent or perform any cfg.Merge on a nil config), so mergeAutoIncludeIfPresent and later use of config (variable: config) are only reached when config is non-nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/config/config.go`:
- Around line 1397-1410: The code assumes convertToTerragruntConfig returned a
non-nil config and proceeds to call mergeAutoIncludeIfPresent/ cfg.Merge, which
can nil-deref when convertToTerragruntConfig returned (nil, err); add an early
return/continue when err != nil or config == nil: after calling
convertToTerragruntConfig (symbol: convertToTerragruntConfig) if err != nil or
config == nil, append the error(s) as already done and skip the
autoinclude/merge steps (do not call mergeAutoIncludeIfPresent or perform any
cfg.Merge on a nil config), so mergeAutoIncludeIfPresent and later use of config
(variable: config) are only reached when config is non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78c71613-6a19-4cca-8217-455b482bb6ab
📒 Files selected for processing (9)
docs/src/data/changelog/v1.0.8/stack-dependencies-autoinclude-merge.mdxpkg/config/autoinclude_test.gopkg/config/config.gopkg/config/config_partial.gopkg/config/dependency.gopkg/config/stack.gotest/fixtures/stacks/stack-deps-dep-mock-merge/catalog/y/terragrunt.hcltest/fixtures/stacks/stack-deps-dup-dependency/catalog/y/terragrunt.hcltest/integration_stack_dependencies_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/src/data/changelog/v1.0.8/stack-dependencies-autoinclude-merge.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- test/fixtures/stacks/stack-deps-dup-dependency/catalog/y/terragrunt.hcl
- pkg/config/dependency.go
- pkg/config/stack.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config.go (1)
2304-2326:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse deep-merge semantics for the autoinclude override.
This path still documents and applies the sibling autoinclude as a default/shallow merge. That means nested maps and blocks are replaced instead of recursively merged, so dependency-backed fields can still disappear when both the unit and
terragrunt.autoinclude.hcldefine the same parent block. That breaks the full-parse leg of the behavior this PR is supposed to align across phases.Please force the override through the deep-merge path here (for example by applying
DeepMergesemantics beforecfg.Mergeor by calling the explicit deep-merge helper instead).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/config.go` around lines 2304 - 2326, The autoinclude is currently merged using shallow semantics via cfg.Merge in mergeAutoIncludeIfPresent; change this to use the deep-merge path so nested maps/blocks are merged recursively (preventing dependent fields from being clobbered). Locate mergeAutoIncludeIfPresent and replace the shallow merge step (the cfg.Merge(l, autoIncludeConfig) call) with the deep-merge semantics used elsewhere (e.g., call the existing DeepMerge helper or perform DeepMerge on cfg with autoIncludeConfig, then assign the result back to cfg) while preserving the existing clonedPctx/DecodedDependencies handling and error wrapping.
🧹 Nitpick comments (1)
test/fixtures/stacks/stack-deps-nested-unit-dep/units/vpc/main.tf (1)
1-4: ⚡ Quick winMake this fixture input required.
default = []lets the module stay valid even when the nested dependency/autoinclude wiring fails, which weakens the fixture's ability to catch the regression this PR is exercising. Removing the default will make the test fail fast ifavailability_zonesis not actually propagated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/fixtures/stacks/stack-deps-nested-unit-dep/units/vpc/main.tf` around lines 1 - 4, The variable block for availability_zones currently sets a default ([]), which makes the input optional; remove the default from the variable "availability_zones" declaration so the input becomes required (i.e., keep the variable "availability_zones" block but delete the default = [] line) to ensure tests fail fast when the value is not propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/config/config.go`:
- Around line 2304-2326: The autoinclude is currently merged using shallow
semantics via cfg.Merge in mergeAutoIncludeIfPresent; change this to use the
deep-merge path so nested maps/blocks are merged recursively (preventing
dependent fields from being clobbered). Locate mergeAutoIncludeIfPresent and
replace the shallow merge step (the cfg.Merge(l, autoIncludeConfig) call) with
the deep-merge semantics used elsewhere (e.g., call the existing DeepMerge
helper or perform DeepMerge on cfg with autoIncludeConfig, then assign the
result back to cfg) while preserving the existing clonedPctx/DecodedDependencies
handling and error wrapping.
---
Nitpick comments:
In `@test/fixtures/stacks/stack-deps-nested-unit-dep/units/vpc/main.tf`:
- Around line 1-4: The variable block for availability_zones currently sets a
default ([]), which makes the input optional; remove the default from the
variable "availability_zones" declaration so the input becomes required (i.e.,
keep the variable "availability_zones" block but delete the default = [] line)
to ensure tests fail fast when the value is not propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14e5ae2f-930f-4e68-a268-95c0fc1c2bfa
📒 Files selected for processing (14)
pkg/config/config.gopkg/config/config_helpers.gopkg/config/config_partial.gopkg/config/dependency.gotest/fixtures/stacks/stack-deps-locals-readconfig-dep/live/terragrunt.stack.hcltest/fixtures/stacks/stack-deps-nested-remote-state-dep/catalog/units/roles/terragrunt.hcltest/fixtures/stacks/stack-deps-nested-unit-dep/live/terragrunt.stack.hcltest/fixtures/stacks/stack-deps-nested-unit-dep/stacks/core/terragrunt.stack.hcltest/fixtures/stacks/stack-deps-nested-unit-dep/units/data/main.tftest/fixtures/stacks/stack-deps-nested-unit-dep/units/data/terragrunt.hcltest/fixtures/stacks/stack-deps-nested-unit-dep/units/vpc/main.tftest/fixtures/stacks/stack-deps-nested-unit-dep/units/vpc/terragrunt.hcltest/fixtures/stacks/stack-deps-stack-autoinclude-dep-values/live/terragrunt.stack.hcltest/integration_stack_dependencies_test.go
✅ Files skipped from review due to trivial changes (4)
- test/fixtures/stacks/stack-deps-nested-unit-dep/units/vpc/terragrunt.hcl
- test/fixtures/stacks/stack-deps-nested-unit-dep/units/data/terragrunt.hcl
- test/fixtures/stacks/stack-deps-nested-unit-dep/stacks/core/terragrunt.stack.hcl
- test/fixtures/stacks/stack-deps-nested-unit-dep/live/terragrunt.stack.hcl
🚧 Files skipped from review as they are similar to previous changes (6)
- test/fixtures/stacks/stack-deps-locals-readconfig-dep/live/terragrunt.stack.hcl
- test/fixtures/stacks/stack-deps-nested-remote-state-dep/catalog/units/roles/terragrunt.hcl
- pkg/config/config_partial.go
- test/fixtures/stacks/stack-deps-stack-autoinclude-dep-values/live/terragrunt.stack.hcl
- pkg/config/dependency.go
- test/integration_stack_dependencies_test.go
Description
dependency.*outputs now resolve inside remote_state, same-name unit-vs-autoinclude conflicts merge by nameFixes #6244.
RFC: #5663
TODOs
Read the Gruntwork contribution guidelines.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation