fix: better errors reporting form autoincludes - #5985
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 typed errors for malformed autoinclude dependencies and stack-dependency expansion, enforces strict Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Parser as AutoInclude Parser
participant FS as Filesystem
participant Resolver as AutoInclude Resolver
participant Discovery as Dependency Expander
participant Generator as Autoinclude Generator
CLI->>Parser: Parse stack file (include merging)
Parser->>FS: Read included file bytes
FS-->>Parser: Return bytes
Parser-->>Resolver: Return AutoIncludeHCL + SourceBytes
Resolver->>Resolver: Validate dependency blocks (labels, config_path)
alt malformed blocks
Resolver-->>CLI: Return MalformedDependencyError(s)
else valid
Resolver->>Discovery: Expand dependency config_path -> unit paths (os.Stat gate)
alt expansion error
Discovery-->>CLI: Return StackDependencyExpansionError (wraps inner)
else success
Discovery-->>Generator: Provide unit paths
Generator->>FS: Write autoinclude file using SourceBytes for slicing
Generator-->>CLI: Success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…-io/terragrunt into 5980-auto-include-parse-errors
denis256
marked this pull request as ready for review
April 28, 2026 20:44
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/hclparse/autoinclude.go (1)
245-247: Consider returning discovered paths alongside errors.The current implementation returns
nilwhen any dependency block is malformed. For better error recovery, consider returning successfully extracted paths along with the accumulated errors, allowing callers to decide how to handle partial results.However, the all-or-nothing approach is also valid if downstream DAG processing requires complete dependency information to function correctly.
♻️ Optional: Return partial results with errors
- if len(errs) > 0 { - return nil, errors.Join(errs...) + if len(errs) > 0 { + return paths, errors.Join(errs...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/hclparse/autoinclude.go` around lines 245 - 247, The current block returns nil on any accumulated errors (len(errs) > 0); change it to return the successfully discovered paths alongside the joined error so callers can handle partial results. Specifically, when len(errs) > 0 return the variable holding the collected paths (e.g. paths or discoveredPaths) and errors.Join(errs...) instead of nil, and ensure the function signature and callers (the functions that call this autoinclude routine) accept and handle a non-nil slice plus error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/hclparse/autoinclude.go`:
- Around line 245-247: The current block returns nil on any accumulated errors
(len(errs) > 0); change it to return the successfully discovered paths alongside
the joined error so callers can handle partial results. Specifically, when
len(errs) > 0 return the variable holding the collected paths (e.g. paths or
discoveredPaths) and errors.Join(errs...) instead of nil, and ensure the
function signature and callers (the functions that call this autoinclude
routine) accept and handle a non-nil slice plus error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 716f072c-656f-42c7-852d-6f4dd89f9e17
📒 Files selected for processing (6)
internal/hclparse/autoinclude.gointernal/hclparse/errors.gointernal/hclparse/generate.gointernal/hclparse/parse.gointernal/hclparse/stack.gotest/integration_stack_dependencies_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/hclparse/errors.go
- internal/hclparse/generate.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/hclparse/stack.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/hclparse/autoinclude_test.go (1)
421-438: Consider consolidating the error type check.The test writes both a
.jsonsuffixed file and a regular file but only reads from the regular path. The assertion at line 437 usingerrors.Astwice with||is correct but slightly verbose.♻️ Minor simplification
- assert.True(t, errors.As(err, &fpe) || errors.As(err, &ube), "expected FileParseError or UnexpectedBodyTypeError, got %T: %v", err, err) + isExpectedErr := errors.As(err, &fpe) || errors.As(err, &ube) + assert.True(t, isExpectedErr, "expected FileParseError or UnexpectedBodyTypeError, got %T: %v", err, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/hclparse/autoinclude_test.go` around lines 421 - 438, In TestAutoIncludeDependencyPaths_UnexpectedBodyTypeOnJSON, replace the two separate errors.As checks with a single consolidated check that unwraps the error chain and type-switches to detect either hclparse.FileParseError or hclparse.UnexpectedBodyTypeError; locate the assertion using errors.As(err, &fpe) || errors.As(err, &ube) and replace it with a loop that walks err via errors.Unwrap (or errors.As into an interface and type-switches) and fails the test unless a FileParseError or UnexpectedBodyTypeError is found, keeping references to the test function name and the err variable for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/hclparse/stack_test.go`:
- Line 406: Update the misleading comment in internal/hclparse/stack_test.go
near the test that asserts symlink resolution: change the line that currently
reads “not the symlink target” to clearly state that the path should resolve to
the real directory (the symlink target) and reject the symlink path (e.g., “Path
should resolve to the real directory (the symlink target), not the symlink
path.”) so it matches the assertions in the test surrounding the symlink
handling code in this test function.
---
Nitpick comments:
In `@internal/hclparse/autoinclude_test.go`:
- Around line 421-438: In
TestAutoIncludeDependencyPaths_UnexpectedBodyTypeOnJSON, replace the two
separate errors.As checks with a single consolidated check that unwraps the
error chain and type-switches to detect either hclparse.FileParseError or
hclparse.UnexpectedBodyTypeError; locate the assertion using errors.As(err,
&fpe) || errors.As(err, &ube) and replace it with a loop that walks err via
errors.Unwrap (or errors.As into an interface and type-switches) and fails the
test unless a FileParseError or UnexpectedBodyTypeError is found, keeping
references to the test function name and the err variable for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15cfec02-6eb4-4e29-af74-a89b234ee936
📒 Files selected for processing (6)
internal/discovery/helpers.gointernal/hclparse/autoinclude_test.gointernal/hclparse/parse.gointernal/hclparse/stack_test.gopkg/config/stack.gotest/integration_stack_dependencies_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/hclparse/parse.go
yhakbar
reviewed
May 1, 2026
| filePath := filepath.Join(tmpDir, "another-name.hcl") | ||
| require.NoError(t, os.WriteFile(filePath, []byte(`# regular file, not a directory`), 0644)) | ||
|
|
||
| result, err := hclparse.ParseStackFileFromPath(vfs.NewOSFS(), filePath) |
Collaborator
There was a problem hiding this comment.
NIT: can't we use an in-memory fs here?
yhakbar
reviewed
May 1, 2026
| assert.Equal(t, "app", result.Units[0].Name) | ||
| } | ||
|
|
||
| // Uses OSFS because MemMapFS does not faithfully reproduce os.Symlink semantics that util.ResolvePath relies on. |
Collaborator
There was a problem hiding this comment.
This is interesting. Is this a bug in our vfs implementation?
yhakbar
approved these changes
May 1, 2026
3 tasks
Rahul-Kumar-prog
pushed a commit
to Rahul-Kumar-prog/terragrunt
that referenced
this pull request
May 10, 2026
* bug: HCL parsing errors handling * bug: handling of auto include parsing errors * chore: auto include fixes * chore: simplified parsing errors * chore: stack parsing fixes * chore: internal tests * chore: statck parsing fixes * chore: stack dependencies autoinclude * chore: internal test fix * chore: internal test fix * chore: improved discovery errors * chore: fuzz test * chore: fuzzy tests improvements * chore: auto include fix * chore: stacks simplification * chore: simplified auto include errors * chore: PR comments * chore: failing tests fixes * chore: discovery helpers * chore: symlinks handling * chore: parser cleanup
8 tasks
8 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
dependencyblocks with zero or 2+ labelsFixes #5980.
RFC: #5663
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Bug Fixes
Tests