feat: Adding reading support in find
#5002
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR extends the find command to report files read by components during parsing. It introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Find CLI
participant Cmd as Find Command
participant Disc as Discovery
participant Parser as Config Parser
CLI->>Cmd: Run with Reading flag
Cmd->>Cmd: Create DiscoveryCommandOptions<br/>(QueueConstructAs, Reading)
Cmd->>Disc: NewForCommand(opts)
alt Reading flag set
Disc->>Disc: WithReadFiles()
Disc->>Disc: WithRequiresParse()
end
alt QueueConstructAs provided
Disc->>Disc: Parse shell command<br/>via shellwords
Disc->>Disc: WithRequiresParse()
end
Disc->>Parser: Parse configs with ReadFiles
Parser->>Parser: trackFileRead(FilesRead, path)<br/>deduplicate
Parser-->>Disc: Return components<br/>+ FilesRead list
Disc-->>Cmd: Discovered components
Cmd->>Cmd: Convert to FoundComponent<br/>populate Reading field
Cmd-->>CLI: JSON output with<br/>Reading: [files...]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes span multiple layers with interconnected logic (discovery wiring, deduplication), introduce new public API fields and methods, and refactor shellwords parsing across commands. However, most individual changes follow consistent patterns (flag/field additions, method chaining), and the behavioral changes are localized to specific areas without affecting core signatures. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
chore/updating-language-in-discovery-commands
branch
from
e32fdc6 to
b84846b
Compare
yhakbar
force-pushed
the
feat/adding-reading-support-in-find
branch
from
5160bea to
6d81158
Compare
yhakbar
force-pushed
the
feat/adding-reading-support-in-find
branch
from
cd62648 to
954ebe4
Compare
yhakbar
force-pushed
the
feat/adding-reading-support-in-find
branch
from
954ebe4 to
e866c94
Compare
…o parsing when users request tracking of read files
yhakbar
force-pushed
the
feat/adding-reading-support-in-find
branch
from
68d4167 to
0d016f1
Compare
![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
🧹 Nitpick comments (4)
cli/commands/find/cli.go (1)
84-90: Auto-switch to JSON when --reading is set (prevents confusing no-op in text mode).Make --reading behave like --json by forcing FormatJSON in Before. This avoids a silent no-op when format=text.
func NewCommand(l log.Logger, opts *options.TerragruntOptions) *cli.Command { @@ if cmdOpts.JSON { cmdOpts.Format = FormatJSON } + // Reading output is only surfaced in JSON; align behavior with --json alias. + if cmdOpts.Reading { + cmdOpts.Format = FormatJSON + } + if cmdOpts.DAG { cmdOpts.Mode = ModeDAG }Also applies to: 121-134
cli/commands/find/find_test.go (1)
410-478: Solid coverage of reading JSON output.Test exercises both read_terragrunt_config and read_tfvars_file and validates relative paths. Consider adding a quick negative case asserting that reading is omitted when the flag is false.
Also applies to: 501-502
config/config_helpers.go (1)
724-725: Dedup helper is good; add small robustness and keep paths canonical.
- Add filepath.Clean inside trackFileRead so all call sites get normalized even if a future one forgets to clean/abs-ify first.
- Optional: if FilesRead grows large, consider a membership set to avoid O(n) Contains checks, but current approach is fine for small lists.
func trackFileRead(filesRead *[]string, path string) { if filesRead == nil { return } + // Normalize to improve dedup reliability. + path = filepath.Clean(path) + if slices.Contains(*filesRead, path) { return } *filesRead = append(*filesRead, path) }Also applies to: 943-944, 1141-1143, 1190-1192, 1306-1318
cli/commands/find/find.go (1)
169-179: Avoid empty strings on path conversion errors; normalize slashes for stable JSON across OS.Preallocating and indexing leaves zero-values when Rel fails. Build slices with append and ToSlash for portability.
- if opts.Reading && len(c.Reading) > 0 { - foundComponent.Reading = make([]string, len(c.Reading)) - - for i, reading := range c.Reading { - relReadingPath, err := filepath.Rel(opts.WorkingDir, reading) - if err != nil { - errs = append(errs, errors.New(err)) - } - - foundComponent.Reading[i] = relReadingPath - } - } + if opts.Reading && len(c.Reading) > 0 { + var relReads []string + for _, reading := range c.Reading { + relReadingPath, err := filepath.Rel(opts.WorkingDir, reading) + if err != nil { + errs = append(errs, errors.New(err)) + continue + } + relReads = append(relReads, filepath.ToSlash(relReadingPath)) + } + if len(relReads) > 0 { + foundComponent.Reading = relReads + } + } @@ - if opts.Dependencies && len(c.Dependencies()) > 0 { - foundComponent.Dependencies = make([]string, len(c.Dependencies())) - - for i, dep := range c.Dependencies() { - relDepPath, err := filepath.Rel(opts.WorkingDir, dep.Path) - if err != nil { - errs = append(errs, errors.New(err)) - continue - } - - foundComponent.Dependencies[i] = relDepPath - } - } + if opts.Dependencies && len(c.Dependencies()) > 0 { + var deps []string + for _, dep := range c.Dependencies() { + relDepPath, err := filepath.Rel(opts.WorkingDir, dep.Path) + if err != nil { + errs = append(errs, errors.New(err)) + continue + } + deps = append(deps, filepath.ToSlash(relDepPath)) + } + if len(deps) > 0 { + foundComponent.Dependencies = deps + } + }Also applies to: 182-195
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cli/commands/find/cli.go(2 hunks)cli/commands/find/find.go(3 hunks)cli/commands/find/find_test.go(3 hunks)cli/commands/find/options.go(2 hunks)cli/commands/list/list.go(1 hunks)config/config_helpers.go(5 hunks)internal/discovery/constructor.go(3 hunks)internal/discovery/discovery.go(4 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_helpers.gointernal/discovery/constructor.gocli/commands/list/list.gocli/commands/find/options.gocli/commands/find/find_test.gocli/commands/find/find.gocli/commands/find/cli.gointernal/discovery/discovery.go
🧬 Code graph analysis (6)
internal/discovery/constructor.go (3)
cli/commands/find/cli.go (1)
Reading(30-30)internal/discovery/discovery.go (1)
Parse(423-522)internal/component/component.go (1)
DiscoveryContext(81-84)
cli/commands/list/list.go (3)
cli/commands/find/cli.go (2)
Dependencies(26-26)External(27-27)config/dependency.go (1)
Dependencies(47-47)internal/experiment/experiment.go (1)
Experiments(46-46)
cli/commands/find/options.go (1)
cli/commands/find/cli.go (5)
Dependencies(26-26)Exclude(28-28)Include(29-29)External(27-27)Reading(30-30)
cli/commands/find/find_test.go (2)
cli/commands/find/find.go (1)
FoundComponents(113-113)cli/commands/find/cli.go (1)
Reading(30-30)
cli/commands/find/find.go (3)
cli/commands/find/cli.go (2)
Dependencies(26-26)Reading(30-30)config/dependency.go (1)
Dependencies(47-47)cli/commands/find/options.go (1)
ModeDAG(25-25)
cli/commands/find/cli.go (2)
cli/flags/flag.go (1)
NewFlag(28-41)internal/cli/bool_flag.go (1)
BoolFlag(13-49)
⏰ 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). (4)
- GitHub Check: license_check / License Check
- GitHub Check: lint / lint
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (7)
cli/commands/find/options.go (1)
62-64: LGTM: option surfaces the flag cleanly.No issues. Ensure CLI enforces/auto-switches JSON when set (see cli.go suggestion).
cli/commands/list/list.go (1)
27-34: LGTM: delegates queue-construction parsing to discovery constructor.Clean pass-through; matches shared behavior with find.
internal/discovery/discovery.go (5)
110-111: LGTM! Well-defined configuration fields.The new fields are clearly documented and serve distinct purposes for configuring discovery behavior (config filenames, dependency depth, concurrency, and read tracking).
Also applies to: 116-117, 119-120, 146-147
208-236: LGTM! Consistent precondition pattern.The modifications ensure
WithRequiresParse()is called before enabling features that depend on parsing. This is a good defensive pattern that prevents configuration errors.
238-252: LGTM! Clean builder methods.Both
WithReadFiles()andWithRequiresParse()follow the established builder pattern and correctly set their respective flags. The precondition call toWithRequiresParse()ensures parsing is enabled when file reading is requested.
282-286: LGTM! Simple configuration method.The
WithConfigFilenames()method correctly sets the config filenames and follows the builder pattern.
515-519: The design properly filtersReadingoutput at the CLI layer.Verification confirms that the CLI layer correctly filters the
Readingfield based on the--readingflag (line 169 incli/commands/find/find.go). WhileReadingis populated unconditionally when parsing occurs, parsing itself only happens when required by other operations (--discover-dependencies,--parse-exclude,--parse-include, or--reading). Additionally, theReadingfield is JSON-marshaled withomitempty, so empty slices don't appear in output.The current design—populate all fields during parsing and filter on display—is sound and avoids unnecessary complexity threading the
readFilesflag through internal parsing logic.
| if opts.Reading { | ||
| d = d.WithReadFiles() | ||
| } | ||
|
|
||
| if opts.QueueConstructAs != "" { | ||
| d = d.WithParseExclude() | ||
| d = d.WithDiscoverDependencies() | ||
|
|
||
| parser := shellwords.NewParser() | ||
|
|
||
| args, err := parser.Parse(opts.QueueConstructAs) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| cmd := args[0] | ||
| if len(args) > 1 { | ||
| args = args[1:] | ||
| } else { | ||
| args = nil | ||
| } | ||
|
|
||
| d = d.WithDiscoveryContext(&component.DiscoveryContext{ | ||
| Cmd: cmd, | ||
| Args: args, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Potential panic: shellwords.Parse may return zero args for whitespace-only input. Guard len(args).
Accessing args[0] without checking length can panic. Return a clear error instead.
import (
"github.com/gruntwork-io/terragrunt/internal/component"
+ "github.com/gruntwork-io/terragrunt/internal/errors"
"github.com/gruntwork-io/terragrunt/internal/experiment"
"github.com/gruntwork-io/terragrunt/internal/filter"
"github.com/mattn/go-shellwords"
)
@@
parser := shellwords.NewParser()
args, err := parser.Parse(opts.QueueConstructAs)
if err != nil {
return nil, err
}
- cmd := args[0]
+ if len(args) == 0 {
+ return nil, errors.New("queue-construct-as parsed to zero tokens; provide a command")
+ }
+ cmd := args[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if opts.Reading { | |
| d = d.WithReadFiles() | |
| } | |
| if opts.QueueConstructAs != "" { | |
| d = d.WithParseExclude() | |
| d = d.WithDiscoverDependencies() | |
| parser := shellwords.NewParser() | |
| args, err := parser.Parse(opts.QueueConstructAs) | |
| if err != nil { | |
| return nil, err | |
| } | |
| cmd := args[0] | |
| if len(args) > 1 { | |
| args = args[1:] | |
| } else { | |
| args = nil | |
| } | |
| d = d.WithDiscoveryContext(&component.DiscoveryContext{ | |
| Cmd: cmd, | |
| Args: args, | |
| }) | |
| } | |
| if opts.Reading { | |
| d = d.WithReadFiles() | |
| } | |
| if opts.QueueConstructAs != "" { | |
| d = d.WithParseExclude() | |
| d = d.WithDiscoverDependencies() | |
| parser := shellwords.NewParser() | |
| args, err := parser.Parse(opts.QueueConstructAs) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if len(args) == 0 { | |
| return nil, errors.New("queue-construct-as parsed to zero tokens; provide a command") | |
| } | |
| cmd := args[0] | |
| if len(args) > 1 { | |
| args = args[1:] | |
| } else { | |
| args = nil | |
| } | |
| d = d.WithDiscoveryContext(&component.DiscoveryContext{ | |
| Cmd: cmd, | |
| Args: args, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In internal/discovery/constructor.go around lines 51 to 77, the code assumes
parser.Parse(opts.QueueConstructAs) returns at least one element and directly
accesses args[0], which can panic for whitespace-only input; change the flow to
check the returned args length after parsing and if len(args) == 0 return a
clear error (e.g., "invalid QueueConstructAs: no command provided") instead of
dereferencing args[0], otherwise proceed to set cmd=args[0] and slice args as
before and attach the DiscoveryContext.
Description
Adds support for
--readingflag tofindto show users the content that's being read by Terragrunt during HCL parsing.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Chores