chore: experiments tests improvements #4782
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds configurable discovery filters, parser options, and external-dependency handling; updates runner pool builder to pass these options; adjusts artifact path resolution to root-level; switches a runner error to a sentinel; tightens experiment-mode env parsing; and refines CI test-failure parsing and missing-log handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RP as RunnerPool.Builder
participant Disc as Discovery
participant FS as Filesystem
participant Dep as DependencyDiscovery
participant Parser as HCL Parser
RP->>Disc: Configure WithParserOptions / WithIncludeDirs / WithExcludeDirs
RP->>Disc: WithStrictInclude / WithExcludeByDefault / WithIgnoreExternalDependencies
RP->>Disc: Discover(stackDir)
rect rgba(230,240,255,0.5)
Note over Disc: Apply include/exclude filtering (glob) and hidden-dir rules
Disc->>FS: Walk directories
Disc-->>Disc: Filter paths per flags
end
Disc->>Parser: Parse configs with parserOptions
Disc->>Dep: DiscoverDependencies(ignoreExternal, parserOptions)
alt ignoreExternal
Dep-->>Disc: Skip external deps
else include external
Dep->>FS: Resolve deps (internal/external)
end
Disc-->>RP: Discovered units
sequenceDiagram
autonumber
participant RP as RunnerPool
participant Build as NewRunnerPoolStack
participant Disc as Discovery
RP->>Build: Build stack
Build->>Disc: Discover()
alt no units found
Build-->>RP: return common.ErrNoUnitsFound
else units found
Build-->>RP: return RunnerPoolStack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
yhakbar
approved these changes
Sep 3, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/runner/common/unit.go (1)
93-121: Path traversal risk: unit.Path outside RootWorkingDir can escape outputFolderIf unit.Path isn’t under RootWorkingDir (Rel error, different drive on Windows, or resulting rel starts with ..), joining with outputFolder can write artifacts outside the intended root. Also, falling back to an absolute unit.Path causes outputFolder to be ignored. Harden the path derivation.
- path, err := filepath.Rel(opts.RootWorkingDir, unit.Path) - if err != nil { - l.Warnf("Failed to get relative path for %s: %v", unit.Path, err) - path = unit.Path - } - - dir := filepath.Join(outputFolder, path) - - if !filepath.IsAbs(dir) { - // Resolve relative output folder against root working directory, not the unit working directory, - // so that artifacts for all units are stored under a single root-level out dir structure. - base := opts.RootWorkingDir - if !filepath.IsAbs(base) { - // In case RootWorkingDir is somehow relative, resolve it first. - if absBase, err := filepath.Abs(base); err == nil { - base = absBase - } else { - l.Warnf("Failed to get absolute path for root working dir %s: %v", base, err) - } - } - - dir = filepath.Join(base, dir) - - if absDir, err := filepath.Abs(dir); err == nil { - dir = absDir - } else { - l.Warnf("Failed to get absolute path for %s: %v", dir, err) - } - } + // Resolve base (prefer RootWorkingDir, fallback to WorkingDir) + base := opts.RootWorkingDir + if base == "" { + base = opts.WorkingDir + } + if !filepath.IsAbs(base) { + if absBase, err := filepath.Abs(base); err == nil { + base = absBase + } else { + l.Warnf("Failed to get absolute path for root working dir %s: %v", base, err) + } + } + + // Compute a safe relative path; if unit.Path is outside base, sanitize the name + relPath, relErr := filepath.Rel(base, unit.Path) + unsafeRel := relErr != nil || relPath == ".." || + strings.HasPrefix(relPath, ".."+string(filepath.Separator)) || + filepath.IsAbs(relPath) + if unsafeRel { + l.Warnf("Unit path %s is outside root %s; using a sanitized directory name under the output folder", unit.Path, base) + relPath = strings.TrimLeft(filepath.ToSlash(unit.Path), "/") + relPath = strings.NewReplacer(":", "_").Replace(relPath) // windows drive letter, etc. + relPath = strings.ReplaceAll(relPath, "/", "_") // flatten to a single safe path element + } + + dir := filepath.Join(outputFolder, relPath) + if !filepath.IsAbs(dir) { + dir = filepath.Join(base, dir) + } + if absDir, err := filepath.Abs(dir); err == nil { + dir = absDir + } else { + l.Warnf("Failed to get absolute path for %s: %v", dir, err) + }internal/discovery/discovery.go (2)
368-374: Compile error and logic bug in hidden-dir checkstrings.SplitSeq doesn’t exist; also range returns indices unless you capture the value. This will not compile and/or behave incorrectly.
Apply:
- parts := strings.SplitSeq(path, string(os.PathSeparator)) - for part := range parts { + parts := strings.Split(path, string(os.PathSeparator)) + for _, part := range parts { hiddenPath = filepath.Join(hiddenPath, part)
923-936: Invalid loop syntax — won’t compilefor range maxCycleChecks is invalid on an int. Use a counted loop.
- for range maxCycleChecks { + for i := 0; i < maxCycleChecks; i++ { if cfg, err = c.CycleCheck(); err == nil { break }
🧹 Nitpick comments (8)
test/helpers/test_helpers.go (1)
62-66: Docs drift: update comments to match stricter experiment-mode semanticsThe code now enables experiment mode only when TG_EXPERIMENT_MODE equals "true" (case-insensitive, trimmed), but the comments still say “is set.” Align the comments; optional micro-cleanup in the return line.
-// IsRunnerPoolExperimentEnabled returns true if either TG_EXPERIMENT_MODE is set or TG_EXPERIMENT is set to "runner-pool". +// IsRunnerPoolExperimentEnabled returns true if experiment mode is enabled (TG_EXPERIMENT_MODE="true") or TG_EXPERIMENT is "runner-pool". @@ -// IsExperimentMode returns true if the TG_EXPERIMENT_MODE environment variable is set. +// IsExperimentMode returns true only if TG_EXPERIMENT_MODE equals "true" (case-insensitive, trimmed). @@ - val := strings.TrimSpace(os.Getenv("TG_EXPERIMENT_MODE")) - - return strings.EqualFold(val, "true") + return strings.EqualFold(strings.TrimSpace(os.Getenv("TG_EXPERIMENT_MODE")), "true")Also applies to: 68-75
.github/workflows/integration-test.yml (1)
268-275: Make failure parsing robust and consistent with the commentAdd pipefail to honor the “safe with -e -o pipefail” note, and print only the test names (without durations).
- if [[ -f test_output.log ]]; then - # Count only test failure lines in a way that is safe with -e -o pipefail + set -o pipefail + if [[ -f test_output.log ]]; then + # Count only test failure lines in a way that is safe with -o pipefail failed_count=$(grep -E -c '^--- FAIL:' test_output.log || echo 0) @@ - grep -E '^--- FAIL:' test_output.log | sed 's/.*FAIL:[[:space:]]*//' + grep -E '^--- FAIL:' test_output.log | sed -E 's/^--- FAIL:[[:space:]]*([^[:space:]]+).*/\1/'internal/runner/runnerpool/runner.go (1)
41-41: Preserve stack context while returning the sentinelReturn the sentinel wrapped with a stack so callers can both errors.Is(.., common.ErrNoUnitsFound) and get a useful trace.
- return nil, common.ErrNoUnitsFound + return nil, errors.WithStack(common.ErrNoUnitsFound)Add import:
import "github.com/gruntwork-io/terragrunt/internal/errors"Please confirm any call sites switched to errors.Is/As rather than string matching.
internal/discovery/discovery_test.go (2)
644-649: Windows glob portability for includeDirsOn Windows, some globbers expect forward slashes. To avoid OS-specific surprises, normalize the pattern.
- d := discovery.NewDiscovery(tmpDir).WithIncludeDirs([]string{filepath.Join(tmpDir, ".hidden", "**")}) + pat := filepath.ToSlash(filepath.Join(tmpDir, ".hidden", "**")) + d := discovery.NewDiscovery(tmpDir).WithIncludeDirs([]string{pat})
670-717: Extra assertion to lock intentOptionally assert that the remaining deps in appCfg are all internal (External == false) to future-proof behavior.
for _, dep := range appCfg.Dependencies { require.False(t, dep.External, "expected only internal dependencies") }internal/runner/runnerpool/builder.go (1)
49-52: Skip external discovery when it will be ignoredMinor perf: if IgnoreExternalDependencies is true, avoid enabling external discovery upfront.
You can guard the earlier call:
// only call when not ignoring externals if !terragruntOptions.IgnoreExternalDependencies { d = d.WithDiscoverExternalDependencies() }internal/discovery/discovery.go (2)
581-586: Fix log grammar and format verbUse “were” and %v to avoid fmt %w misuse in non-Errorf formatters.
- l.Warnf("Parsing errors where encountered while discovering dependencies. They were suppressed, and can be found in the debug logs.") - - l.Debugf("Errors: %w", err) + l.Warnf("Parsing errors were encountered while discovering dependencies. They were suppressed; see debug logs for details.") + l.Debugf("Errors: %v", err)
520-527: Directory-level pruning to reduce I/OConsider applying exclude/include checks on directories and returning filepath.SkipDir to prune traversal early instead of evaluating only at file level. This will cut FS walk cost on large repos.
Also applies to: 415-423
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.github/workflows/integration-test.yml(1 hunks)internal/discovery/discovery.go(15 hunks)internal/discovery/discovery_test.go(1 hunks)internal/runner/common/unit.go(2 hunks)internal/runner/runnerpool/builder.go(1 hunks)internal/runner/runnerpool/runner.go(1 hunks)test/helpers/test_helpers.go(1 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:
internal/runner/common/unit.gotest/helpers/test_helpers.gointernal/runner/runnerpool/runner.gointernal/discovery/discovery_test.gointernal/runner/runnerpool/builder.gointernal/discovery/discovery.go
🧠 Learnings (3)
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
PR: gruntwork-io/terragrunt#3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
internal/runner/runnerpool/runner.gointernal/discovery/discovery.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
PR: gruntwork-io/terragrunt#4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
internal/runner/runnerpool/runner.gointernal/discovery/discovery.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
PR: gruntwork-io/terragrunt#4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
internal/discovery/discovery.go
🧬 Code graph analysis (2)
internal/runner/runnerpool/runner.go (1)
internal/runner/common/errors.go (1)
ErrNoUnitsFound(46-46)
internal/runner/runnerpool/builder.go (1)
config/config.go (1)
DefaultParserOptions(101-129)
🔇 Additional comments (5)
internal/discovery/discovery_test.go (1)
588-631: Nice coverage for include/exclude behaviorGood, focused assertions and use of ElementsMatch keep tests order-agnostic. 👍
internal/runner/runnerpool/builder.go (1)
27-30: Passing parser options from TerragruntOptions looks correctGood propagation via WithParserOptions; aligns with config.DefaultParserOptions contract.
internal/discovery/discovery.go (3)
142-145: Sane defaults for include patternsDefaulting to stack dir (and its subtree) is a good baseline for discovery.
424-461: Include filter runs before hidden-dir allowance; risk of excluding stack contentsWith StrictInclude/ExcludeByDefault, include-pattern enforcement happens before the hidden-directory allowance, so stack contents can be dropped if callers don’t include stack patterns. The proposed WithIncludeDirs change mitigates this; otherwise, consider checking stack allowance before returning on “not included”.
Also applies to: 468-493
316-320: Parser options propagation: LGTMCleanly applies custom parser options into the parsing context.
Comment on lines
+234
to
+238
| // WithIncludeDirs sets include directory glob patterns used for filtering during discovery. | ||
| func (d *Discovery) WithIncludeDirs(dirs []string) *Discovery { | ||
| d.includeDirs = dirs | ||
| return d | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep stack patterns even when callers set IncludeDirs
Avoid accidentally dropping .terragrunt-stack when StrictInclude/ExcludeByDefault is enabled. Append user patterns to the defaults instead of replacing them.
Apply:
func (d *Discovery) WithIncludeDirs(dirs []string) *Discovery {
- d.includeDirs = dirs
+ d.includeDirs = append([]string{
+ config.StackDir,
+ filepath.Join(config.StackDir, "**"),
+ }, dirs...)
return d
}📝 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.
Suggested change
| // WithIncludeDirs sets include directory glob patterns used for filtering during discovery. | |
| func (d *Discovery) WithIncludeDirs(dirs []string) *Discovery { | |
| d.includeDirs = dirs | |
| return d | |
| } | |
| // WithIncludeDirs sets include directory glob patterns used for filtering during discovery. | |
| func (d *Discovery) WithIncludeDirs(dirs []string) *Discovery { | |
| d.includeDirs = append([]string{ | |
| config.StackDir, | |
| filepath.Join(config.StackDir, "**"), | |
| }, dirs...) | |
| return d | |
| } |
🤖 Prompt for AI Agents
In internal/discovery/discovery.go around lines 234 to 238, the WithIncludeDirs
method currently replaces d.includeDirs with the caller-supplied dirs which can
drop default stack patterns like ".terragrunt-stack" when
StrictInclude/ExcludeByDefault is enabled; change the method to append the
incoming dirs to the existing default include patterns (preserving defaults and
avoiding duplicates if desired) so user-provided patterns are added rather than
replacing the defaults, then return d.
denis256
added a commit
that referenced
this pull request
Sep 5, 2025
* Group tests update * Added checking for group tests * chore: lint fixes * runner-pool handling of TestOutputModuleGroups * Updated plan path save file * Improved FAIL errors * IsExperimentMode() simplification * Discovery include flags * Added passing of discovery include/exclude directories * Fixed discovery flags passing * Improved parsing of tests
yhakbar
added a commit
that referenced
this pull request
Sep 5, 2025
* Backend bootstrap only by explicit flag * Cleanup * lint issues * Updated error explainer error * Documentation update * Improved errors detection * GCP bootstrap flags * Tests update * Bootstrap flags update * Updated --backend-bootstrap to OIDC * GCP cleanup * Backend update * GCP tests cleanup * AWS tests update * AWS docs update * Docs bootstrap fix * Tests simplification * docs: Add Terralith to Terragrunt guide (#4709) * feat: Adding fixture for Terralith to Terragrunt guide * feat: Adding Terralith to Terragrunt walkthrough * fix: Use Asides where possible * fix: Moving import up to avoid breaking list formatting * fi:x Removing incorrect tip * fix: Fixing asset links * fix: The `gitignore` syntax highlight doesn't exist * fix: Moving fixtures to the `docs-starlight` directory * fix: Adjusting path * fix: Removing `package-lock.json` entry in `.vercelignore` * Revert "fix: Adjusting path" This reverts commit 62e6d2d. * fix: Removing duplicate fixtures * Update docs-starlight/src/content/docs/02-guides/01-terralith-to-terragrunt/05-step-2-refactoring.mdx Co-authored-by: Zach Goldberg <zach@gruntwork.io> * fix: Fixing link for fixture code --------- Co-authored-by: Zach Goldberg <zach@gruntwork.io> * Updated form link (#4771) * Polish to contact form (#4769) * Pricing Page Launch (#4772) * New supercharge module * Updates to links etc * Update copy * Update docs-starlight/src/components/dv-PetAdvertise.astro Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update docs-starlight/src/components/dv-PetAdvertise.astro Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update docs-starlight/src/components/dv-PetAdvertise.astro Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Polish (#4773) * fix: ensure custom API endpoints are set correctly (#4756) * fix: ensure custom API endpoints are set correctly * refactor: ensure consistent use of awshelper.CreateS3Client * chore: fix runner-pool experiment tests (#4770) * Group tests update * Added checking for group tests * chore: lint fixes * runner-pool handling of TestOutputModuleGroups * Bypass partytown (#4783) * Bypass partytown * GTM in header * fix: Only override ref when not set (#4781) * A collection of website polishing (#4784) * Tighten up sidebar. * Standardize CSS ordering between dev and prod. Previously, the site would render one way in prod and another in dev! The issue was that vite was tree-shaking CSS and wound up re-ordering things in a way it thought was permissible, whereas in dev, without that optimization, the CSS was actuallys sequenced differently. This led to a noticable huge margin in dev, but not in prod. This commit asserts the official ordering the CSS layers, and then adds a fix for the left-margin issue. This should now standard dev and prod. * Reduce code font-size from 16px to 14px. * Improve main paragraph text rendering. Use more readable line height and paragraph separation. * Improve spacing after file tree. * Fix sidebar inconsistencies on 3rd level of nav. * feat: Generate stacks in topological order (#4786) * feat: Finalizing topological generation of stacks * feat: Adding tests for topological stack generation * fix: Address race condition in warning suppression * feat: Set name of test to `TestStackGenerationWithNestedTopologyWithRacing` to ensure it's caught by race test * feat: Adding extra generate at the end for confirmation * fix: Updating expected log messages in tests * fix: Fixing AWS Account ID w/ Provider CMD (#4779) * test: Attempting to reproduce issue with OIDC * fix: Fixing `get_aws_account_id()` when using AuthProviderCmd * fix: Addressing lint findings * fix: Adding fixture for backend with OIDC * fix: Adding integration test for OIDC with backend * fix: Consolidating logic for AWS credential acquisition * fix: Addressing lint findings * test: Removing cleanup to fix this * fix: Fixing delete bucket cleanup * fix: Fixing role assumption when env creds aren't fetched from auth provider * fix: Removing unnecessary debug * fix: Skipping failing test for now * Fixed failing OIDC tests * Tests cleanup * chore: aws helper complexity reduction * Updated cleanup order * enabled build tags --------- Co-authored-by: Denis O <denis.o@linux.com> * chore: experiments tests improvements (#4782) * Group tests update * Added checking for group tests * chore: lint fixes * runner-pool handling of TestOutputModuleGroups * Updated plan path save file * Improved FAIL errors * IsExperimentMode() simplification * Discovery include flags * Added passing of discovery include/exclude directories * Fixed discovery flags passing * Improved parsing of tests * docs: Updating migration docs (#4711) * Added --non-interactive * Market strict control as completed * fix: Fixing failing OIDC test (#4791) --------- Co-authored-by: Yousif Akbar <11247449+yhakbar@users.noreply.github.com> Co-authored-by: Zach Goldberg <zach@gruntwork.io> Co-authored-by: Karl Carstensen <karl.carstensen@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: AJ (.jASM) <aj@48k.io> Co-authored-by: Rodin Velichkov <148242776+rvelichkov@users.noreply.github.com> Co-authored-by: Josh Padnick <josh@gruntwork.io>
This was referenced Sep 9, 2025
This was referenced Sep 17, 2025
Merged
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
RFC: #3629
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores