chore: Tracking reads in components #5000
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
yhakbar
changed the title
📝 WalkthroughWalkthroughReplaces thread-safe map-based file read tracking in TerragruntOptions with slice-based tracking. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as Config Parser
participant ParsingCtx as ParsingContext
participant Discovery as Discovery
participant Component as Component
participant UnitResolver as Unit Resolver
participant Unit as Unit
Parser->>ParsingCtx: Initialize with FilesRead slice
Parser->>ParsingCtx: Append file paths to FilesRead during parsing
Discovery->>ParsingCtx: Get FilesRead after parsing
ParsingCtx-->>Discovery: Return []string of read files
Discovery->>Component: Populate Reading field from FilesRead
UnitResolver->>ParsingCtx: Extract FilesRead from parsing context
ParsingCtx-->>UnitResolver: Return file paths
UnitResolver->>Unit: Set Reading field with extracted paths
Note over Unit: Contains all files read during<br/>parsing for this unit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Rationale: Changes follow a consistent homogeneous pattern (adding Reading fields across multiple structs), but affect multiple layers of the system (parsing, discovery, unit resolution, runner). The architectural shift from thread-safe map-based tracking to slice-based propagation is straightforward but requires verification across integration points. Test coverage is added appropriately, and old race-condition test is removed as expected. 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/tracking-reads-in-components
branch
from
4630c51 to
83b46c1
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: 0
🧹 Nitpick comments (2)
internal/runner/common/unit_resolver.go (1)
896-896: Consider a map for large Reading slices (optional optimization).The current
slices.Containsimplementation is O(n) per unit checked. Ifunit.Readingor the number of units becomes large, consider using a map for O(1) lookups instead.For example:
+func (r *UnitResolver) flagUnitsThatRead(opts *options.TerragruntOptions, units Units) Units { + if len(opts.UnitsReading) == 0 { + return units + } + + // Build map of paths to check for O(1) lookup + pathsToCheck := make(map[string]bool, len(opts.UnitsReading)) + for _, path := range opts.UnitsReading { + if !filepath.IsAbs(path) { + path = filepath.Join(opts.WorkingDir, path) + path = filepath.Clean(path) + } + pathsToCheck[path] = true + } + + for _, unit := range units { + for _, readPath := range unit.Reading { + if pathsToCheck[readPath] { + unit.FlagExcluded = false + break + } + } + } + + return units +}internal/discovery/discovery_test.go (1)
577-635: LGTM! Comprehensive test for Reading field population.The test effectively verifies the entire flow: files are read during parsing → tracked in parsing context → propagated to component. Good coverage of
read_terragrunt_configandread_tfvars_file.Consider adding a test case for the
mark_as_readfunction as well to ensure complete coverage of all read-tracking mechanisms:// In terragrunt.hcl locals { shared_config = read_terragrunt_config("../shared.hcl") tfvars = read_tfvars_file("../shared.tfvars") explicitly_marked = mark_as_read("../explicitly-tracked.txt") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
config/config_helpers.go(4 hunks)config/parsing_context.go(2 hunks)internal/component/component.go(1 hunks)internal/discovery/discovery.go(1 hunks)internal/discovery/discovery_test.go(1 hunks)internal/runner/common/unit.go(1 hunks)internal/runner/common/unit_resolver.go(4 hunks)internal/runner/runnerpool/runner.go(1 hunks)options/options.go(0 hunks)test/race_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- options/options.go
- test/race_test.go
🧰 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:
internal/component/component.gointernal/runner/common/unit.goconfig/config_helpers.gointernal/discovery/discovery_test.gointernal/discovery/discovery.goconfig/parsing_context.gointernal/runner/runnerpool/runner.gointernal/runner/common/unit_resolver.go
🧬 Code graph analysis (3)
internal/discovery/discovery_test.go (4)
options/options.go (1)
NewTerragruntOptions(382-384)test/helpers/logger/logger.go (1)
CreateLogger(9-14)internal/discovery/discovery.go (1)
NewDiscovery(169-188)internal/component/component.go (1)
Component(28-41)
config/parsing_context.go (2)
options/options.go (1)
TerragruntOptions(99-331)config/config.go (1)
DefaultParserOptions(101-129)
internal/runner/common/unit_resolver.go (2)
internal/runner/common/unit.go (1)
Unit(24-33)options/options.go (1)
TerragruntOptions(99-331)
⏰ 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). (10)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (9)
internal/component/component.go (1)
35-36: LGTM! Clean addition of the Reading field.The new
Reading []stringfield is appropriately scoped as a public field for cross-package access and aligns with the PR's goal of tracking files read during component parsing.internal/discovery/discovery.go (1)
496-500: Verify slice copy semantics.The assignment
c.Reading = *parsingCtx.FilesReadcreates a copy of the slice header, but the underlying array is shared. IfparsingCtx.FilesReadis appended to after this assignment,c.Readingwon't see those additions (expected), but both slices share the same underlying array elements (modifications to existing elements would be visible to both).Is this the intended behavior, or should this be a deep copy to ensure complete isolation?
internal/runner/common/unit.go (1)
28-28: LGTM! Consistent field addition.The
Readingfield onUnitmirrors the same field onComponent, maintaining consistency across the data structures.config/parsing_context.go (2)
46-48: LGTM! Well-documented shared state design.The pointer-to-slice approach is appropriate for sharing read files across parsing context copies. The comment clearly documents this design choice.
65-66: LGTM! Proper initialization.The
FilesReadfield is correctly initialized in the constructor, ensuring it's never nil when methods append to it.Also applies to: 71-71
internal/runner/runnerpool/runner.go (1)
515-515: LGTM! Reading field properly propagated.The
Readingfield is correctly included when creating a filtered component copy, ensuring read-file information flows through the filtering process.internal/runner/common/unit_resolver.go (2)
387-391: LGTM! Safe extraction of read files.Proper nil check before dereferencing
parseCtx.FilesRead. The slice assignment copies the slice header, capturing the state at this point.
414-414: LGTM! Reading field populated during unit creation.The
readFilesslice is correctly assigned to the new unit, enabling downstream filtering based on files read.config/config_helpers.go (1)
723-724: No evidence of concurrent access - review comment appears speculative.Thorough verification found no goroutines spawning with shared
ParsingContext. All appends toFilesReadoccur sequentially during single parse operations. Context copies viaWithDecodeList(),WithParseOption(), etc. intentionally share theFilesReadpointer but are used in serial parsing flows. The slice reads indiscovery.goandunit_resolver.gocopy values rather than operate on the shared pointer.While the lack of synchronization around slice appends is technically a vulnerability if concurrent access were introduced, the current codebase exhibits no race condition. The concern is valid as defensive programming but does not identify an actual bug in the current implementation.
Description
Tracks reads in components, rather than tracking them in
TerragruntOptions.This is important, as it saves us from having shared mutable access to the map in
TerragruntOptions, which should remove lock contention, and allows us to propagate the read files into the filter struct, which is useful as we filter on read files.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Release Notes
Refactor
Tests