fix: Avoiding panic on missing if_exists
#5072
Conversation
📝 WalkthroughWalkthroughAdded defensive nil-check in ParseConfig to prevent nil pointer dereference after handleInclude. Added validation requiring if_exists attribute on generate blocks, collecting errors during processing instead of returning immediately to enable comprehensive error reporting. Changes
Sequence DiagramsequenceDiagram
participant User
participant ParseConfig
participant handleInclude
participant convertToTerragruntConfig
participant validateGenerate
User->>ParseConfig: Parse config
ParseConfig->>handleInclude: Handle include
handleInclude-->>ParseConfig: return config or nil
alt Config is nil (NEW)
ParseConfig-->>User: Return current config + errors
else Config exists
ParseConfig->>convertToTerragruntConfig: Convert to terragrunt config
loop For each generate block
convertToTerragruntConfig->>validateGenerate: Check if_exists attribute (NEW)
alt if_exists missing
validateGenerate-->>convertToTerragruntConfig: Append error, continue
else if_exists present
validateGenerate-->>convertToTerragruntConfig: Generate value (NEW)
alt Generation fails (NEW)
convertToTerragruntConfig->>convertToTerragruntConfig: Append error, continue
else Generation succeeds
convertToTerragruntConfig->>convertToTerragruntConfig: Process normally
end
end
end
convertToTerragruntConfig-->>ParseConfig: Return config with accumulated errors
ParseConfig-->>User: Return TerragruntConfig + errors
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (1)
config/config_test.go (1)
1552-1571: Test correctly verifies the fix for missing if_exists attribute.The test validates that:
- An error is returned when
if_existsis missing- A non-nil config is returned (best-effort parsing)
- The error message mentions the missing attribute
Consider adding a complementary test case for an invalid
if_existsvalue (e.g.,if_exists = "invalid_value") to ensure that scenario is also handled gracefully:func TestParseConfigWithInvalidIfExists(t *testing.T) { t.Parallel() cfg := `generate "test" { path = "test.tf" contents = "foo" if_exists = "invalid_value" }` l := createLogger() ctx := config.NewParsingContext(t.Context(), l, mockOptionsForTest(t)) terragruntConfig, err := config.ParseConfigString(ctx, l, config.DefaultTerragruntConfigPath, cfg, nil) require.Error(t, err) assert.Contains(t, err.Error(), "generate") assert.NotNil(t, terragruntConfig) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(2 hunks)config/config_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/config_test.goconfig/config.go
🧬 Code graph analysis (2)
config/config_test.go (2)
config/parsing_context.go (1)
NewParsingContext(62-73)config/config.go (2)
ParseConfigString(1217-1230)DefaultTerragruntConfigPath(48-48)
config/config.go (1)
codegen/generate.go (1)
GenerateConfigExistsFromString(585-598)
⏰ 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). (12)
- GitHub Check: build_no_proxy / Build (windows/386)
- GitHub Check: build_no_proxy / Build (darwin/arm64)
- GitHub Check: build_no_proxy / Build (linux/amd64)
- GitHub Check: build_no_proxy / Build (linux/arm64)
- GitHub Check: build / Build (windows/amd64)
- GitHub Check: build / Build (windows/386)
- GitHub Check: build / Build (darwin/amd64)
- GitHub Check: build / Build (linux/amd64)
- GitHub Check: build / Build (darwin/arm64)
- GitHub Check: base_tests / Test (macos)
- GitHub Check: base_tests / Test (ubuntu)
- GitHub Check: build-and-test
🔇 Additional comments (2)
config/config.go (2)
1742-1751: Good error collection pattern for validation.The validation correctly enforces the required
if_existsattribute and collects all errors before returning. This improves user experience by showing all validation errors at once rather than failing on the first error.The use of
continueinstead of immediate return allows processing to continue and collect errors from all generate blocks.
1338-1343: The defensive nil-check is appropriate and necessary; handleInclude legitimately returns nil on merge errors.The verification shows that
handleIncludereturns(nil, error)in three error conditions (lines 158, 166, 171 in config/include.go), all related to failures during merge operations. The nil-check at lines 1338-1343 correctly handles this designed behavior: when a merge fails, the function signals failure by returning nil, and the caller appropriately stops processing and returns accumulated errors.This is proper error handling, not a symptom treatment—the merge failures are being correctly propagated up the call stack.
Description
Fixes #5071
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Bug Fixes
Tests