fix: shallow merge configuration fixes #5161
Conversation
… / exclude_from_copy
📝 WalkthroughWalkthroughImplements shallow-merge support for Terraform Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 (4)
config/include.go (1)
665-683: mergeListArgPreserveEmpty correctly distinguishes nil vs explicit empty listsThe helper cleanly encodes the desired behavior:
childListArg == nil→ no-op (inherit parent).len(*childListArg) == 0→ set a non-nil empty slice (explicit clear).- Non-empty child list → override with a copied slice (no aliasing to child).
Implementation is sound and pointer-safe; no changes required.
config/include_test.go (2)
153-182: Shallow-merge tests thoroughly cover copy-filter edge casesThe added cases exercise all key scenarios for
IncludeInCopy/ExcludeFromCopyunder shallow merge (child-only, parent-only, both-set, explicit clears, and parent-without-lists). They align with the newmergeListArgPreserveEmptybehavior and should prevent regressions.
334-369: Deep-merge tests validate concatenation semantics and duplicatesThese cases accurately encode current DeepMerge behavior: parent vs child list combinations are concatenated in the expected order, nil vs empty lists behave correctly, and duplicates are intentionally preserved. Comments are clear enough, despite the slightly confusing “parent/child” wording relative to
source/target.test/integration_regressions_test.go (1)
444-521: Integration test robustly covers shallow merge copy-filter regression
TestShallowMergeCopyFiltersexercises both the “parent nil, child set” and “both set, child wins” cases viaterragrunt render --json, asserting onterraform.source,exclude_from_copy, andinclude_in_copy. The structure and assertions closely mirror the reported bug, so this should effectively guard against regressions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/include.go(2 hunks)config/include_test.go(2 hunks)test/fixtures/regressions/shallow-merge-copy-filters-4757/app/terragrunt.hcl(1 hunks)test/fixtures/regressions/shallow-merge-copy-filters-4757/both-set/terragrunt.hcl(1 hunks)test/fixtures/regressions/shallow-merge-copy-filters-4757/shared/parent-with-filters.hcl(1 hunks)test/fixtures/regressions/shallow-merge-copy-filters-4757/shared/parent.hcl(1 hunks)test/integration_regressions_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:
config/include.goconfig/include_test.gotest/integration_regressions_test.go
🧠 Learnings (2)
📚 Learning: 2025-02-10T23:20:04.295Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 3868
File: docs-starlight/patches/@astrojs%[email protected]:33-33
Timestamp: 2025-02-10T23:20:04.295Z
Learning: In Terragrunt projects, all `.hcl` files can be assumed to be Terragrunt configurations by default, with specific exceptions like `.terraform.lock.hcl` that need explicit handling.
Applied to files:
test/fixtures/regressions/shallow-merge-copy-filters-4757/both-set/terragrunt.hcltest/fixtures/regressions/shallow-merge-copy-filters-4757/app/terragrunt.hcl
📚 Learning: 2025-11-03T04:40:01.000Z
Learnt from: ThisGuyCodes
Repo: gruntwork-io/terragrunt PR: 5041
File: test/fixtures/hclvalidate/valid/duplicate-attributes-in-required-providers/main.tf:2-7
Timestamp: 2025-11-03T04:40:01.000Z
Learning: In the terragrunt repository, test fixtures under test/fixtures/hclvalidate/valid/ are intentionally testing that INPUT validation succeeds even when Terraform code contains syntax errors or other issues unrelated to input validation (e.g., duplicate attributes, circular references, invalid locals). The "valid" designation means "valid for input validation purposes" not "syntactically valid Terraform/OpenTofu code".
Applied to files:
config/include_test.gotest/integration_regressions_test.go
🧬 Code graph analysis (1)
test/integration_regressions_test.go (2)
test/helpers/package.go (3)
CleanupTerraformFolder(882-889)CopyEnvironment(89-105)RunTerragruntCommand(965-969)util/file.go (1)
JoinPath(626-628)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (5)
test/fixtures/regressions/shallow-merge-copy-filters-4757/shared/parent.hcl (1)
1-3: Fixture parent.hcl correctly defines shared module sourceSource path matches the integration test expectations for the shallow-merge copy-filters regression; no issues.
config/include.go (1)
308-310: Shallow merge now correctly preserves/overrides copy filtersUsing
mergeListArgPreserveEmptyforIncludeInCopyandExcludeFromCopyinMergefixes the shallow-merge bug: child-provided lists (including explicit empties) now override parent lists without concatenation, matching the intended semantics for issue #4757.test/fixtures/regressions/shallow-merge-copy-filters-4757/shared/parent-with-filters.hcl (1)
2-6: Parent-with-filters fixture correctly defines source and copy filtersSource and parent copy filters are well-formed and match the regression test scenarios for “both_set_child_wins”; nothing to change here.
test/fixtures/regressions/shallow-merge-copy-filters-4757/both-set/terragrunt.hcl (1)
1-8: Child fixture for “both_set” correctly overrides parent filtersThe include path and child terraform copy filters line up with the integration test’s “both_set_child_wins” expectations; configuration is minimal and appropriate for the regression.
test/fixtures/regressions/shallow-merge-copy-filters-4757/app/terragrunt.hcl (1)
2-9: App fixture correctly models parent-without-filters scenarioThe include and terraform blocks accurately represent the “parent Terraform present, child sets filters” case targeted by the regression test; no adjustments needed.
ThisGuyCodes
left a comment
ThisGuyCodes
left a comment
There was a problem hiding this comment.
either the name + docstring is wrong, or the implimentation needs to be changed to do an actual merge
| func mergeListArgPreserveEmpty(childListArg *[]string, parentListArg **[]string) { | ||
| if childListArg == nil { | ||
| return | ||
| } | ||
|
|
||
| if len(*childListArg) == 0 { | ||
| empty := make([]string, 0) | ||
| *parentListArg = &empty | ||
|
|
||
| return | ||
| } | ||
|
|
||
| copied := append([]string(nil), *childListArg...) | ||
| *parentListArg = &copied | ||
| } |
There was a problem hiding this comment.
Either this function is wrong, or the name + docstring is. This doesn't merge the two lists under any circumstances; it either:
- replaces the parent list with a new empty list if the child list is empty (couldn't we just use the existing one instead of allocating a new one?)
- replaces the parent list with the non-empty child list
- leaves the parent list alone if the child list is nil
none of these are a "merge"
3 participants
Description
Based on PR: #4758
Fixes #4757.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Bug Fixes
include_in_copyandexclude_from_copy). Child configurations now properly override parent settings instead of being ignored, and explicit empty filters are now respected to clear parent values.Tests
✏️ Tip: You can customize this high-level summary in your review settings.