chore: Refactor discovery for nuanced parse handling #5477
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces legacy monolithic discovery with a new multi‑phase pipeline (Filesystem, Worktree, Parse, Graph, Relationship, Final); adds classifier/matcher, candidacy analysis, phases, helpers, errors, and many tests; removes old Dependency/Dependent/Relationship/Worktree discovery implementations and queue-based exclude logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Discovery
participant Filesystem as FilesystemPhase
participant Worktree as WorktreePhase
participant Parse as ParsePhase
participant Graph as GraphPhase
participant Relationship as RelationshipPhase
Client->>Discovery: Discover(ctx, logger, opts)
Discovery->>Filesystem: Run(PhaseInput)
Filesystem-->>Discovery: Discovered/Candidates/Errors
alt Worktrees configured
Discovery->>Worktree: Run(PhaseInput)
Worktree-->>Discovery: Discovered/Candidates/Errors
end
Discovery->>Parse: Run(PhaseInput)
Parse-->>Discovery: Discovered/Candidates/Errors
Discovery->>Graph: Run(PhaseInput)
Graph-->>Discovery: Discovered/Candidates/Errors
Discovery->>Relationship: Run(PhaseInput)
Relationship-->>Discovery: Discovered/Candidates/Errors
Discovery-->>Client: Aggregated components, error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@internal/discovery/discovery_integration_test.go`:
- Around line 109-132: The test TestDiscovery_StackHiddenAllowed claims to
verify behavior with NoHidden but never enables it; update the discovery builder
to call WithNoHidden() on the discovery.New(tmpDir) chain (i.e., replace or
augment discovery.New(tmpDir).WithDiscoveryContext(...) with
discovery.New(tmpDir).WithNoHidden().WithDiscoveryContext(...)) so the
Discover(ctx, l, opts) call runs with NoHidden enabled and the assertion that
components.Filter(component.UnitKind).Paths() contains stackHiddenDir is
meaningful.
In `@internal/discovery/discovery_test.go`:
- Around line 205-209: The test currently ignores the error return from
classifier.Classify(c, ctx); capture the third return value (err) when calling
classifier.Classify in the test (alongside status and reason) and assert that
err is nil (e.g., use require.NoError(t, err) or assert.NoError(t, err)) before
checking status and reason to ensure the call succeeded; the call site to update
is the classifier.Classify(c, ctx) invocation that uses
filter.ClassificationContext{}.
In `@internal/discovery/doc.go`:
- Around line 66-74: Replace the example's call to discovery.New(workingDir)
with the package-recommended constructor NewDiscovery(workingDir); update the
example chain to start with NewDiscovery(...) so it benefits from the smarter
initialization (CPU-aware worker count and discoveryContext) used by
NewForDiscoveryCommand and NewForHCLCommand while keeping the same fluent calls
(.WithFilters, .WithDiscoveryContext, .WithRelationships,
.WithMaxDependencyDepth) and subsequent Discover invocation.
In `@internal/discovery/helpers.go`:
- Around line 70-75: The current check uses strings.HasPrefix(relPath, "..")
which false-positives paths like "..foo"; change the test to only treat true
parent escapes by checking relPath == ".." or starts with ".." +
filepath.Separator (e.g. relPath == ".." || strings.HasPrefix(relPath,
".."+string(os.PathSeparator))). Update the return expression that currently
uses relPath to use this precise check (referencing the relPath variable and
filepath package) so only actual parent-directory escapes are considered
external.
- Around line 132-175: mergeResults depends on data channels
(PhaseOutput.Discovered, .Candidates, .Errors) being closed before the Done
channel to avoid fragile ordering from LIFO defers; update phase goroutines (the
implementations that write to these channels) to explicitly close Discovered,
Candidates and Errors before signaling Done (i.e., close data channels first,
then close/send on Done) rather than relying on defer ordering, or add
documentation in those phase functions describing the required close sequence so
mergeResults can safely range over the channels after Done.
In `@internal/discovery/phase_filesystem.go`:
- Around line 176-189: The hidden-directory check in
FilesystemPhase.skipDirIfIgnorable currently skips any directory whose base
begins with "." (via discovery.noHidden), which unintentionally excludes
".terragrunt-stack"; update the method to exempt ".terragrunt-stack" from being
skipped by adding a special-case in skipDirIfIgnorable (method) so that when
discovery.noHidden is true it returns nil instead of filepath.SkipDir for base
== ".terragrunt-stack" (i.e., allow ".terragrunt-stack" to be discovered while
still skipping other dot-prefixed dirs).
In `@internal/discovery/phase_graph.go`:
- Around line 604-611: The code in phase_graph.go uses discovery.configFilenames
directly when checking and creating candidates; add the same defensive fallback
as in phase_filesystem.go by defaulting an empty discovery.configFilenames to
DefaultConfigFilenames before using it (i.e., ensure a local variable or
overwrite discovery.configFilenames with DefaultConfigFilenames if len==0), then
use that variable in the slices.Contains check and when calling
createComponentFromPath(discovery.configFilenames, discovery.discoveryContext)
so the phase is safe if configFilenames is not set.
- Around line 764-804: The nested g.Go call inside the goroutine that ranges
over discoveredDependents violates errgroup rules; remove the inner g.Go and
invoke p.discoverDependentsUpstream inline instead, keeping creation of
freshVisitedDirs and the filepath.Dir(dependent.Path()) arguments the same; if
discoverDependentsUpstream returns an error, lock errMu, append to errs, unlock,
and continue (return nil only for the outer goroutine), and ensure no new g.Go
calls are made from within running goroutines (all g.Go calls must happen before
g.Wait()).
In `@internal/discovery/phase_worktree.go`:
- Around line 424-442: The goroutines in the shaGroup are writing to the shared
outer variable err (and implicitly to fromSHA/toSHA) causing a data race; fix by
using per-goroutine local result and error variables (e.g. fsha, ferr and tsha,
terr) inside each shaGroup.Go closure, return the local error from the closure,
and after shaGroup.Wait() copy the local fsha/tsha into the outer fromSHA/toSHA
and handle/return the error if any; reference GenerateDirSHA256 and shaGroup.Go
closures to find the affected code.
- Around line 162-168: The current handling after discoveryGroup.Wait() drops
errors when the errors channel is full; change it to a context-aware blocking
send so errors aren't lost: replace the non-blocking select (select { case
errors <- err: default: }) with a blocking send that waits for either the send
to succeed or context cancellation (select { case errors <- err: case
<-ctx.Done(): }) using the existing ctx passed into this scope (or add one if
missing); alternatively, if you prefer buffering, increase the capacity where
the errors channel is created to accommodate expected concurrent errors
(reference discoveryGroup.Wait(), the errors channel variable, and ctx).
In `@internal/filter/classifier.go`:
- Around line 216-222: The conditional inside the parse-required check is dead
code because both branches return the same values; in the method using
c.parseExprs and ctx.ParseDataAvailable, remove the redundant inner if (the
matchesFilesystem || matchesGit check) and replace it with a single return
StatusCandidate, CandidacyReasonRequiresParse, -1 so that the code simply checks
if len(c.parseExprs) > 0 && !ctx.ParseDataAvailable and then returns the
candidacy result without the pointless branch (update the block containing
c.parseExprs, ctx.ParseDataAvailable, matchesFilesystem, matchesGit
accordingly).
In `@internal/filter/matcher.go`:
- Around line 19-41: MatchComponent currently treats PrefixExpression with
operator "!" as a no-op and evaluates InfixExpression with "|" like an AND;
update it so PrefixExpression with Operator "!" returns the inverted result of
MatchComponent(c, node.Right) (i.e., call MatchComponent and return !match along
with any error), and change InfixExpression with Operator "|" to implement
short-circuit OR: evaluate left with MatchComponent(c, node.Left) and if true
return true, otherwise evaluate and return MatchComponent(c, node.Right); keep
error propagation from the recursive calls (e.g., in MatchComponent,
PrefixExpression, InfixExpression).
In `@test/integration_debug_test.go`:
- Around line 300-305: Replace the unconditional t.Skip in
test/integration_debug_test.go with a documented quarantine: either re-enable
the test, or gate its skip behind an explicit opt-out (e.g., check an env var
like SKIP_INTEGRATION_DEBUG) and call t.Skipf only when the opt-out is set,
and/or add a TODO comment referencing a tracked issue and timeline (include the
issue number like `#5469`) so the test remains discoverable and the regression gap
is tracked; ensure you update the test’s t.Skip/t.Skipf call and add the issue
reference and expected timeline in the test file.
🧹 Nitpick comments (7)
internal/filter/classifier.go (2)
251-257: Silently ignoring errors fromMatchComponent.Errors from
MatchComponentare discarded throughout the matching helpers (matchesAnyNegated,matchesAnyPositive,matchesFilesystemExpression,matchesGraphExpressionTarget). While this may be intentional to avoid blocking classification on transient errors, consider at minimum debug-level logging to aid troubleshooting.♻️ Optional: Log matching errors at debug level
func (c *Classifier) matchesAnyNegated(comp component.Component) bool { return slices.ContainsFunc(c.negatedExprs, func(expr Expression) bool { - match, _ := MatchComponent(comp, expr) + match, err := MatchComponent(comp, expr) + if err != nil { + c.logger.Debugf("error matching component %s against negated expression: %v", comp.Path(), err) + } return match }) }
117-135:Analyzealways returnsnilerror.The method signature indicates it can return an error, but it never does. This could be intentional for future extensibility, but currently it's misleading for callers.
Consider either:
- Changing the signature to not return an error if none is expected
- Adding a comment explaining this is for future extensibility
internal/discovery/phase_worktree_test.go (1)
534-547: Potential out-of-bounds access ingetRelativePathhelper.The helper function assumes
c.Path()always starts withdc.WorkingDir. If this assumption is violated (e.g., path doesn't have the working directory prefix), the slice operation at line 541 could produce unexpected results or panic iflen(dc.WorkingDir) > len(c.Path()).Consider using
filepath.Relorstrings.TrimPrefixfor safer path handling:🔧 Safer implementation using strings.TrimPrefix
func getRelativePath(c component.Component) string { dc := c.DiscoveryContext() if dc == nil { return c.Path() } - rel := c.Path()[len(dc.WorkingDir):] + rel := strings.TrimPrefix(c.Path(), dc.WorkingDir) if rel == "" { return "/" } return filepath.Clean(rel) }internal/discovery/phase_worktree.go (3)
292-298: Redundant error check appears unreachable.If
g.Wait()returns an error (line 292), we return early. If it returns nil, all goroutines succeeded (returned nil), meaning no errors were appended toerrs. The check at lines 296-298 appears to be dead code.Consider removing the redundant check or, if intentional (e.g., for defensive coding), add a comment explaining when this path could be taken.
396-402: Same redundant error check pattern as indiscoverChangesInWorktreeStacks.The check at lines 400-402 follows the same pattern - it's only reachable when
discoveryGroup.Wait()returns nil, which means no goroutine returned an error anderrsshould be empty.
461-489: Consider using a map for O(n) matching instead of O(n×m) nested loops.The current implementation has quadratic complexity. For large worktrees with many components, building a map of suffixes could improve performance.
♻️ Proposed optimization
func MatchComponentPairs( fromComponents component.Components, toComponents component.Components, ) []ComponentPair { componentPairs := make([]ComponentPair, 0, max(len(fromComponents), len(toComponents))) + // Build index of to-components by their relative path suffix + toIndex := make(map[string]component.Component, len(toComponents)) + for _, toComponent := range toComponents { + suffix := filepath.Clean(strings.TrimPrefix( + toComponent.Path(), + toComponent.DiscoveryContext().WorkingDir, + )) + toIndex[suffix] = toComponent + } + for _, fromComponent := range fromComponents { fromComponentSuffix := strings.TrimPrefix( fromComponent.Path(), fromComponent.DiscoveryContext().WorkingDir, ) + cleanedSuffix := filepath.Clean(fromComponentSuffix) - for _, toComponent := range toComponents { - toComponentSuffix := strings.TrimPrefix( - toComponent.Path(), - toComponent.DiscoveryContext().WorkingDir, - ) - - if filepath.Clean(fromComponentSuffix) == filepath.Clean(toComponentSuffix) { - componentPairs = append(componentPairs, ComponentPair{ - FromComponent: fromComponent, - ToComponent: toComponent, - }) - } + if toComponent, ok := toIndex[cleanedSuffix]; ok { + componentPairs = append(componentPairs, ComponentPair{ + FromComponent: fromComponent, + ToComponent: toComponent, + }) } } return componentPairs }internal/discovery/discovery.go (1)
552-573: Consider logging removed components during cycle removal.The cycle removal silently removes components. Adding debug logging would help users understand which components were removed to break cycles.
📝 Suggested enhancement
Since this function doesn't have access to a logger, consider returning the removed paths for logging by the caller, or accept a logger parameter:
-func removeCycles(components component.Components) component.Components { +func removeCycles(l log.Logger, components component.Components) component.Components { var ( c component.Component err error ) for range maxCycleRemovalAttempts { c, err = components.CycleCheck() if err == nil { break } if c == nil { break } + l.Debugf("Removing component %s to break dependency cycle", c.Path()) components = components.RemoveByPath(c.Path()) } return components }
| // TestDiscovery_StackHiddenAllowed tests that .terragrunt-stack directories are discovered even with NoHidden. | ||
| func TestDiscovery_StackHiddenAllowed(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tmpDir := helpers.TmpDirWOSymlinks(t) | ||
| stackHiddenDir := filepath.Join(tmpDir, ".terragrunt-stack", "u") | ||
| require.NoError(t, os.MkdirAll(stackHiddenDir, 0755)) | ||
| require.NoError(t, os.WriteFile(filepath.Join(stackHiddenDir, "terragrunt.hcl"), []byte(""), 0644)) | ||
|
|
||
| l := logger.CreateLogger() | ||
| opts := &options.TerragruntOptions{ | ||
| WorkingDir: tmpDir, | ||
| } | ||
|
|
||
| ctx := t.Context() | ||
|
|
||
| // Even with NoHidden, .terragrunt-stack contents should be discovered | ||
| d := discovery.New(tmpDir). | ||
| WithDiscoveryContext(&component.DiscoveryContext{WorkingDir: tmpDir}) | ||
|
|
||
| components, err := d.Discover(ctx, l, opts) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, components.Filter(component.UnitKind).Paths(), stackHiddenDir) | ||
| } |
There was a problem hiding this comment.
Test doesn’t enable NoHidden despite the stated intent.
This test never calls WithNoHidden(), so it only validates the default behavior.
🛠️ Suggested fix
- d := discovery.New(tmpDir).
- WithDiscoveryContext(&component.DiscoveryContext{WorkingDir: tmpDir})
+ d := discovery.New(tmpDir).
+ WithDiscoveryContext(&component.DiscoveryContext{WorkingDir: tmpDir}).
+ WithNoHidden()📝 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.
| // TestDiscovery_StackHiddenAllowed tests that .terragrunt-stack directories are discovered even with NoHidden. | |
| func TestDiscovery_StackHiddenAllowed(t *testing.T) { | |
| t.Parallel() | |
| tmpDir := helpers.TmpDirWOSymlinks(t) | |
| stackHiddenDir := filepath.Join(tmpDir, ".terragrunt-stack", "u") | |
| require.NoError(t, os.MkdirAll(stackHiddenDir, 0755)) | |
| require.NoError(t, os.WriteFile(filepath.Join(stackHiddenDir, "terragrunt.hcl"), []byte(""), 0644)) | |
| l := logger.CreateLogger() | |
| opts := &options.TerragruntOptions{ | |
| WorkingDir: tmpDir, | |
| } | |
| ctx := t.Context() | |
| // Even with NoHidden, .terragrunt-stack contents should be discovered | |
| d := discovery.New(tmpDir). | |
| WithDiscoveryContext(&component.DiscoveryContext{WorkingDir: tmpDir}) | |
| components, err := d.Discover(ctx, l, opts) | |
| require.NoError(t, err) | |
| assert.Contains(t, components.Filter(component.UnitKind).Paths(), stackHiddenDir) | |
| } | |
| // TestDiscovery_StackHiddenAllowed tests that .terragrunt-stack directories are discovered even with NoHidden. | |
| func TestDiscovery_StackHiddenAllowed(t *testing.T) { | |
| t.Parallel() | |
| tmpDir := helpers.TmpDirWOSymlinks(t) | |
| stackHiddenDir := filepath.Join(tmpDir, ".terragrunt-stack", "u") | |
| require.NoError(t, os.MkdirAll(stackHiddenDir, 0755)) | |
| require.NoError(t, os.WriteFile(filepath.Join(stackHiddenDir, "terragrunt.hcl"), []byte(""), 0644)) | |
| l := logger.CreateLogger() | |
| opts := &options.TerragruntOptions{ | |
| WorkingDir: tmpDir, | |
| } | |
| ctx := t.Context() | |
| // Even with NoHidden, .terragrunt-stack contents should be discovered | |
| d := discovery.New(tmpDir). | |
| WithDiscoveryContext(&component.DiscoveryContext{WorkingDir: tmpDir}). | |
| WithNoHidden() | |
| components, err := d.Discover(ctx, l, opts) | |
| require.NoError(t, err) | |
| assert.Contains(t, components.Filter(component.UnitKind).Paths(), stackHiddenDir) | |
| } |
🤖 Prompt for AI Agents
In `@internal/discovery/discovery_integration_test.go` around lines 109 - 132, The
test TestDiscovery_StackHiddenAllowed claims to verify behavior with NoHidden
but never enables it; update the discovery builder to call WithNoHidden() on the
discovery.New(tmpDir) chain (i.e., replace or augment
discovery.New(tmpDir).WithDiscoveryContext(...) with
discovery.New(tmpDir).WithNoHidden().WithDiscoveryContext(...)) so the
Discover(ctx, l, opts) call runs with NoHidden enabled and the assertion that
components.Filter(component.UnitKind).Paths() contains stackHiddenDir is
meaningful.
| ctx := filter.ClassificationContext{} | ||
| status, reason, _ := classifier.Classify(c, ctx) | ||
|
|
||
| // Compare basic component properties | ||
| require.Len(t, components, len(wantDiscovery)) | ||
|
|
||
| components = components.Sort() | ||
| wantDiscovery = wantDiscovery.Sort() | ||
|
|
||
| for i, c := range components { | ||
| want := wantDiscovery[i] | ||
| assert.Equal(t, evalPath(want.Path()), evalPath(c.Path()), "Component path mismatch at index %d", i) | ||
| assert.Equal(t, want.Kind(), c.Kind(), "Component kind mismatch at index %d", i) | ||
| assert.Equal(t, want.External(), c.External(), "Component external flag mismatch at index %d", i) | ||
|
|
||
| // Compare dependencies | ||
| cfgDeps := c.Dependencies().Sort() | ||
| wantDeps := want.Dependencies().Sort() | ||
| require.Len(t, cfgDeps, len(wantDeps), "Dependencies count mismatch for %s", c.Path()) | ||
|
|
||
| for j, dep := range cfgDeps { | ||
| wantDep := wantDeps[j] | ||
| assert.Equal(t, evalPath(wantDep.Path()), evalPath(dep.Path()), "Dependency path mismatch at component %d, dependency %d", i, j) | ||
| assert.Equal(t, wantDep.Kind(), dep.Kind(), "Dependency kind mismatch at component %d, dependency %d", i, j) | ||
| assert.Equal(t, wantDep.External(), dep.External(), "Dependency external flag mismatch at component %d, dependency %d", i, j) | ||
|
|
||
| // Compare nested dependencies (one level deep) | ||
| depDeps := dep.Dependencies().Sort() | ||
| wantDepDeps := wantDep.Dependencies().Sort() | ||
| require.Len(t, depDeps, len(wantDepDeps), "Nested dependencies count mismatch for %s -> %s", c.Path(), dep.Path()) | ||
|
|
||
| for k, nestedDep := range depDeps { | ||
| wantNestedDep := wantDepDeps[k] | ||
| assert.Equal(t, evalPath(wantNestedDep.Path()), evalPath(nestedDep.Path()), "Nested dependency path mismatch") | ||
| } | ||
| } | ||
| } | ||
| assert.Equal(t, tt.expectStatus, status, "status mismatch") | ||
| assert.Equal(t, tt.expectReason, reason, "reason mismatch") |
There was a problem hiding this comment.
Error return value from Classify is ignored.
The third return value (error) from classifier.Classify(c, ctx) is discarded. For thorough test coverage, consider asserting that no error occurs.
💚 Proposed fix
ctx := filter.ClassificationContext{}
- status, reason, _ := classifier.Classify(c, ctx)
+ status, reason, classifyErr := classifier.Classify(c, ctx)
+ require.NoError(t, classifyErr)
assert.Equal(t, tt.expectStatus, status, "status mismatch")
assert.Equal(t, tt.expectReason, reason, "reason mismatch")📝 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.
| ctx := filter.ClassificationContext{} | |
| status, reason, _ := classifier.Classify(c, ctx) | |
| // Compare basic component properties | |
| require.Len(t, components, len(wantDiscovery)) | |
| components = components.Sort() | |
| wantDiscovery = wantDiscovery.Sort() | |
| for i, c := range components { | |
| want := wantDiscovery[i] | |
| assert.Equal(t, evalPath(want.Path()), evalPath(c.Path()), "Component path mismatch at index %d", i) | |
| assert.Equal(t, want.Kind(), c.Kind(), "Component kind mismatch at index %d", i) | |
| assert.Equal(t, want.External(), c.External(), "Component external flag mismatch at index %d", i) | |
| // Compare dependencies | |
| cfgDeps := c.Dependencies().Sort() | |
| wantDeps := want.Dependencies().Sort() | |
| require.Len(t, cfgDeps, len(wantDeps), "Dependencies count mismatch for %s", c.Path()) | |
| for j, dep := range cfgDeps { | |
| wantDep := wantDeps[j] | |
| assert.Equal(t, evalPath(wantDep.Path()), evalPath(dep.Path()), "Dependency path mismatch at component %d, dependency %d", i, j) | |
| assert.Equal(t, wantDep.Kind(), dep.Kind(), "Dependency kind mismatch at component %d, dependency %d", i, j) | |
| assert.Equal(t, wantDep.External(), dep.External(), "Dependency external flag mismatch at component %d, dependency %d", i, j) | |
| // Compare nested dependencies (one level deep) | |
| depDeps := dep.Dependencies().Sort() | |
| wantDepDeps := wantDep.Dependencies().Sort() | |
| require.Len(t, depDeps, len(wantDepDeps), "Nested dependencies count mismatch for %s -> %s", c.Path(), dep.Path()) | |
| for k, nestedDep := range depDeps { | |
| wantNestedDep := wantDepDeps[k] | |
| assert.Equal(t, evalPath(wantNestedDep.Path()), evalPath(nestedDep.Path()), "Nested dependency path mismatch") | |
| } | |
| } | |
| } | |
| assert.Equal(t, tt.expectStatus, status, "status mismatch") | |
| assert.Equal(t, tt.expectReason, reason, "reason mismatch") | |
| ctx := filter.ClassificationContext{} | |
| status, reason, classifyErr := classifier.Classify(c, ctx) | |
| require.NoError(t, classifyErr) | |
| assert.Equal(t, tt.expectStatus, status, "status mismatch") | |
| assert.Equal(t, tt.expectReason, reason, "reason mismatch") |
🤖 Prompt for AI Agents
In `@internal/discovery/discovery_test.go` around lines 205 - 209, The test
currently ignores the error return from classifier.Classify(c, ctx); capture the
third return value (err) when calling classifier.Classify in the test (alongside
status and reason) and assert that err is nil (e.g., use require.NoError(t, err)
or assert.NoError(t, err)) before checking status and reason to ensure the call
succeeded; the call site to update is the classifier.Classify(c, ctx) invocation
that uses filter.ClassificationContext{}.
| // mergeResults merges discovered and candidate results from a phase output. | ||
| func mergeResults(output PhaseOutput) ([]DiscoveryResult, []DiscoveryResult, []error) { | ||
| var ( | ||
| discovered []DiscoveryResult | ||
| candidates []DiscoveryResult | ||
| errs []error | ||
| ) | ||
|
|
||
| // Drain all channels | ||
| done := false | ||
| for !done { | ||
| select { | ||
| case result, ok := <-output.Discovered: | ||
| if ok { | ||
| discovered = append(discovered, result) | ||
| } | ||
| case result, ok := <-output.Candidates: | ||
| if ok { | ||
| candidates = append(candidates, result) | ||
| } | ||
| case err, ok := <-output.Errors: | ||
| if ok && err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| case <-output.Done: | ||
| done = true | ||
| } | ||
| } | ||
|
|
||
| // Drain remaining items after done signal | ||
| for result := range output.Discovered { | ||
| discovered = append(discovered, result) | ||
| } | ||
|
|
||
| for result := range output.Candidates { | ||
| candidates = append(candidates, result) | ||
| } | ||
|
|
||
| for err := range output.Errors { | ||
| if err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the PhaseOutput definition
rg -n "type PhaseOutput" internal/discovery -g'*.go' -A 10Repository: gruntwork-io/terragrunt
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Find all files in the discovery package
git ls-files internal/discovery | grep '\.go$'Repository: gruntwork-io/terragrunt
Length of output: 754
🏁 Script executed:
#!/bin/bash
# Search for where Done channel is closed
rg -n "close.*Done" internal/discovery -g'*.go' -B 2 -A 2Repository: gruntwork-io/terragrunt
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for where Discovered/Candidates/Errors channels are closed
rg -n "close\(" internal/discovery -g'*.go' -B 3 -A 1 | head -100Repository: gruntwork-io/terragrunt
Length of output: 3333
Verify defer execution order for channel closure in phase implementations.
The channel-closure order does have a potential issue: all phase implementations use defer statements in LIFO order, causing Done to close before Discovered, Candidates, and Errors. However, this does not cause mergeResults to hang because all defer statements execute atomically when the goroutine exits—there is no window for new values to arrive on the data channels after Done closes.
That said, the current pattern is fragile. To improve clarity and robustness, consider explicitly closing the data channels before Done (by closing them before returning from the phase goroutine, rather than relying on defer order), or document the expected closure sequence.
🤖 Prompt for AI Agents
In `@internal/discovery/helpers.go` around lines 132 - 175, mergeResults depends
on data channels (PhaseOutput.Discovered, .Candidates, .Errors) being closed
before the Done channel to avoid fragile ordering from LIFO defers; update phase
goroutines (the implementations that write to these channels) to explicitly
close Discovered, Candidates and Errors before signaling Done (i.e., close data
channels first, then close/send on Done) rather than relying on defer ordering,
or add documentation in those phase functions describing the required close
sequence so mergeResults can safely range over the channels after Done.
| if err := discoveryGroup.Wait(); err != nil { | ||
| select { | ||
| case errors <- err: | ||
| default: | ||
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
Error silently dropped when channel is full.
The default case silently discards errors if the error channel is full. Consider increasing the buffer size or using a blocking send (with context cancellation) to ensure errors aren't lost.
🛠️ Suggested alternatives
Option 1 - Use context-aware blocking send:
if err := discoveryGroup.Wait(); err != nil {
select {
case errors <- err:
- default:
+ case <-ctx.Done():
}
return
}Option 2 - Increase buffer size to accommodate potential errors:
- errors := make(chan error, p.numWorkers)
+ errors := make(chan error, p.numWorkers*channelBufferMultiplier)🤖 Prompt for AI Agents
In `@internal/discovery/phase_worktree.go` around lines 162 - 168, The current
handling after discoveryGroup.Wait() drops errors when the errors channel is
full; change it to a context-aware blocking send so errors aren't lost: replace
the non-blocking select (select { case errors <- err: default: }) with a
blocking send that waits for either the send to succeed or context cancellation
(select { case errors <- err: case <-ctx.Done(): }) using the existing ctx
passed into this scope (or add one if missing); alternatively, if you prefer
buffering, increase the capacity where the errors channel is created to
accommodate expected concurrent errors (reference discoveryGroup.Wait(), the
errors channel variable, and ctx).
| case *PrefixExpression: | ||
| if node.Operator != "!" { | ||
| return false, nil | ||
| } | ||
|
|
||
| return MatchComponent(c, node.Right) | ||
|
|
||
| case *InfixExpression: | ||
| if node.Operator != "|" { | ||
| return false, nil | ||
| } | ||
|
|
||
| leftMatch, err := MatchComponent(c, node.Left) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if !leftMatch { | ||
| return false, nil | ||
| } | ||
|
|
||
| return MatchComponent(c, node.Right) | ||
|
|
There was a problem hiding this comment.
Fix negation and OR evaluation in MatchComponent.
!expr should invert the match result, and | should behave as OR. The current logic evaluates as AND and ignores negation.
🐛 Proposed fix
case *PrefixExpression:
if node.Operator != "!" {
return false, nil
}
- return MatchComponent(c, node.Right)
+ match, err := MatchComponent(c, node.Right)
+ if err != nil {
+ return false, err
+ }
+ return !match, nil
case *InfixExpression:
if node.Operator != "|" {
return false, nil
}
leftMatch, err := MatchComponent(c, node.Left)
if err != nil {
return false, err
}
- if !leftMatch {
- return false, nil
- }
-
- return MatchComponent(c, node.Right)
+ if leftMatch {
+ return true, nil
+ }
+ return MatchComponent(c, node.Right)📝 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.
| case *PrefixExpression: | |
| if node.Operator != "!" { | |
| return false, nil | |
| } | |
| return MatchComponent(c, node.Right) | |
| case *InfixExpression: | |
| if node.Operator != "|" { | |
| return false, nil | |
| } | |
| leftMatch, err := MatchComponent(c, node.Left) | |
| if err != nil { | |
| return false, err | |
| } | |
| if !leftMatch { | |
| return false, nil | |
| } | |
| return MatchComponent(c, node.Right) | |
| case *PrefixExpression: | |
| if node.Operator != "!" { | |
| return false, nil | |
| } | |
| match, err := MatchComponent(c, node.Right) | |
| if err != nil { | |
| return false, err | |
| } | |
| return !match, nil | |
| case *InfixExpression: | |
| if node.Operator != "|" { | |
| return false, nil | |
| } | |
| leftMatch, err := MatchComponent(c, node.Left) | |
| if err != nil { | |
| return false, err | |
| } | |
| if leftMatch { | |
| return true, nil | |
| } | |
| return MatchComponent(c, node.Right) | |
🤖 Prompt for AI Agents
In `@internal/filter/matcher.go` around lines 19 - 41, MatchComponent currently
treats PrefixExpression with operator "!" as a no-op and evaluates
InfixExpression with "|" like an AND; update it so PrefixExpression with
Operator "!" returns the inverted result of MatchComponent(c, node.Right) (i.e.,
call MatchComponent and return !match along with any error), and change
InfixExpression with Operator "|" to implement short-circuit OR: evaluate left
with MatchComponent(c, node.Left) and if true return true, otherwise evaluate
and return MatchComponent(c, node.Right); keep error propagation from the
recursive calls (e.g., in MatchComponent, PrefixExpression, InfixExpression).
| // This test is kind of wild. I don't know if it's worth keeping. | ||
| // Removing it for now to avoid blocking the merge of #5469 | ||
| // which is more important. | ||
|
|
||
| t.Skip("Skipping this test to avoid blocking the merge of #5469") | ||
|
|
There was a problem hiding this comment.
Avoid merging with permanently skipped integration coverage.
Skipping this test leaves a regression gap; please re-enable or quarantine with a tracked issue and timeline (or gate behind an explicit opt-out flag).
🤖 Prompt for AI Agents
In `@test/integration_debug_test.go` around lines 300 - 305, Replace the
unconditional t.Skip in test/integration_debug_test.go with a documented
quarantine: either re-enable the test, or gate its skip behind an explicit
opt-out (e.g., check an env var like SKIP_INTEGRATION_DEBUG) and call t.Skipf
only when the opt-out is set, and/or add a TODO comment referencing a tracked
issue and timeline (include the issue number like `#5469`) so the test remains
discoverable and the regression gap is tracked; ensure you update the test’s
t.Skip/t.Skipf call and add the issue reference and expected timeline in the
test file.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/discovery/phase_graph.go`:
- Around line 152-154: Assigning input.Components to allComponents then
appending can mutate the caller's slice via shared backing array; fix by making
an explicit copy of input.Components before appending
resultsToComponents(input.Candidates) so appends operate on a new slice.
Concretely, create a new slice with capacity
len(input.Components)+len(resultsToComponents(...)), copy input.Components into
it, then append the converted candidates and pass that safe slice into
component.NewThreadSafeComponents; update references to allComponents,
input.Components, resultsToComponents, and component.NewThreadSafeComponents
accordingly.
🧹 Nitpick comments (1)
internal/discovery/phase_worktree.go (1)
466-495: Consider O(n²) complexity for large component sets.
MatchComponentPairsuses nested loops, resulting in O(n×m) complexity. For typical Terragrunt projects this is fine, but extremely large monorepos might see slowdowns.♻️ Optional: Use map for O(n+m) matching
func MatchComponentPairs( fromComponents component.Components, toComponents component.Components, ) []ComponentPair { componentPairs := make([]ComponentPair, 0, max(len(fromComponents), len(toComponents))) + // Build lookup map for O(1) matching + toMap := make(map[string]component.Component, len(toComponents)) + for _, toComponent := range toComponents { + suffix := filepath.Clean(strings.TrimPrefix( + toComponent.Path(), + toComponent.DiscoveryContext().WorkingDir, + )) + toMap[suffix] = toComponent + } + for _, fromComponent := range fromComponents { fromComponentSuffix := strings.TrimPrefix( fromComponent.Path(), fromComponent.DiscoveryContext().WorkingDir, ) + cleanedSuffix := filepath.Clean(fromComponentSuffix) - for _, toComponent := range toComponents { - toComponentSuffix := strings.TrimPrefix( - toComponent.Path(), - toComponent.DiscoveryContext().WorkingDir, - ) - - if filepath.Clean(fromComponentSuffix) == filepath.Clean(toComponentSuffix) { - componentPairs = append(componentPairs, ComponentPair{ - FromComponent: fromComponent, - ToComponent: toComponent, - }) - } + if toComponent, ok := toMap[cleanedSuffix]; ok { + componentPairs = append(componentPairs, ComponentPair{ + FromComponent: fromComponent, + ToComponent: toComponent, + }) } } return componentPairs }
yhakbar
force-pushed
the
chore/refactor-discovery-for-more-nuanced-parse-handling
branch
from
3a3e333 to
63a949d
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/filter/candidacy.go`:
- Around line 113-124: The merge for InfixExpression in
analyzeExpressionCandidacy only combines boolean flags and neglects graph
metadata; update the block handling *InfixExpression so that when either
leftInfo or rightInfo has RequiresGraphDiscovery true you also merge
GraphDirection, DependencyDepth, DependentDepth and ExcludeTarget into info
(e.g., prefer the non-zero/stronger value or normalize/OR where sensible), so
info carries the graph-related fields from the side that required graph
discovery; keep the existing boolean merges for
RequiresFilesystemOnly/RequiresParsing/RequiresGraphDiscovery and ensure this
logic is applied inside the case for *InfixExpression in
analyzeExpressionCandidacy to preserve graph metadata in CandidacyInfo.
In `@internal/filter/classifier.go`:
- Around line 173-185: The negated PrefixExpression branch only records
node.Right in c.negatedExprs and may miss nested GraphExpression objects,
preventing the graph phase from running for exclusions; update the
PrefixExpression '!' handling in analyzeExpression (the case for
*PrefixExpression) to also extract graph expressions from node.Right by calling
GetGraphExpressions(node.Right) and appending each result to c.graphExprs (in
addition to existing c.negatedExprs and c.parseExprs logic) so HasGraphFilters()
will be true for negated graph filters; also add a matching branch in
MatchComponent to handle *GraphExpression nodes (invoking the same
graph-evaluation logic used for positive graph filters) so negated graph
expressions can be evaluated during classification.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/discovery/phase_test.go`:
- Around line 401-506: The test case in
TestCandidacyClassifier_AnalyzesFiltersCorrectly uses an invalid filter string
"..../foo"; replace that entry in the test table's filterStrings for the
"dependent graph filter" case with the correct graph-target syntax (e.g. use
"...{./foo}" for a relative path target or "...foo" for an identifier) so
filter.ParseFilterQueries and classifier.Analyze receive a valid graph filter;
update only the filterStrings value in that test row (no other logic changes to
NewClassifier, Analyze, HasGraphFilters, or GraphExpressions).
In `@internal/filter/classifier.go`:
- Around line 238-313: The code is adding inner expressions of negated filters
into c.parseExprs, causing negated parse filters to be treated as positive; fix
by ensuring negated expressions are NOT added to c.parseExprs (only to
c.negatedExprs) in the PrefixExpression handling (the block that appends to
c.negatedExprs and c.parseExprs), and/or make matchesAnyPositive skip any
expressions that are present in c.negatedExprs when iterating c.parseExprs (use
MatchComponent to compare). Update the PrefixExpression branch and
matchesAnyPositive so parseExprs contains only non-negated expressions and
negated expressions are checked only via matchesAnyNegated/negated logic.
🧹 Nitpick comments (1)
internal/discovery/discovery_integration_test.go (1)
934-961: Consider usingfmt.Sprintffor clearer unit naming.The expression
string(rune('a'+i))at line 942 works but is less readable than usingfmt.Sprintf("unit%c", 'a'+i)or simplyfmt.Sprintf("unit%d", i).♻️ Optional refactor for clarity
// Create a few test units - for i := range 5 { - dir := filepath.Join(tmpDir, "unit"+string(rune('a'+i))) + for i := 0; i < 5; i++ { + dir := filepath.Join(tmpDir, fmt.Sprintf("unit%c", 'a'+i)) require.NoError(t, os.MkdirAll(dir, 0755)) require.NoError(t, os.WriteFile(filepath.Join(dir, "terragrunt.hcl"), []byte(""), 0644)) }
| // TestCandidacyClassifier_AnalyzesFiltersCorrectly tests the candidacy classifier analysis. | ||
| func TestCandidacyClassifier_AnalyzesFiltersCorrectly(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| l := logger.CreateLogger() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| filterStrings []string | ||
| expectHasPositive bool | ||
| expectHasParseRequired bool | ||
| expectHasGraphFilters bool | ||
| expectGraphExprCount int | ||
| }{ | ||
| { | ||
| name: "empty filters", | ||
| filterStrings: []string{}, | ||
| expectHasPositive: false, | ||
| }, | ||
| { | ||
| name: "simple path filter", | ||
| filterStrings: []string{"./foo"}, | ||
| expectHasPositive: true, | ||
| }, | ||
| { | ||
| name: "negated path filter only", | ||
| filterStrings: []string{"!./foo"}, | ||
| expectHasPositive: false, | ||
| }, | ||
| { | ||
| name: "path filter with negation", | ||
| filterStrings: []string{"./foo", "!./bar"}, | ||
| expectHasPositive: true, | ||
| }, | ||
| { | ||
| name: "reading attribute filter", | ||
| filterStrings: []string{"reading=config/*"}, | ||
| expectHasPositive: true, | ||
| expectHasParseRequired: true, | ||
| }, | ||
| { | ||
| name: "dependency graph filter", | ||
| filterStrings: []string{"./foo..."}, | ||
| expectHasPositive: true, | ||
| expectHasGraphFilters: true, | ||
| expectGraphExprCount: 1, | ||
| }, | ||
| { | ||
| name: "dependent graph filter", | ||
| filterStrings: []string{"..../foo"}, | ||
| expectHasPositive: true, | ||
| expectHasGraphFilters: true, | ||
| expectGraphExprCount: 1, | ||
| }, | ||
| { | ||
| name: "exclude target graph filter", | ||
| filterStrings: []string{"^{./foo}..."}, | ||
| expectHasPositive: true, | ||
| expectHasGraphFilters: true, | ||
| expectGraphExprCount: 1, | ||
| }, | ||
| { | ||
| name: "multiple graph filters", | ||
| filterStrings: []string{"./foo...", "..../bar"}, | ||
| expectHasPositive: true, | ||
| expectHasGraphFilters: true, | ||
| expectGraphExprCount: 2, | ||
| }, | ||
| { | ||
| name: "name attribute filter", | ||
| filterStrings: []string{"name=my-app"}, | ||
| expectHasPositive: true, | ||
| }, | ||
| { | ||
| name: "type attribute filter", | ||
| filterStrings: []string{"type=unit"}, | ||
| expectHasPositive: true, | ||
| }, | ||
| { | ||
| name: "external attribute filter", | ||
| filterStrings: []string{"external=true"}, | ||
| expectHasPositive: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| filters, err := filter.ParseFilterQueries(l, tt.filterStrings) | ||
| require.NoError(t, err) | ||
|
|
||
| classifier := filter.NewClassifier(l) | ||
| err = classifier.Analyze(filters) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, tt.expectHasPositive, classifier.HasPositiveFilters(), "HasPositiveFilters mismatch") | ||
| assert.Equal(t, tt.expectHasParseRequired, classifier.HasParseRequiredFilters(), "HasParseRequiredFilters mismatch") | ||
| assert.Equal(t, tt.expectHasGraphFilters, classifier.HasGraphFilters(), "HasGraphFilters mismatch") | ||
|
|
||
| if tt.expectGraphExprCount > 0 { | ||
| assert.Len(t, classifier.GraphExpressions(), tt.expectGraphExprCount, "GraphExpressions count mismatch") | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for 4-dot syntax patterns
echo "=== Searching for 4-dot patterns ==="
rg -n '\.\.\.\.\/' --type=go
echo -e "\n=== Searching for 3-dot patterns (dependency graph filters) ==="
rg -n '\.\.\.[^.]' --type=go | head -40
echo -e "\n=== Searching for dependent filter patterns (leading dots) ==="
rg -n '\.\.\/.+' --type=go | head -30Repository: gruntwork-io/terragrunt
Length of output: 4919
🏁 Script executed:
#!/bin/bash
# Find and examine the filter parsing implementation
echo "=== Searching for filter parsing functions ==="
fd -t f -e go -e md -x grep -l "ParseFilterQueries\|dependent.*filter\|graph.*filter" {} \;
echo -e "\n=== Look for filter syntax documentation ==="
fd -t f \( -name "*.md" -o -name "*.go" \) -x grep -l "\.\.\/\|graph.*filter" {} \; | head -20Repository: gruntwork-io/terragrunt
Length of output: 1715
🏁 Script executed:
#!/bin/bash
# Find the filter implementation files
find . -type f -name "*filter*.go" | grep -v node_modules | head -20Repository: gruntwork-io/terragrunt
Length of output: 376
🏁 Script executed:
#!/bin/bash
# Look for filter examples in integration tests
echo "=== Integration filter graph test file ==="
grep -n '\.\.\.\|dependent\|graph.*filter' ./test/integration_filter_graph_test.go | head -50Repository: gruntwork-io/terragrunt
Length of output: 3317
🏁 Script executed:
#!/bin/bash
# Check the filter parser implementation
echo "=== Checking filter parser implementation ==="
if [ -f ./internal/filter/parser.go ]; then
grep -n 'dependent\|graph\|dots\|\.\.\..*/' ./internal/filter/parser.go | head -30
fi
# Also check filters.go
if [ -f ./internal/filter/filters.go ]; then
grep -n 'dependent\|graph\|\.\.\..*/' ./internal/filter/filters.go | head -30
fiRepository: gruntwork-io/terragrunt
Length of output: 884
🏁 Script executed:
#!/bin/bash
# Search for "..../foo" pattern specifically in test files (using different regex approach)
echo "=== Searching for dependent filter syntax in test files ==="
grep -n '\.\.\.\.' ./internal/discovery/phase_test.go
grep -n '\.\.\.\.' ./test/integration_filter_graph_test.go
grep -n '\.\.\.\.' ./test/integration_filter_test.goRepository: gruntwork-io/terragrunt
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Look at the actual test case around line 450 in phase_test.go
echo "=== Context around line 450 in phase_test.go ==="
sed -n '445,460p' ./internal/discovery/phase_test.go
# Also get line 429 to see the "dependency graph filter" test for comparison
echo -e "\n=== Dependency graph filter test (line ~429) ==="
sed -n '429,442p' ./internal/discovery/phase_test.goRepository: gruntwork-io/terragrunt
Length of output: 1026
🏁 Script executed:
#!/bin/bash
# Search for documentation or comments about filter syntax
echo "=== Searching for filter syntax documentation ==="
grep -n 'dependent.*graph\|graph.*dependent' ./internal/filter/doc.go 2>/dev/null || echo "doc.go not found"
# Check if there's an examples file
if [ -f ./internal/filter/examples_test.go ]; then
grep -n '\.\.' ./internal/filter/examples_test.go | head -40
fiRepository: gruntwork-io/terragrunt
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Check the examples file for filter syntax patterns
if [ -f ./internal/filter/examples_test.go ]; then
echo "=== Filter examples ==="
grep -n '\.\.\.' ./internal/filter/examples_test.go | head -60
fiRepository: gruntwork-io/terragrunt
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Look for the GraphExpression structure definition
echo "=== Searching for GraphExpression definition ==="
grep -n 'type GraphExpression' ./internal/filter/*.go
grep -n 'IncludeDependents\|IncludeDependencies' ./internal/filter/*.go | head -20Repository: gruntwork-io/terragrunt
Length of output: 1833
🏁 Script executed:
#!/bin/bash
# Check all test cases in phase_test.go for graph filter patterns
echo "=== All graph filter test cases in phase_test.go ==="
grep -B 2 -A 1 'GraphExprCount' ./internal/discovery/phase_test.goRepository: gruntwork-io/terragrunt
Length of output: 873
🏁 Script executed:
#!/bin/bash
# Look at the parser file to understand syntax parsing
echo "=== Parser implementation ==="
sed -n '90,120p' ./internal/filter/parser.go
echo -e "\n=== More parser context ==="
sed -n '1,50p' ./internal/filter/parser.goRepository: gruntwork-io/terragrunt
Length of output: 1955
🏁 Script executed:
#!/bin/bash
# Check the AST file for GraphExpression construction
echo "=== GraphExpression construction ==="
sed -n '230,280p' ./internal/filter/ast.goRepository: gruntwork-io/terragrunt
Length of output: 1290
🏁 Script executed:
#!/bin/bash
# Look at how the parser processes dots and slashes
echo "=== Parsing dots and operators ==="
grep -n '\.\.\|parseDepth\|ellipsis' ./internal/filter/parser.go | head -40Repository: gruntwork-io/terragrunt
Length of output: 911
🏁 Script executed:
#!/bin/bash
# Check examples in integration_filter_graph_test.go for actual filter syntax
echo "=== Integration test filter patterns ==="
grep -B 2 -A 2 'filterQuery.*\.\.' ./test/integration_filter_graph_test.go | head -80Repository: gruntwork-io/terragrunt
Length of output: 3042
🏁 Script executed:
#!/bin/bash
# Check how the lexer handles dots
echo "=== Lexer implementation ==="
grep -n 'ELLIPSIS\|\.\.\..*case' ./internal/filter/lexer.go | head -40
echo -e "\n=== Lexer token definitions ==="
grep -n 'const\|ELLIPSIS' ./internal/filter/lexer.go | head -50Repository: gruntwork-io/terragrunt
Length of output: 236
🏁 Script executed:
#!/bin/bash
# Look for tokenization of dots
echo "=== Dot tokenization logic ==="
sed -n '1,200p' ./internal/filter/lexer.go | grep -A 5 -B 5 '\.'Repository: gruntwork-io/terragrunt
Length of output: 5045
🏁 Script executed:
#!/bin/bash
# Check readPath implementation
echo "=== readPath implementation ==="
grep -A 30 'func.*readPath' ./internal/filter/lexer.goRepository: gruntwork-io/terragrunt
Length of output: 851
🏁 Script executed:
#!/bin/bash
# Let's trace through what would tokenize for "../foo" and "..../foo"
echo "=== Testing lexer with specific inputs ==="
cd ./internal/filter && python3 << 'EOF'
# Let's manually trace what the lexer would do with "..../foo"
input = "..../foo"
# Lexer logic from what we've seen:
# Position 0: ch='.'
# - peekChar returns '.' (position 1)
# - Check if readPosition+1 < len and input[readPosition+1] == '.' → YES (position 2 is '.')
# - Creates ELLIPSIS token "..."
# - readChar 3 times → now at position 3
# - readChar once more before returning (from the tok assignment) → position 4
# After ELLIPSIS token, next call to NextToken:
# Position 4: ch='/'
# This would be handled by the '/' case in the switch
# So "..../foo" tokenizes as:
# 1. ELLIPSIS ("...")
# 2. Then "/" which calls readPath
# The readPath would include that leading "/"
# So it would read "./foo" as the path
print("Input: " + input)
print("Expected tokens:")
print("1. ELLIPSIS: '...'")
print("2. PATH: '/foo' or similar")
print()
print("Compare to correct syntax '...foo':")
print("1. ELLIPSIS: '...'")
print("2. IDENT: 'foo'")
EOFRepository: gruntwork-io/terragrunt
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Verify the correct syntax by examining all dependent filter examples
echo "=== All dependent filter examples in tests ==="
grep -r '\.\.\.[^.]' ./test/ ./internal/discovery/ ./internal/filter/ --include='*.go' | grep -E 'filterQuery|filterStrings|"\.\.\.([a-zA-Z_])' | head -30Repository: gruntwork-io/terragrunt
Length of output: 2454
🏁 Script executed:
#!/bin/bash
# Search for all instances of 4+ dots in filter contexts
echo "=== Searching for 4+ dot patterns in filter contexts ==="
grep -r '\.\.\.\.\|\.\.\.\.\..*foo' ./test/ ./internal/ --include='*.go' 2>/dev/null | grep -v node_modules
# Compare with correct 3-dot patterns
echo -e "\n=== Correct 3-dot dependent filter patterns ==="
grep -r '"\.\.\.[a-zA-Z_-]' ./test/ ./internal/discovery/ ./internal/filter/ --include='*.go' | grep -v node_modules | head -20Repository: gruntwork-io/terragrunt
Length of output: 3982
🏁 Script executed:
#!/bin/bash
# Check the parser test and candidacy test for these patterns
echo "=== Parser test with 4+ dots ==="
grep -B 5 -A 5 '1......2' ./internal/filter/parser_test.go
echo -e "\n=== Complex test with 4 dots ==="
grep -B 5 -A 5 '1..../apps' ./internal/filter/complex_test.go
echo -e "\n=== Candidacy test with 4 dots ==="
grep -B 5 -A 5 '..../foo' ./internal/filter/candidacy_test.goRepository: gruntwork-io/terragrunt
Length of output: 5896
🏁 Script executed:
#!/bin/bash
# Check what the expected behavior is for these patterns
echo "=== Checking expected output for ..../foo patterns ==="
grep -B 10 -A 5 '"..../foo"' ./internal/discovery/discovery_test.go | head -40
echo -e "\n=== Checking parser documentation ==="
grep -n 'relative.*path\|parent.*directory\|\.\.\/' ./internal/filter/parser.goRepository: gruntwork-io/terragrunt
Length of output: 1138
🏁 Script executed:
#!/bin/bash
# Look for comments explaining the 4-dot syntax
echo "=== Searching for documentation of graph filter syntax ==="
grep -B 5 -A 5 'N\...\|depth' ./internal/filter/ast.go | head -50Repository: gruntwork-io/terragrunt
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Check the discovery_test.go to see if it has the same pattern
echo "=== discovery_test.go dependent filter test ==="
grep -B 5 -A 8 '"..../foo"' ./internal/discovery/discovery_test.go
# Check if these tests are copied or independently created
echo -e "\n=== Check if discovery_test and phase_test have matching test cases ==="
grep -n 'dependent graph filter' ./internal/discovery/discovery_test.go ./internal/discovery/phase_test.goRepository: gruntwork-io/terragrunt
Length of output: 1603
🏁 Script executed:
#!/bin/bash
# See how the parser actually handles ..../foo vs ...foo
echo "=== Parser behavior with different syntaxes ==="
cat > /tmp/test_parse.go << 'EOF'
package main
import (
"fmt"
"regexp"
)
func main() {
// Simulating what the lexer does with different inputs
inputs := []string{"...foo", "..../foo", "...b-dependency", "..../bar"}
for _, input := range inputs {
fmt.Printf("Input: %s\n", input)
// Check if it starts with .../ (3 dots + slash)
if match, _ := regexp.MatchString(`^\.\.\./`, input); match {
fmt.Println(" → Pattern: .../ (3 dots + slash)")
} else if match, _ := regexp.MatchString(`^\.\.\.`, input); match {
fmt.Println(" → Pattern: ... (3 dots)")
}
fmt.Println()
}
}
EOF
cd /tmp && go run test_parse.goRepository: gruntwork-io/terragrunt
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Check all test case names and patterns across test files
echo "=== Comparing filter patterns in all test files ==="
echo "Phase test patterns:"
grep 'name.*"dependent\|name.*"graph' ./internal/discovery/phase_test.go -A 2 | grep filterStrings
echo -e "\nDiscovery test patterns:"
grep 'name.*"dependent\|name.*"graph' ./internal/discovery/discovery_test.go -A 2 | grep filterStrings
echo -e "\nCandidacy test patterns:"
grep 'name.*"dependent' ./internal/filter/candidacy_test.go -A 2 | grep filterStrRepository: gruntwork-io/terragrunt
Length of output: 443
🏁 Script executed:
#!/bin/bash
# Look at the parser to see how it handles the target after ELLIPSIS
echo "=== Parser handling of target after ELLIPSIS ==="
sed -n '95,145p' ./internal/filter/parser.go
echo -e "\n=== Check what types of targets can follow ELLIPSIS ==="
sed -n '145,180p' ./internal/filter/parser.goRepository: gruntwork-io/terragrunt
Length of output: 2576
🏁 Script executed:
#!/bin/bash
# Let's verify how "...foo" vs "..../foo" are actually parsed by checking if tests pass
echo "=== Check if phase_test.go currently passes ==="
cd ./internal/discovery && go test -run TestCandidacyClassifier_AnalyzesFiltersCorrectly -v 2>&1 | head -50Repository: gruntwork-io/terragrunt
Length of output: 3120
🏁 Script executed:
#!/bin/bash
# Look for documentation on what the correct dependent filter syntax should be
echo "=== Search for comments on graph filter syntax ==="
grep -r 'dependent.*filter\|\.\.\..*target\|target.*dependent' ./internal/filter/ --include='*.go' -B 2 -A 2 | head -80Repository: gruntwork-io/terragrunt
Length of output: 5429
🏁 Script executed:
#!/bin/bash
# Look at the full doc.go file for filter syntax documentation
echo "=== Full filter documentation ==="
cat ./internal/filter/doc.goRepository: gruntwork-io/terragrunt
Length of output: 9590
Change "..../foo" to "...{./foo}" or "...foo" depending on the intended target.
At line 450, the filter uses ..../foo which parses as a dependent graph filter targeting the absolute path /foo. The documentation shows the correct syntax for path targets is braced syntax like ...{./foo} (for relative paths) or ...foo (for identifiers/names). All integration tests consistently use ...identifier without slashes.
🤖 Prompt for AI Agents
In `@internal/discovery/phase_test.go` around lines 401 - 506, The test case in
TestCandidacyClassifier_AnalyzesFiltersCorrectly uses an invalid filter string
"..../foo"; replace that entry in the test table's filterStrings for the
"dependent graph filter" case with the correct graph-target syntax (e.g. use
"...{./foo}" for a relative path target or "...foo" for an identifier) so
filter.ParseFilterQueries and classifier.Analyze receive a valid graph filter;
update only the filterStrings value in that test row (no other logic changes to
NewClassifier, Analyze, HasGraphFilters, or GraphExpressions).
| case *PrefixExpression: | ||
| // Right now, the only prefix operator is "!". | ||
| // If we encounter an unknown operator, just analyze the inner expression. | ||
| if node.Operator != "!" { | ||
| c.analyzeExpression(node.Right, filterIndex) | ||
| break | ||
| } | ||
|
|
||
| c.negatedExprs = append(c.negatedExprs, node.Right) | ||
| if _, requiresParse := node.Right.RequiresParse(); requiresParse { | ||
| c.parseExprs = append(c.parseExprs, node.Right) | ||
| } | ||
|
|
||
| c.extractNegatedGraphExpressions(node.Right, filterIndex) | ||
|
|
||
| case *InfixExpression: | ||
| c.analyzeExpression(node.Left, filterIndex) | ||
| c.analyzeExpression(node.Right, filterIndex) | ||
| } | ||
| } | ||
|
|
||
| // extractNegatedGraphExpressions walks through a negated expression and extracts | ||
| // any graph expressions found within it. This ensures that filters like "!...db" | ||
| // or "!db..." trigger the graph discovery phase. | ||
| func (c *Classifier) extractNegatedGraphExpressions(expr Expression, filterIndex int) { | ||
| WalkExpressions(expr, func(e Expression) bool { | ||
| if graphExpr, ok := e.(*GraphExpression); ok { | ||
| info := &GraphExpressionInfo{ | ||
| Target: graphExpr.Target, | ||
| FullExpression: graphExpr, | ||
| Index: filterIndex, | ||
| IncludeDependencies: graphExpr.IncludeDependencies, | ||
| IncludeDependents: graphExpr.IncludeDependents, | ||
| ExcludeTarget: graphExpr.ExcludeTarget, | ||
| DependencyDepth: graphExpr.DependencyDepth, | ||
| DependentDepth: graphExpr.DependentDepth, | ||
| IsNegated: true, | ||
| } | ||
| c.graphExprs = append(c.graphExprs, info) | ||
| } | ||
|
|
||
| return true | ||
| }) | ||
| } | ||
|
|
||
| // matchesAnyNegated checks if the component matches any negated expression. | ||
| func (c *Classifier) matchesAnyNegated(comp component.Component) bool { | ||
| return slices.ContainsFunc(c.negatedExprs, func(expr Expression) bool { | ||
| match, _ := MatchComponent(comp, expr) | ||
| return match | ||
| }) | ||
| } | ||
|
|
||
| // matchesAnyPositive checks if the component matches any positive (non-negated) expression. | ||
| func (c *Classifier) matchesAnyPositive(comp component.Component, ctx ClassificationContext) bool { | ||
| if c.matchesFilesystemExpression(comp) { | ||
| return true | ||
| } | ||
|
|
||
| if c.matchesGraphExpressionTarget(comp) >= 0 { | ||
| return true | ||
| } | ||
|
|
||
| if c.matchesGitExpression(comp) { | ||
| return true | ||
| } | ||
|
|
||
| if !ctx.ParseDataAvailable || len(c.parseExprs) == 0 { | ||
| return false | ||
| } | ||
|
|
||
| return slices.ContainsFunc(c.parseExprs, func(expr Expression) bool { | ||
| match, _ := MatchComponent(comp, expr) | ||
| return match | ||
| }) | ||
| } |
There was a problem hiding this comment.
Negated parse filters are treated as positive matches.
parseExprs currently stores the inner expression for negated filters, and matchesAnyPositive treats all parse expressions as positive. This makes filters like !reading=shared.hcl classify matching components as discovered instead of excluded. Consider preserving the negation in parseExprs and skipping negated expressions in matchesAnyPositive.
🔧 Suggested fix
case *PrefixExpression:
// Right now, the only prefix operator is "!".
// If we encounter an unknown operator, just analyze the inner expression.
if node.Operator != "!" {
c.analyzeExpression(node.Right, filterIndex)
break
}
c.negatedExprs = append(c.negatedExprs, node.Right)
if _, requiresParse := node.Right.RequiresParse(); requiresParse {
- c.parseExprs = append(c.parseExprs, node.Right)
+ // Preserve the negation so later evaluation can respect it.
+ c.parseExprs = append(c.parseExprs, node)
}
c.extractNegatedGraphExpressions(node.Right, filterIndex) func (c *Classifier) matchesAnyPositive(comp component.Component, ctx ClassificationContext) bool {
if c.matchesFilesystemExpression(comp) {
return true
}
if c.matchesGraphExpressionTarget(comp) >= 0 {
return true
}
if c.matchesGitExpression(comp) {
return true
}
if !ctx.ParseDataAvailable || len(c.parseExprs) == 0 {
return false
}
return slices.ContainsFunc(c.parseExprs, func(expr Expression) bool {
+ if IsNegated(expr) {
+ return false
+ }
match, _ := MatchComponent(comp, expr)
return match
})
}🤖 Prompt for AI Agents
In `@internal/filter/classifier.go` around lines 238 - 313, The code is adding
inner expressions of negated filters into c.parseExprs, causing negated parse
filters to be treated as positive; fix by ensuring negated expressions are NOT
added to c.parseExprs (only to c.negatedExprs) in the PrefixExpression handling
(the block that appends to c.negatedExprs and c.parseExprs), and/or make
matchesAnyPositive skip any expressions that are present in c.negatedExprs when
iterating c.parseExprs (use MatchComponent to compare). Update the
PrefixExpression branch and matchesAnyPositive so parseExprs contains only
non-negated expressions and negated expressions are checked only via
matchesAnyNegated/negated logic.
fix: Addressing lints fix: Fixing dependent discovery fix: Adding some testing fix: Adding more testing fix: Addressing lints fix: Addressing gaps from read files fix: Addressing gaps from original terragrunt dir fix: Addressing double-negations fix: Adding exclude target testing fix: Handling edge case for parse exclude chore: Adding benchmarks fix: Fixing #5467 fix: Addressing weird tests fix: Addressing lints chore: Adding more steps to the bench fix: Fixing dependent discovery out of discovery working dir fix: Cleaning up context fix: Addressing lints chore: Cleanup chore: Deduplicating classifier logic into the `filter` package chore: Getting rid of hidden memo fix: Addressing lints fix: Cleanup fix: Cleanup chore: Preallocating where possible fix: Cleanup fix: Fixing benchmark
yhakbar
force-pushed
the
chore/refactor-discovery-for-more-nuanced-parse-handling
branch
from
c7940c5 to
30cf30d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/discovery/phase_graph.go`:
- Around line 817-823: The parent.AddDependency call is adding a freshly
constructed depComponent before threadSafeComponents.EnsureComponent can return
the canonical/existing component; move the dependency addition until after
calling threadSafeComponents.EnsureComponent so the parent references the
canonical component. Specifically, create depComponent and set external via
isExternal(...), then call threadSafeComponents.EnsureComponent(...) and use the
returned component (not the local depComponent) when calling
parent.AddDependency(...); ensure you still propagate external state if
EnsureComponent returns an existing component.
🧹 Nitpick comments (12)
internal/discovery/phase_relationship.go (2)
107-136: Dead code:g.Wait()error will always be nil.The goroutines passed to
g.Go()always returnnil(line 127), even when errors occur. Errors are collected separately viaerrMu/errs. This meansg.Wait()at line 131 will never return a non-nil error, making lines 131-136 effectively dead code.This pattern is intentional for continuing on partial failures, but consider removing the unreachable error handling or adding a comment explaining the design.
♻️ Optional: Remove dead code or add clarifying comment
- if err := g.Wait(); err != nil { - select { - case errChan <- err: - default: - } - } + // Wait for all goroutines to complete. Errors are collected via errMu/errs + // rather than through errgroup to allow partial failures. + _ = g.Wait()
224-225: Nested errgroup may cause excessive concurrency.
discoverRelationshipscreates a new errgroup withg.SetLimit(p.numWorkers)(line 225) inside the recursive call. When called fromrunRelationshipDiscoverywhich also uses an errgroup with the same limit, this can result in up tonumWorkers × numWorkersconcurrent goroutines, potentially overwhelming system resources.Consider either:
- Sharing a single worker pool across recursion levels.
- Reducing the limit for nested calls.
internal/filter/evaluator.go (1)
372-390: Consider removing commented-out code.This block of commented-out code (lines 372-390) adds noise without providing value. If this logic might be needed in the future, consider documenting the scenario in a comment without the full implementation, or tracking it in an issue.
internal/discovery/helpers.go (1)
164-209: Channel draining logic is correct but fragile by design.The
mergeResultsfunction correctly drains channels in two phases:
- Select loop until
Donesignal.- Range over remaining items after
Done.This relies on the phase implementations closing data channels before or atomically with
Done. The current implementation works because alldeferstatements in phase goroutines execute atomically when the goroutine exits. However, the previous review noted this pattern is fragile.Consider adding a brief comment documenting the expected closure sequence for future maintainers.
internal/discovery/phase_filesystem.go (1)
127-164: Single consumer for file paths limits parallelism.The
filePathschannel is consumed by a single goroutine (lines 127-164), which processes files sequentially. Withg.SetLimit(p.numWorkers), only two goroutines run: the walker and the processor. Consider spawning multiple consumer goroutines to parallelize file processing.internal/discovery/phase_parse.go (3)
94-105: Discovered components parsed when readFiles/parseExclude/parseIncludes enabled may cause duplicate processing.When
readFiles,parseExclude, orparseIncludesis enabled,input.Componentsare added tocomponentsToParsewith statusStatusDiscovered. If a component appears in bothinput.Componentsandinput.Candidates, it could be processed twice. Consider deduplicating by path before processing.
292-307: Inconsistent error suppression between diagnostics handler and PartialParseConfigFile.The diagnostics handler at lines 282-288 suppresses all diagnostics when
suppressParseErrorsis true, but lines 294-307 still check forTerragruntConfigNotFoundErrorand other errors. This could lead to cases where diagnostics are suppressed but the overall parse still fails.
224-231: Parameter order:ctxshould be first per Go convention.The function signature has
cbeforectx, but Go convention placescontext.Contextas the first parameter.♻️ Suggested fix
func parseComponent( - c component.Component, ctx context.Context, + c component.Component, l log.Logger, opts *options.TerragruntOptions, suppressParseErrors bool, parserOptions []hclparse.Option, ) error {internal/discovery/phase_worktree.go (3)
266-298: Redundant error collection pattern.The error handling has redundancy: goroutines both return errors (which
g.Wait()captures) and append to theerrsslice. If a goroutine returns an error at line 281,g.Wait()at line 292 returns that same error, and it's also inerrs. This results in the error being reported twice if the caller inspects both.Consider simplifying to use only one approach - either return errors from goroutines (and rely on
g.Wait()) or collect inerrsand returnnilfrom goroutines.♻️ Proposed fix using errgroup's error handling only
- var ( - mu sync.Mutex - errs = make([]error, 0, len(stackDiff.Changed)) - ) - for _, changed := range stackDiff.Changed { g.Go(func() error { components, err := p.walkChangedStack(ctx, input, changed.FromStack, changed.ToStack) if err != nil { - mu.Lock() - - errs = append(errs, err) - - mu.Unlock() - return err } for _, c := range components { discoveredComponents.EnsureComponent(c) } return nil }) } if err := g.Wait(); err != nil { return nil, err } - - if len(errs) > 0 { - return nil, errors.Join(errs...) - }
424-448: Data race: concurrent writes tofromSHAandtoSHA.While the error variable race was fixed using
localErr, thefromSHAandtoSHAvariables declared at line 425 are still written concurrently by the two goroutines (lines 433 and 441). Although Go's memory model typically makes this safe afterWait()returns (since the writes complete beforeWait()returns), it's cleaner and more explicit to use the same local-variable pattern for the SHA values.♻️ Proposed fix for clarity and consistency
for _, pair := range componentPairs { - var fromSHA, toSHA string + var ( + fromSHA string + toSHA string + ) shaGroup, _ := errgroup.WithContext(ctx) shaGroup.SetLimit(min(runtime.NumCPU(), 2)) //nolint:mnd shaGroup.Go(func() error { - var localErr error - - fromSHA, localErr = GenerateDirSHA256(pair.FromComponent.Path()) - - return localErr + sha, err := GenerateDirSHA256(pair.FromComponent.Path()) + if err != nil { + return err + } + fromSHA = sha + return nil }) shaGroup.Go(func() error { - var localErr error - - toSHA, localErr = GenerateDirSHA256(pair.ToComponent.Path()) - - return localErr + sha, err := GenerateDirSHA256(pair.ToComponent.Path()) + if err != nil { + return err + } + toSHA = sha + return nil })
335-402: Inconsistent error handling pattern between methods.In
walkChangedStack, goroutines append errors toerrsbut returnnil(lines 349-356, 377-384), allowing both discoveries to run even if one fails. However, the checkif err = discoveryGroup.Wait(); err != nilat line 396 will never be true since goroutines always returnnil.This is fine if the intent is to collect all errors, but it's inconsistent with
discoverChangesInWorktreeStackswhere goroutines return the error directly.♻️ Proposed fix to remove dead code
- if err = discoveryGroup.Wait(); err != nil { - return nil, err - } + _ = discoveryGroup.Wait() if len(errs) > 0 { return nil, errors.Join(errs...) }internal/discovery/discovery.go (1)
453-474: Potential dead code in error handling.Since goroutines always return
nil(line 464),g.Wait()at line 468 will never return a non-nil error. The error handling at lines 468-471 is effectively dead code.♻️ Proposed fix to remove dead code
}) } - err := g.Wait() - if err != nil { - l.Debugf("Error building dependency graph: %v", err) - } + _ = g.Wait() return errs }
| depComponent := component.NewUnit(depPath) | ||
|
|
||
| if isExternal(parentCtx.WorkingDir, depPath) { | ||
| depComponent.SetExternal() | ||
| } | ||
|
|
||
| parent.AddDependency(depComponent) |
There was a problem hiding this comment.
Dependency added to parent before ensuring component exists.
At line 823, parent.AddDependency(depComponent) is called before threadSafeComponents.EnsureComponent at line 825. If EnsureComponent returns a different (existing) component, the parent's dependency list will reference the newly created component rather than the canonical one.
🐛 Proposed fix
depComponent := component.NewUnit(depPath)
if isExternal(parentCtx.WorkingDir, depPath) {
depComponent.SetExternal()
}
- parent.AddDependency(depComponent)
-
addedComponent, created := threadSafeComponents.EnsureComponent(depComponent)
+ parent.AddDependency(addedComponent)
+
if created {
copiedCtx := parentCtx.CopyWithNewOrigin(component.OriginGraphDiscovery)🤖 Prompt for AI Agents
In `@internal/discovery/phase_graph.go` around lines 817 - 823, The
parent.AddDependency call is adding a freshly constructed depComponent before
threadSafeComponents.EnsureComponent can return the canonical/existing
component; move the dependency addition until after calling
threadSafeComponents.EnsureComponent so the parent references the canonical
component. Specifically, create depComponent and set external via
isExternal(...), then call threadSafeComponents.EnsureComponent(...) and use the
returned component (not the local depComponent) when calling
parent.AddDependency(...); ensure you still propagate external state if
EnsureComponent returns an existing component.
Description
Redesigns the internal/discovery package to use a multi-phase pipeline architecture that lazily evaluates components based on filter requirements. This eliminates unnecessary parsing when there's a nuanced set of requirements for which components need parsing.
For example, say a user uses the following filter query:
Currently, the logic in discovery sees that dependency expression on
barand decides that everything discovered needs parsing by settingRequiresParsetotrue. That's a lot more coarse than things need to be. We don't, for example, need to parsebazunless it's in the dependency graph ofbar. We also don't need to parse anything that isn't in the dependency graph ofbarorfoo.The goal of this update is to classify the filter expressions provided and make decisions during discovery as to whether any particular component can be dropped earlier in discovery based on whether the filters users use make it impossible to end up in the final result set.
Key changes:
Components are classified into three statuses during discovery:
Phase Flow
--dag).Side-effects
This PR started out because #5469 was too hacky, and we needed a fix for #5467.
We were silently ignoring failed parsing due to logic like
ShouldSkipUnitWithoutTerraformandWithSuppressParseErrors.Removing this logic resulted in quite a lot of fallout, as we had tests that were silently passing due to the suppressions and skips. It's likely that there are users out there that had Terragrunt silently ignoring parsing errors, and that they'll need to start explicitly excluding things with negated filters or fix their configurations.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Documentation & Tests