bug: discovery parsing errors handling #5037
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an early deprecated-configuration detection to partial parsing, refines discovery error-handling to optionally skip include-only/no-settings configs when suppression is enabled, and introduces fixtures and tests validating deprecated dependency-input detection and run-all with generate/expose scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PartialParseConfig
participant DetectDeprecatedConfigurations
participant ParsingLogic
Caller->>PartialParseConfig: Call PartialParseConfig()
PartialParseConfig->>DetectDeprecatedConfigurations: Run early detection
alt Deprecated syntax detected
DetectDeprecatedConfigurations-->>PartialParseConfig: Return deprecated-config error
PartialParseConfig-->>Caller: Return error (stop)
else No deprecated syntax
DetectDeprecatedConfigurations-->>PartialParseConfig: OK
PartialParseConfig->>ParsingLogic: Proceed with decode/includes
ParsingLogic-->>PartialParseConfig: Parsed partial config
PartialParseConfig-->>Caller: Return config
end
sequenceDiagram
participant Discover
participant Parse
participant PartialParseConfig
participant containsNoSettingsError
Discover->>Parse: Call Parse(path, suppressParseErrors)
Parse->>PartialParseConfig: Call PartialParseConfig(path)
alt PartialParseConfig returns error
PartialParseConfig-->>Parse: Return error
Parse->>containsNoSettingsError: Check error for "no settings" (when suppression enabled)
alt containsNoSettingsError == true && suppressParseErrors
containsNoSettingsError-->>Parse: True
Parse-->>Discover: Log debug, return nil (skip include-only config)
else
Parse-->>Discover: Return error (propagate)
end
else PartialParseConfig succeeds
PartialParseConfig-->>Parse: Return config
Parse-->>Discover: Return parsed config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
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 |
yhakbar
previously approved these changes
Oct 30, 2025
internal/discovery/discovery.go
Outdated
| if suppressParseErrors && containsNoSettingsError(err) { | ||
| l.Debugf("Skipping include-only config during discovery: %s", parseOpts.TerragruntConfigPath) | ||
|
|
||
| // Store an empty partial config to avoid nil dereferences in subsequent dependency discovery |
There was a problem hiding this comment.
This is odd to me. Why don't we just nil check later?
yhakbar
previously approved these changes
Oct 30, 2025
![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)
test/integration_regressions_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:
test/integration_regressions_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: jorhett
Repo: gruntwork-io/terragrunt PR: 1234
File: errors/multierror.go:35-39
Timestamp: 2025-09-10T04:41:35.652Z
Learning: In Terragrunt's MultiError.Error() method, checking for the exact string "exit status 2" and returning it directly is not a brittle hack but a semantic fix. Terraform's --detailed-exitcode flag uses exit code 2 to mean "plan succeeded with changes" (not an error), so when multiple modules return this status, it should not be wrapped in "Hit multiple errors:" formatting as that misrepresents successful operations as errors.
📚 Learning: 2025-11-03T04:40:00.989Z
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:00.989Z
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:
test/integration_regressions_test.go
🧬 Code graph analysis (1)
test/integration_regressions_test.go (2)
test/helpers/package.go (3)
CleanupTerraformFolder(879-886)CopyEnvironment(89-102)RunTerragruntCommandWithOutput(1004-1008)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). (5)
- GitHub Check: license_check / License Check
- GitHub Check: lint / lint
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
Comment on lines
+312
to
+316
| // Set TG_PROVIDER_CACHE=1 and use --queue-exclude-external as in the repro steps | ||
| stdout, stderr, err := helpers.RunTerragruntCommandWithOutput( | ||
| t, | ||
| "terragrunt run --all --queue-exclude-external plan --non-interactive --working-dir "+rootPath, | ||
| ) |
There was a problem hiding this comment.
Set TG_PROVIDER_CACHE so this regression test actually covers the reported scenario.
The comment says we mirror the repro flags, but the code never sets TG_PROVIDER_CACHE=1. Without that env var, this test won’t catch regressions that only surface when the provider cache is enabled—the exact case called out in issue #4983. Please set the env var before invoking Terragrunt so the test exercises the intended code path.
func TestRunAllWithGenerateAndExpose_WithProviderCacheAndExcludeExternal(t *testing.T) {
t.Parallel()
+ t.Setenv("TG_PROVIDER_CACHE", "1")
+
testFixture := "fixtures/regressions/parsing-run-all-with-generate"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/integration_regressions_test.go around lines 312 to 316, the test
invokes Terragrunt with the repro flags but never sets TG_PROVIDER_CACHE=1 so it
won't exercise the provider-cache code path; set TG_PROVIDER_CACHE=1 before
running the command (either by passing an env map into
helpers.RunTerragruntCommandWithOutput if supported, or by calling
os.Setenv("TG_PROVIDER_CACHE","1") and deferring restore to the previous value)
so the test runs with the provider cache enabled while keeping the existing
command args unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Before this fix:
After this fix:
Fixes #4983.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Bug Fixes
Tests