docs: Documenting filter-flag experiment
#4973
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
📝 WalkthroughWalkthroughAdds comprehensive documentation for an experimental Changes
Sequence Diagram(s)(Skipped — changes are documentation and tests; no runtime control-flow changes to diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: many documentation additions (low complexity) plus substantial new integration tests and fixture generators (higher complexity) and refactors to existing tests; heterogeneous changes require careful reading of tests and examples. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
docs/documenting-filter-experiment
branch
from
b3cb9d4 to
d686ba9
Compare
yhakbar
force-pushed
the
docs/documenting-filter-experiment
branch
from
d686ba9 to
392c712
Compare
yhakbar
force-pushed
the
docs/documenting-filter-experiment
branch
from
392c712 to
565a153
Compare
|
|
||
| ```bash | ||
| # Relative paths with globs | ||
| terragrunt find --filter './envs/prod/**' |
There was a problem hiding this comment.
Missing --experiment filter-flag flag
|
Noticed that in experiment mode are failing multiple FilterFlat tests |
yhakbar
force-pushed
the
docs/documenting-filter-experiment
branch
from
e6348df to
dfcd46b
Compare
|
Sorry about that. This passes now: go test -v -run '^TestFilterFlagEdgeCases$|^TestFilterFlagMultipleFilters$|^TestFilterFlagWithDAG$|^TestFilterFlagWithList$|^TestFilterFlagWithFindJSON$|^TestFilterFlagWithFind$|^TestFilterDocumentationExamples$' ./...I relied too much on CI, thinking that the experimental tests would run. The fixes were all in the tests. |
yhakbar
force-pushed
the
docs/documenting-filter-experiment
branch
from
e869abd to
a2da0b9
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: 5
♻️ Duplicate comments (1)
docs-starlight/src/data/flags/filter.mdx (1)
13-16: Add one example without--experimentand note--experiment-modeShow both ways to enable experiments for quick copy/paste.
terragrunt find --experiment filter-flag --filter 'app*' -terragrunt list --experiment filter-flag --filter './prod/** | type=unit' +terragrunt list --experiment filter-flag --filter './prod/** | type=unit' +# Or enable all experiments: +TG_EXPERIMENT_MODE=true terragrunt list --filter './prod/** | type=unit'
🧹 Nitpick comments (17)
docs-starlight/src/content/docs/04-reference/04-experiments.md (2)
56-64: Consistent CLI example styleFor consistency with other pages, prefer placing flags after the subcommand, e.g.,
terragrunt plan --experiment symlinksinstead ofterragrunt --experiment symlinks plan. Same for the multiple-experiments example.
169-189: Deprecations: add cross-linksConsider linking each legacy queue flag to its run command reference to help users map old→new quickly.
docs-starlight/src/data/commands/find.mdx (3)
287-297: Single source of truth on gatingTip says examples will omit
--experiment filter-flag, but advanced examples below add it back. Drop it there or remove the sentence here for consistency.-Examples below will omit the `--experiment filter-flag` flag for brevity. +Examples below omit `--experiment filter-flag` for brevity (you can also enable all experiments with `--experiment-mode` or `TG_EXPERIMENT_MODE=true`).
321-330: Remove redundant--experimentfrom advanced examples (per tip above)Keep examples either all-with or all-without
--experiment filter-flag. Since the tip says to omit, remove it here.-terragrunt find --experiment filter-flag --filter '!./test/**' +terragrunt find --filter '!./test/**' -terragrunt find --experiment filter-flag --filter 'app1' --filter 'app2' +terragrunt find --filter 'app1' --filter 'app2' -terragrunt find --experiment filter-flag --filter './dev/** | type=unit | !name=unit1' +terragrunt find --filter './dev/** | type=unit | !name=unit1'
299-316: Add note about quoting to avoid shell globbingUsers on POSIX shells need quotes around patterns like
*/**. Add a short caution note here.The `find` command supports the `--filter` flag to target specific configurations using a flexible query language. This is particularly useful for discovering configurations that match specific criteria before running operations on them. + +<Aside type="caution" title="Quote your filter expressions"> +On POSIX shells, unquoted `*`/`**` patterns are expanded by the shell. Wrap filters in quotes (e.g., `--filter './prod/**'`) to ensure Terragrunt receives the pattern as-is. +</Aside>docs-starlight/src/data/commands/list.mdx (2)
232-241: Consider aligning gating style with find docsIf find omits
--experiment filter-flagin examples, consider doing the same here (or add a one-liner explaining why they’re included here). Consistency reduces confusion.
242-259: Add quoting cautionAdd a brief note to prevent shell globbing surprises with
*/**.The `list` command supports the `--filter` flag to target specific configurations using a flexible query language. This is particularly useful for listing configurations that match specific criteria before running operations on them. + +<Aside type="caution" title="Quote your filter expressions"> +On POSIX shells, unquoted `*`/`**` patterns may be expanded by the shell. Wrap filters in quotes (e.g., `--filter './prod/**'`). +</Aside>docs-starlight/src/content/docs/03-features/18-filter.mdx (2)
12-24: Briefly mention--experiment-modealternativeSince many examples omit
--experiment filter-flag, add that enabling--experiment-modeorTG_EXPERIMENT_MODE=truemakes examples work as written.
68-111: Add quoting guidance once, earlyPlace a short caution about quoting filters (to avoid shell globbing) near the start of “Filter Types” so it applies to all examples.
## Filter Types @@ There are several different types of filters, and particular ways in which they can be combined to achieve different results. You can learn more about that below. + +<Aside type="caution" title="Quote your filters"> +On POSIX shells, `*`/`**` can expand before Terragrunt sees them. Use quotes: `--filter './envs/prod/**'`. +</Aside>docs-starlight/src/data/flags/filter.mdx (1)
31-40: Quoting cautionAdd a short note that unquoted globs can be expanded by the shell.
### Path-Based Filtering Match configurations by their file system path: + +<Aside type="caution" title="Quote your paths"> +Use quotes around globs (`--filter './envs/prod/**'`) to avoid shell expansion. +</Aside>docs-starlight/src/content/docs/03-features/05-run-queue.mdx (1)
96-101: Cross-link the flag referenceAdd a link to the
--filterflag reference alongside the feature page link for quicker navigation.-The experimental [`--filter` flag](/docs/features/filter) provides a unified approach... +The experimental [`--filter` flag](/docs/features/filter) (see the [flag reference](/docs/reference/cli/flags/filter)) provides a unified approach...test/integration_docs_test.go (1)
649-660: Minimal stacks should not defineterraformblockFor stacks, the
terragrunt.stack.hcltypically won’t need aterraform { source = "." }block. If intentional for the fixture, add a brief comment to avoid confusion. Otherwise, remove it.- // Create minimal terragrunt.stack.hcl file - require.NoError(t, os.WriteFile(filepath.Join(dir, "terragrunt.stack.hcl"), []byte("terraform {\n source = \".\"\n}"), 0644)) + // Create minimal terragrunt.stack.hcl file (no terraform block needed for stacks) + require.NoError(t, os.WriteFile(filepath.Join(dir, "terragrunt.stack.hcl"), []byte("# empty stack fixture\n"), 0644))If stack fixtures rely on the
terraformblock, keep it and ignore this suggestion.test/integration_filter_test.go (5)
284-288: Consider checking stderr in error cases for consistency.Unlike other test functions in this file (e.g., lines 113-116, 183-186), this error handling path doesn't verify that stderr contains an error message. Adding this check would make error validation more robust.
if tc.expectError { require.Error(t, err, "Expected error for filter query: %s", tc.filterQuery) + assert.NotEmpty(t, stderr, "Expected error message in stderr") return }
306-349: Inconsistent with absolute path pattern used in other tests.Unlike
TestFilterFlagWithFind,TestFilterFlagWithFindJSON,TestFilterFlagWithList, andTestFilterFlagWithDAG, this test doesn't compute absolute paths upfront. For consistency and to align with the comment at line 203 ("The CLI constructor ensures that the working directory is always absolute"), consider refactoring to compute the absolute path before the test cases loop.+ workingDir, err := filepath.Abs(testFixtureFilterList) + require.NoError(t, err) + testCases := []struct { name string - workingDir string filterQuery string expectedOutput string expectError bool }{ { name: "filter by name - exact match long format", - workingDir: testFixtureFilterList, filterQuery: "a-unit", expectedOutput: "Type Path\nunit a-unit\n", expectError: false, }, // ... (update other test cases similarly) } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - helpers.CleanupTerraformFolder(t, tc.workingDir) + helpers.CleanupTerraformFolder(t, workingDir) - cmd := "terragrunt list --no-color --working-dir " + tc.workingDir + " --long --filter " + tc.filterQuery + cmd := "terragrunt list --no-color --working-dir " + workingDir + " --long --filter " + tc.filterQuery
372-415: Inconsistent with absolute path pattern used in other tests.Same issue as
TestFilterFlagWithListLong. For consistency, compute the absolute path upfront rather than embedding relative paths in test case structs.+ workingDir, err := filepath.Abs(testFixtureFilterList) + require.NoError(t, err) + testCases := []struct { name string - workingDir string filterQuery string expectedOutput string expectError bool }{ { name: "filter by name - exact match tree format", - workingDir: testFixtureFilterList, filterQuery: "a-unit", expectedOutput: ".\n╰── a-unit\n", expectError: false, }, // ... (update other test cases similarly) } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - helpers.CleanupTerraformFolder(t, tc.workingDir) + helpers.CleanupTerraformFolder(t, workingDir) - cmd := "terragrunt list --no-color --working-dir " + tc.workingDir + " --tree --filter " + tc.filterQuery + cmd := "terragrunt list --no-color --working-dir " + workingDir + " --tree --filter " + tc.filterQuery
555-555: Consider capturing stderr for better error visibility.Ignoring stderr with
_could hide unexpected error messages. For consistency with other tests in this file and better debugging, capture and verify stderr is empty when not expecting errors.- stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, cmd) + stdout, stderr, err := helpers.RunTerragruntCommandWithOutput(t, cmd) if tc.expectError { require.Error(t, err, "Expected error for filter queries: %v", tc.filterQueries) } else { require.NoError(t, err, "Unexpected error for filter queries: %v", tc.filterQueries) + assert.Empty(t, stderr, "Unexpected error message in stderr") assert.Equal(t, tc.expectedOutput, stdout, "Output mismatch for filter queries: %v", tc.filterQueries) }
611-611: Consider capturing stderr for better error visibility.Same issue as line 555. Capturing stderr would help catch unexpected error messages during test execution.
- stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, cmd) + stdout, stderr, err := helpers.RunTerragruntCommandWithOutput(t, cmd) if tc.expectError { require.Error(t, err, "Expected error for filter query: %s", tc.filterQuery) } else { require.NoError(t, err, "Unexpected error for filter query: %s", tc.filterQuery) + assert.Empty(t, stderr, "Unexpected error message in stderr") assert.Equal(t, tc.expectedOutput, stdout, "Output mismatch for filter query: %s", tc.filterQuery) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs-starlight/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
docs-starlight/src/content/docs/03-features/05-run-queue.mdx(1 hunks)docs-starlight/src/content/docs/03-features/18-filter.mdx(1 hunks)docs-starlight/src/content/docs/04-reference/04-experiments.md(2 hunks)docs-starlight/src/data/commands/find.mdx(2 hunks)docs-starlight/src/data/commands/list.mdx(2 hunks)docs-starlight/src/data/flags/filter.mdx(1 hunks)test/integration_docs_test.go(2 hunks)test/integration_filter_test.go(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs-starlight/**/*.md*
⚙️ CodeRabbit configuration file
Review the documentation for clarity, grammar, and spelling. Make sure that the documentation is easy to understand and follow. There is currently a migration underway from the Jekyll based documentation in
docsto the Starlight + Astro based documentation indocs-starlight. Make sure that thedocs-starlightdocumentation is accurate and up-to-date with thedocsdocumentation, and that any difference between them results in an improvement in thedocs-starlightdocumentation.
Files:
docs-starlight/src/content/docs/03-features/05-run-queue.mdxdocs-starlight/src/content/docs/04-reference/04-experiments.mddocs-starlight/src/data/commands/find.mdxdocs-starlight/src/content/docs/03-features/18-filter.mdxdocs-starlight/src/data/flags/filter.mdxdocs-starlight/src/data/commands/list.mdx
**/*.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:
test/integration_filter_test.gotest/integration_docs_test.go
🧬 Code graph analysis (2)
test/integration_filter_test.go (3)
cli/commands/find/find.go (1)
Run(23-132)cli/commands/list/list.go (1)
Run(26-135)test/helpers/package.go (2)
CleanupTerraformFolder(878-885)RunTerragruntCommandWithOutput(993-997)
test/integration_docs_test.go (2)
test/helpers/test_helpers.go (1)
IsExperimentMode(65-71)test/helpers/package.go (1)
RunTerragruntCommandWithOutput(993-997)
⏰ 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: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
- GitHub Check: integration_tests / Test (Provider Cache Server with Tofu)
🔇 Additional comments (8)
test/integration_filter_test.go (8)
4-5: LGTM!The addition of
filepathandstringsimports is appropriate for the absolute path computation and output parsing functionality added in the tests.
27-28: Good practice computing absolute paths upfront.Computing the absolute working directory at the test suite level improves test reproducibility and aligns with how the CLI constructor handles paths.
133-134: LGTM!Consistent use of absolute path computation as in
TestFilterFlagWithFind.
203-205: Excellent explanatory comment.The comment provides valuable context explaining why absolute paths are required, making the test setup easier to understand.
292-293: LGTM!Using
strings.FieldswithElementsMatchprovides order-independent comparison appropriate for unordered module names.
438-439: LGTM!Consistent use of absolute path computation.
514-515: LGTM!Consistent use of absolute path computation.
575-576: LGTM!Consistent use of absolute path computation.
| // Run the find command with the filter | ||
| command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", tc.filterQuery, tc.extraFlags, workingDir) | ||
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | ||
|
|
||
| if err != nil { | ||
| t.Logf("Command failed: %s", command) | ||
| t.Logf("Error: %v", err) | ||
| t.Logf("Output: %s", stdout) | ||
| } | ||
|
|
||
| require.NoError(t, err, "Command should succeed") | ||
| assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result") | ||
| }) |
There was a problem hiding this comment.
Quote filter queries to avoid shell globbing by the shell
Unquoted */** may expand before Terragrunt runs, making tests flaky or platform-dependent. Quote the filter argument safely.
- command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", tc.filterQuery, tc.extraFlags, workingDir)
+ command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", shQuote(tc.filterQuery), tc.extraFlags, workingDir)Add a small helper:
@@
func TestFilterDocumentationExamplesWithUnion(t *testing.T) {
@@
}
+// Minimal POSIX-like shell quoting to prevent glob expansion in commands we execute via the shell.
+func shQuote(s string) string {
+ return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'"
+}📝 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.
| // Run the find command with the filter | |
| command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", tc.filterQuery, tc.extraFlags, workingDir) | |
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | |
| if err != nil { | |
| t.Logf("Command failed: %s", command) | |
| t.Logf("Error: %v", err) | |
| t.Logf("Output: %s", stdout) | |
| } | |
| require.NoError(t, err, "Command should succeed") | |
| assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result") | |
| }) | |
| // Run the find command with the filter | |
| command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", shQuote(tc.filterQuery), tc.extraFlags, workingDir) | |
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | |
| if err != nil { | |
| t.Logf("Command failed: %s", command) | |
| t.Logf("Error: %v", err) | |
| t.Logf("Output: %s", stdout) | |
| } | |
| require.NoError(t, err, "Command should succeed") | |
| assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result") | |
| }) |
| // Run the find command with the filter | |
| command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", tc.filterQuery, tc.extraFlags, workingDir) | |
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | |
| if err != nil { | |
| t.Logf("Command failed: %s", command) | |
| t.Logf("Error: %v", err) | |
| t.Logf("Output: %s", stdout) | |
| } | |
| require.NoError(t, err, "Command should succeed") | |
| assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result") | |
| }) | |
| // Minimal POSIX-like shell quoting to prevent glob expansion in commands we execute via the shell. | |
| func shQuote(s string) string { | |
| return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" | |
| } |
🤖 Prompt for AI Agents
In test/integration_docs_test.go around lines 447 to 459, the test constructs
the shell command with an unquoted filter argument which allows shell globbing
of * and ** before Terragrunt runs; update the command construction to quote or
escape the filter argument so the shell passes the literal pattern (e.g., wrap
tc.filterQuery in single quotes or use a helper to escape/quote it), and add a
small helper function used by tests that safely quotes/escapes shell arguments
(use it for tc.filterQuery when building the command), then use that helper
wherever filter queries are passed to shell commands to avoid platform-dependent
globbing.
| // Run the find command with the filter | ||
| var filterArgs []string | ||
| for _, query := range tc.filterQueries { | ||
| filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", query)) | ||
| } | ||
| command := fmt.Sprintf("terragrunt find %s --working-dir %s", strings.Join(filterArgs, " "), workingDir) | ||
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | ||
| require.NoError(t, err, "Command should succeed") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Also quote multi-flag union queries
Apply the same quoting for each --filter occurrence in the union test.
- for _, query := range tc.filterQueries {
- filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", query))
- }
+ for _, query := range tc.filterQueries {
+ filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", shQuote(query)))
+ }📝 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.
| // Run the find command with the filter | |
| var filterArgs []string | |
| for _, query := range tc.filterQueries { | |
| filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", query)) | |
| } | |
| command := fmt.Sprintf("terragrunt find %s --working-dir %s", strings.Join(filterArgs, " "), workingDir) | |
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | |
| require.NoError(t, err, "Command should succeed") | |
| // Run the find command with the filter | |
| var filterArgs []string | |
| for _, query := range tc.filterQueries { | |
| filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", shQuote(query))) | |
| } | |
| command := fmt.Sprintf("terragrunt find %s --working-dir %s", strings.Join(filterArgs, " "), workingDir) | |
| stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) | |
| require.NoError(t, err, "Command should succeed") |
🤖 Prompt for AI Agents
In test/integration_docs_test.go around lines 516 to 523, the filter flags are
built without quoting each multi-flag union query which breaks union tests;
update the filterArgs construction to quote each --filter value (e.g. format
each entry as --filter '<query>' or --filter "<query>") so every occurrence of
--filter wraps the query in quotes before joining and executing the command.
| name: "filter with intersection - path and negation", | ||
| workingDir: testFixtureFilterBasic, | ||
| filterQuery: "./* | !unit", | ||
| filterQuery: "./*|!unit", // Our testing arg parsing is busted. Don't put whitespace between these. |
There was a problem hiding this comment.
Track the test argument parsing limitation.
The comment indicates a limitation in the test infrastructure that requires avoiding whitespace in filter arguments. This workaround suggests underlying technical debt in how test commands parse arguments.
Would you like me to open an issue to track fixing the test argument parsing to properly handle whitespace?
🤖 Prompt for AI Agents
In test/integration_filter_test.go around line 86, the inline comment notes a
test-arg parsing limitation requiring no whitespace in filterQuery
("./*|!unit"); open a tracking issue to fix the test argument parsing so it
accepts and correctly handles whitespace. Create an issue in the repo with a
clear title, reproduction steps (example arguments that fail and the current
behavior), the expected behavior, pointers to the test file and parsing code,
priority/labels (bug/test-technical-debt), and an initial estimate or owner to
pick it up.
| name: "filter with empty intersection", | ||
| workingDir: testFixtureFilterBasic, | ||
| filterQuery: "unit | nonexistent", | ||
| filterQuery: "unit|nonexistent", // Our testing arg parsing is busted. Don't put whitespace between these. |
There was a problem hiding this comment.
Same test argument parsing limitation as line 86.
This is the same technical debt mentioned earlier about test infrastructure argument parsing.
🤖 Prompt for AI Agents
In test/integration_filter_test.go around line 598, the test uses filterQuery:
"unit|nonexistent" and the test harness cannot accept whitespace around the '|'
delimiter; update the test argument parsing so it tolerates spaces by splitting
filter strings on a pattern that ignores surrounding whitespace (e.g., split on
a regex like `\s*\|\s*` or trim tokens after splitting) so future tests can use
"unit | nonexistent" without breaking, then run tests and adjust any affected
test cases accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
test/integration_docs_test.go (3)
453-465: Quote filter and workingDir; normalize newlines to avoid shell globbing/pipes and OS flakinessUnquoted patterns may be expanded by the shell; '|' may be treated as a pipe; '!' as history expansion; braces may expand. Quote args and normalize newlines.
- command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", tc.filterQuery, tc.extraFlags, workingDir) + command := fmt.Sprintf("terragrunt find --filter %s %s --working-dir %s", shQuote(tc.filterQuery), tc.extraFlags, shQuote(workingDir)) stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) @@ - require.NoError(t, err, "Command should succeed") - assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result") + require.NoError(t, err, "Command should succeed") + // Normalize for Windows CRLF + stdout = strings.ReplaceAll(stdout, "\r\n", "\n") + assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result")
522-531: Also quote each --filter in union tests; quote workingDir and normalize newlinesPrevents shell expansion of '*', '!', and braces; improves portability.
- var filterArgs []string - for _, query := range tc.filterQueries { - filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", query)) - } - command := fmt.Sprintf("terragrunt find %s --working-dir %s", strings.Join(filterArgs, " "), workingDir) - stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) - require.NoError(t, err, "Command should succeed") - - assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result") + var filterArgs []string + for _, query := range tc.filterQueries { + filterArgs = append(filterArgs, fmt.Sprintf("--filter %s", shQuote(query))) + } + command := fmt.Sprintf("terragrunt find %s --working-dir %s", strings.Join(filterArgs, " "), shQuote(workingDir)) + stdout, _, err := helpers.RunTerragruntCommandWithOutput(t, command) + require.NoError(t, err, "Command should succeed") + stdout = strings.ReplaceAll(stdout, "\r\n", "\n") + assert.Equal(t, tc.expectedOutput, stdout, "Output should match expected result")
536-536: Add minimal POSIX shell quoting helper used by testsProvides safe single-quote escaping for filter queries and paths.
+// Minimal POSIX-like shell quoting to prevent globbing, brace expansion, pipes, and history expansion. +func shQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" +}
🧹 Nitpick comments (3)
test/integration_docs_test.go (3)
317-323: Replace name-based skip with per-case support flagAvoid brittle name checks; keep unsupported cases documented in-table.
- testCases := []struct { + testCases := []struct { name string fixtureDir string filterQuery string expectedOutput string extraFlags string + supported bool }{ @@ - if tc.name != "intersection-by-path-and-attribute" { - t.Skip("Skipping test case - not supported yet") + if !tc.supported { + t.Skip("Skipping test case - scenario not yet supported") return }And mark supported cases, e.g.:
{ name: "intersection-by-path-and-attribute", fixtureDir: "intersection", filterQuery: "./prod/**|type=unit", // Our testing arg parsing is busted. Don't put whitespace between these. expectedOutput: "prod/units/unit1\nprod/units/unit2\n", + supported: true, },Also applies to: 444-448
352-356: Clarify test name and normalize path separators for cross-platform stabilityThis is a glob, not an exact match; use ToSlash to avoid backslashes on Windows when filtering by absolute path.
- name: "path-based-absolute-exact-match", + name: "path-based-absolute-glob", fixtureDir: "path-based", - filterQuery: filepath.Join(tmpDir, "path-based", "root", "envs", "dev", "apps", "*"), + filterQuery: filepath.ToSlash(filepath.Join(tmpDir, "path-based", "root", "envs", "dev", "apps", "*")), expectedOutput: "envs/dev/apps/app1\nenvs/dev/apps/app2\n",
538-548: Mark helper functions as test helpers for clearer failure locationsAdd t.Helper() at the top of each helper.
func generateNameBasedFixture(t *testing.T, baseDir string) { + t.Helper() fixtureDir := filepath.Join(baseDir, "name-based", "root", "apps") @@ func generateAttributeBasedFixture(t *testing.T, baseDir string) { + t.Helper() rootDir := filepath.Join(baseDir, "attribute-based", "root") @@ func generatePathBasedFixture(t *testing.T, baseDir string) { + t.Helper() rootDir := filepath.Join(baseDir, "path-based", "root") @@ func generateNegationFixture(t *testing.T, baseDir string) { + t.Helper() rootDir := filepath.Join(baseDir, "negation", "root") @@ func generateIntersectionFixture(t *testing.T, baseDir string) { + t.Helper() rootDir := filepath.Join(baseDir, "intersection", "root") @@ func generateUnionFixture(t *testing.T, baseDir string) { + t.Helper() rootDir := filepath.Join(baseDir, "union", "root") @@ func createTerragruntUnit(t *testing.T, dir string) { + t.Helper() require.NoError(t, os.MkdirAll(dir, 0755)) @@ func createTerragruntStack(t *testing.T, dir string) { + t.Helper() require.NoError(t, os.MkdirAll(dir, 0755)) @@ func createTerragruntUnitWithDependency(t *testing.T, dir, dep string) { + t.Helper() require.NoError(t, os.MkdirAll(dir, 0755))Also applies to: 550-563, 565-581, 583-599, 601-621, 623-651, 655-661, 662-667, 668-680
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs-starlight/src/data/flags/filter.mdx(1 hunks)test/integration_docs_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs-starlight/src/data/flags/filter.mdx
🧰 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:
test/integration_docs_test.go
🧬 Code graph analysis (1)
test/integration_docs_test.go (2)
test/helpers/test_helpers.go (1)
IsExperimentMode(65-71)test/helpers/package.go (1)
RunTerragruntCommandWithOutput(993-997)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: license_check / License Check
- GitHub Check: lint / lint
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: build-and-test
🔇 Additional comments (1)
test/integration_docs_test.go (1)
420-436: Intersection expected outputs corrected — LGTMThe expectations now align with the intended intersection semantics.
2 participants
Description
Documents
filter-flagexperiment.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added documentation for
--filterflag.Migration Guide
Summary by CodeRabbit
New Features
Documentation
Tests