prevent overwriting Exclude config when not set in current config #5232
Conversation
HeikoNeblung
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
@HeikoNeblung is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe change modifies the config parsing logic to conditionally preserve the Exclude block from an included configuration file. Previously, any Exclude block from an included config would be unconditionally merged into the current config; now it is only applied if the current config has not already defined an Exclude block. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config.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/config.go
🧬 Code graph analysis (1)
config/config.go (1)
cli/commands/find/cli.go (1)
Exclude(27-27)
|
|
||
| // preserve included Exclude config when no Exclude config is present in current config | ||
| if config.Exclude != nil { | ||
| mergedConfig.Exclude = config.Exclude | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the code around lines 1353-1357 to understand the full context
cat -n config/config.go | sed -n '1340,1365p'Repository: gruntwork-io/terragrunt
Length of output: 1391
🏁 Script executed:
# Run the verification script to examine handleInclude and FieldsMetadata behavior
#!/bin/bash
set -euo pipefail
echo "=== Locate handleInclude implementation ==="
rg -n 'func\s+handleInclude' config/ -A 15 || true
echo
echo "=== Find where Exclude is merged/overridden ==="
rg -n 'Exclude\s*=' config/config.go | head -20
echo
echo "=== Find FieldsMetadata operations ==="
rg -n 'FieldsMetadata|SetFieldMetadata' config/config.go | head -30
echo
echo "=== Look for tests with include/exclude ==="
find . -name '*test.go' -type f | head -10Repository: gruntwork-io/terragrunt
Length of output: 5715
🏁 Script executed:
# Check if SetFieldMetadata method exists
rg -n 'func.*SetFieldMetadata' . --type goRepository: gruntwork-io/terragrunt
Length of output: 436
🏁 Script executed:
# Get the full handleInclude implementation
cat -n config/include.go | sed -n '116,183p'Repository: gruntwork-io/terragrunt
Length of output: 3241
🏁 Script executed:
# Check the merge strategy implementation
rg -n 'GetMergeStrategy|mergeStrategy' config/include.go -A 5 -B 2 | head -60Repository: gruntwork-io/terragrunt
Length of output: 2033
🏁 Script executed:
# Search for exclude-related tests
find . -name '*test.go' -type f | xargs rg -l 'exclude' -i | head -10Repository: gruntwork-io/terragrunt
Length of output: 389
🏁 Script executed:
# Look at the actual merge function to see how metadata is handled
rg -n 'func.*merge|mergeWith' config/include.go -A 10 | head -80Repository: gruntwork-io/terragrunt
Length of output: 3336
🏁 Script executed:
# Find Merge and DeepMerge implementations
rg -n 'func.*\(.*TerragruntConfig\).*Merge' config/ -A 20 | head -120Repository: gruntwork-io/terragrunt
Length of output: 2357
🏁 Script executed:
# Look specifically for how Exclude is handled in merge operations
rg -n 'Exclude' config/config.go -B 2 -A 2Repository: gruntwork-io/terragrunt
Length of output: 2275
🏁 Script executed:
# Check if FieldsMetadata is copied during merge
rg -n 'FieldsMetadata' config/config.go -B 1 -A 1 | head -60Repository: gruntwork-io/terragrunt
Length of output: 826
🏁 Script executed:
# Get complete Merge implementation
sed -n '242,363p' config/include.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 5193
🏁 Script executed:
# Get complete DeepMerge implementation
sed -n '364,500p' config/include.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 5204
🏁 Script executed:
# Check if Exclude is handled in Merge or DeepMerge
rg -n 'Exclude' config/include.goRepository: gruntwork-io/terragrunt
Length of output: 740
🏁 Script executed:
# Get the end of DeepMerge function
sed -n '364,600p' config/include.go | tail -100Repository: gruntwork-io/terragrunt
Length of output: 3084
🏁 Script executed:
# Find CopyFieldsMetadata implementation
rg -n 'func.*CopyFieldsMetadata' config/ -A 30Repository: gruntwork-io/terragrunt
Length of output: 3260
🏁 Script executed:
# Check if there are tests for exclude + include scenarios
find . -name '*test.go' -type f -exec rg -l 'exclude.*include|include.*exclude' {} \; | head -5Repository: gruntwork-io/terragrunt
Length of output: 258
🏁 Script executed:
# Search for tests related to exclude and include
rg -n 'exclude.*include|Exclude.*Include' test/ config/ -i | grep -i testRepository: gruntwork-io/terragrunt
Length of output: 1125
🏁 Script executed:
# Look for integration tests that might test this scenario
rg -n 'func.*Test.*Exclude|func.*Test.*Include' test/ -A 5 | head -100Repository: gruntwork-io/terragrunt
Length of output: 6732
🏁 Script executed:
# Check integration_regressions_test.go which was found
rg -n 'exclude|Exclude' test/integration_regressions_test.go -B 2 -A 5 | head -80Repository: gruntwork-io/terragrunt
Length of output: 3563
🏁 Script executed:
# Search for uses of FieldsMetadata and GetFieldMetadata related to exclude
rg -n 'GetFieldMetadata|FieldsMetadata\[.*exclude' config/ -i -C 3Repository: gruntwork-io/terragrunt
Length of output: 804
🏁 Script executed:
# Check if metadata is used for validation or error reporting
rg -n 'FoundInFile|FieldsMetadata' config/ -B 2 -A 2 | grep -A 10 -B 10 'exclude\|Exclude' | head -80Repository: gruntwork-io/terragrunt
Length of output: 49
🏁 Script executed:
# Look for any code that validates or uses metadata for exclude specifically
rg -n 'exclude' config/config.go | grep -i metadataRepository: gruntwork-io/terragrunt
Length of output: 118
🏁 Script executed:
# Find where defaultMetadata is created/defined
rg -n 'defaultMetadata\s*:=' config/ -B 5 -A 2Repository: gruntwork-io/terragrunt
Length of output: 488
🏁 Script executed:
# Look for FoundInFile constant/usage
rg -n 'FoundInFile' config/Repository: gruntwork-io/terragrunt
Length of output: 282
🏁 Script executed:
# Check the full context of lines around 1713 to see how defaultMetadata is built
sed -n '1700,1720p' config/config.go | cat -nRepository: gruntwork-io/terragrunt
Length of output: 986
Ensure exclude field metadata is updated when child config overrides the included value.
The override at lines 1355–1356 correctly preserves the included exclude when the child omits it. However, the FieldsMetadata[exclude] is not updated, leaving it pointing to the included file instead of the current config file. This causes metadata to be inconsistent with the value's origin, which can lead to confusing error messages and incorrect file attribution in logs.
Add a SetFieldMetadata call after the override to keep metadata in sync:
// preserve included Exclude config when no Exclude config is present in current config
if config.Exclude != nil {
mergedConfig.Exclude = config.Exclude
+ mergedConfig.SetFieldMetadata(MetadataExclude, map[string]any{FoundInFile: config.ConfigPath})
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In config/config.go around lines 1353 to 1357, when preserving the included
Exclude value for a child that omits it the code assigns mergedConfig.Exclude =
config.Exclude but does not update FieldsMetadata; update metadata immediately
after the assignment by calling mergedConfig.SetFieldMetadata for the "exclude"
field using the current child config's metadata (e.g., the child's file path or
its FieldsMetadata entry for "exclude") so the merged metadata reflects the
child config as the origin.
|
Hi @yhakbar i had to rebase my branch to fix the tests on macos. Unfortunately the original PR was closed by this. Can you please run the CI again on your site. The base-test was sucessful in my forked repo. |
1 participant
Description
Fixes #5089
Exclude blocks defined in parent configuration files will be merged in the resulting config even when no exclude block is present in current configuration.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
fixed handling of included exclude blocks
Migration Guide
n/a
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.