chore: Disable experimental tests ignore in CI #5021
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemove ignored failures from CI workflow; resolve symlinks in many tests and remove extra-version-command detection; migrate filesystem walking APIs to WalkDir/DirEntry and add WalkDirWithSymlinks; add CAS gating and fallback in source download; make ProviderCache concurrency-safe; introduce HCL diagnostics handler and Stack-specific parsing; numerous Terraform provider-version bumps in test fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as downloadSource
participant CAS as CAS subsystem
participant Getter as standard go-getter
note right of Caller #DDEEF9: Source is non-local?
Caller->>Caller: isLocal := tf.IsLocalSource(src.CanonicalSourceURL)
alt allowCAS && !isLocal
Caller->>CAS: init CAS
alt CAS init OK
Caller->>CAS: attempt download (Pwd = opts.WorkingDir)
alt CAS download success
CAS-->>Caller: return downloaded path
else CAS download fails
CAS-->>Caller: error
Caller->>Getter: fallback download
Getter-->>Caller: return downloaded path or error
end
else CAS init fails
CAS-->>Caller: error (logged)
Caller->>Getter: fallback download
Getter-->>Caller: return downloaded path or error
end
else
Caller->>Getter: standard download (local or CAS disabled)
Getter-->>Caller: return downloaded path or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
2 times, most recently
from
810762b to
4e88bb5
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
4e88bb5 to
c70563d
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
c70563d to
2414861
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
2414861 to
c9dff4c
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
c9dff4c to
ed880b5
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
ed880b5 to
a62cb8a
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
a62cb8a to
399e56b
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
06feeae to
aab6976
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
ac8d424 to
94d4d0e
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
09eb8dd to
c574388
Compare
fix: Fixing `TestVersionIsInvokedInDifferentDirectory` and `TestVersionIsInvokedOnlyOnce` in experiment mode fix: Fixing logic for early return in `hcl fmt` fix: Disable CAS for local files fix: Fixing some logic around validation with filter flags fix: Adding ignore for counter files fix: Adding some synchronization for provider cache fix: Fixing null provider pin fix: Making tests a little more reliable fix: Use `WalkDir` implementation for both symlink and non-symlink implementations chore: Refactor all usage of `WalkWithSymlinks` to `WalkDirWithSymlinks` fix: Fixing commit reference
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
from
c574388 to
4de37b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tf/source.go (1)
85-120: Restore Lstat fallback for dangling symlinks
DirEntry.Info()will surfaceErrNotExistwhen the entry can’t be re-stated (e.g. a dangling symlink or a path that disappears between the read and the stat). In the oldfilepath.Walk-based flow we always received anos.FileInfofromos.Lstat, so these entries still participated in the hash. With the new code the error bubbles up,EncodeSourceVersionfails, and Terragrunt falls back to a random version string on every run, forcing redundant downloads for any local module tree that contains a broken link (common on Windows repos checked out with dangling toolchain links). The Go docs call out theErrNotExistbehavior explicitly, while the previous implementation relied onlstatsemantics, so this is a regression.(text.baldanders.info)Please mirror the old behavior by retrying with
os.Lstat(path)whend.Info()fails, e.g.:- info, err := d.Info() - if err != nil { - return err - } + info, err := d.Info() + if err != nil { + info, err = os.Lstat(path) + if err != nil { + return err + } + }(Same adjustment is needed in the non-symlink branch below.) That keeps dangling links from breaking the local source hash while still surfacing genuine directory walk errors.
Also applies to: 117-120
♻️ Duplicate comments (2)
test/fixtures/stack/stage/search-app/main.tf (1)
9-9: Floating version in test fixture.Same consideration as other fixtures regarding test determinism with
~> 3.2.4.test/fixtures/hooks/before-after-and-on-error/main.tf (1)
7-7: Floating version in test fixture.Same consideration as other fixtures regarding test determinism.
🧹 Nitpick comments (8)
test/fixtures/hooks/interpolations/main.tf (1)
7-7: Consider pinning exact provider versions in test fixtures.The change from
"3.2.3"to"~> 3.2.4"introduces a floating version constraint that allows patch-level updates (e.g., 3.2.5, 3.2.6). While this may be intentional for testing against the latest compatible versions, it can lead to non-deterministic test behavior across different environments or time periods as new patch versions are released.Note: This pattern is applied consistently across multiple test fixtures in this PR (hooks/interpolations, hooks/skip-on-error, download/stdout, streaming/unit1, streaming/unit2, stack/mgmt/vpc, stack/stage/frontend-app), suggesting it's a deliberate choice.
test/integration_run_cmd_flags_test.go (1)
65-70: Consider extracting cleanup logic to reduce duplication.The cleanup logic is duplicated between the pre-test cleanup (lines 48-51) and the
t.Cleanupregistration. Consider extracting this into a helper function.Example refactor:
+func cleanupCounterFiles(scriptsPath string) { + _ = os.Remove(filepath.Join(scriptsPath, "global_counter.txt")) + _ = os.Remove(filepath.Join(scriptsPath, "no_cache_counter.txt")) +} + func runCmdFlagsFixture(t *testing.T) runCmdFixtureResult { t.Helper() @@ -45,10 +50,7 @@ func runCmdFlagsFixture(t *testing.T) runCmdFixtureResult { helpers.CleanupTerraformFolder(t, modulePath) } - // Clean up counter files from previous test runs in the fixture directory scriptsPath := filepath.Join(testFixtureRunCmdFlags, "scripts") - _ = os.Remove(filepath.Join(scriptsPath, "global_counter.txt")) - _ = os.Remove(filepath.Join(scriptsPath, "no_cache_counter.txt")) + cleanupCounterFiles(scriptsPath) tmpEnvPath := helpers.CopyEnvironment(t, testFixtureRunCmdFlags) rootPath := util.JoinPath(tmpEnvPath, testFixtureRunCmdFlags) @@ -62,11 +64,8 @@ func runCmdFlagsFixture(t *testing.T) runCmdFixtureResult { stdout, stderr, err := helpers.RunTerragruntCommandWithOutput(t, cmd) require.NoError(t, err) - // Clean up counter files after test execution t.Cleanup(func() { - scriptsPath := filepath.Join(testFixtureRunCmdFlags, "scripts") - _ = os.Remove(filepath.Join(scriptsPath, "global_counter.txt")) - _ = os.Remove(filepath.Join(scriptsPath, "no_cache_counter.txt")) + cleanupCounterFiles(filepath.Join(testFixtureRunCmdFlags, "scripts")) }) return runCmdFixtureResult{internal/experiment/experiment.go (1)
133-141: Minor inconsistency: EnableExperiment allows enabling any experiment but error lists only ongoing.The refactored logic now enables any experiment by name (including completed experiments), but the error message at line 140 only lists ongoing experiments. While enabling completed experiments is harmless (they're always evaluated as enabled per line 188), this creates a subtle inconsistency: users can successfully enable completed experiments, but the error message suggests only ongoing experiments are valid.
Consider either:
- Restricting
EnableExperimentto only enable ongoing experiments (checke.Status == StatusOngoingbefore enabling)- Updating the error message to list all experiments, not just ongoing ones
test/integration_destroy_test.go (1)
53-59: Optional: Multiline command formatting improves readability.The refactored command construction using
fmt.Sprintfwith multiline formatting is clearer than the previous compact format.util/file.go (1)
1046-1125: Verify symlink cycle prevention logic.The
WalkDirWithSymlinksimplementation looks solid with dual tracking:
visitedmap prevents circular symlinks using "realPath:symlinkPath" keysvisitedLogicalmap prevents duplicate processing of the same logical pathHowever, please verify the logic on Line 1096:
if visited[realPath+":"+currentPath] {This concatenation could theoretically create collisions if paths contain
:. While unlikely in practice, consider using a struct as the map key for stronger guarantees:type pathKey struct { real, current string } visited := make(map[pathKey]bool)test/fixtures/hooks/before-and-after/main.tf (1)
7-7: Consider using exact version for test fixture determinism.The floating version constraint
~> 3.2.4allows automatic patch updates (e.g., 3.2.5, 3.2.6) which may introduce non-deterministic behavior in tests. For test fixtures, exact versions typically provide more reproducible results.If determinism is important for these tests, consider pinning to an exact version:
- version = "~> 3.2.4" + version = "3.2.4"However, if the intent is to test against the latest patch releases, the current approach is acceptable.
test/fixtures/download/hello-world/main.tf (1)
7-7: Consider exact version for test fixture reproducibility.Similar to other fixtures, the floating version
~> 3.2.4may introduce non-deterministic test behavior. See comment in test/fixtures/hooks/before-and-after/main.tf.test/fixtures/buffer-module-output/app/main.tf (1)
4-5: Provider source simplified and version updated.The source simplification from
registry.terraform.io/hashicorp/nulltohashicorp/nullis standard Terraform notation. The floating version constraint has the same consideration as other fixtures regarding test determinism.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
.github/workflows/integration-test.yml(0 hunks)cli/commands/hcl/validate/validate.go(3 hunks)cli/commands/run/download_source.go(3 hunks)config/config.go(2 hunks)config/stack.go(2 hunks)internal/discovery/discovery.go(1 hunks)internal/experiment/experiment.go(1 hunks)internal/filter/ast.go(1 hunks)internal/filter/filters.go(1 hunks)internal/services/catalog/module/repo.go(2 hunks)mise.toml(1 hunks)test/fixtures/buffer-module-output/app/main.tf(1 hunks)test/fixtures/buffer-module-output/app2/main.tf(1 hunks)test/fixtures/buffer-module-output/app3/main.tf(1 hunks)test/fixtures/download/hello-world/main.tf(2 hunks)test/fixtures/download/local-windows/JZwoL6Viko8bzuRvTOQFx3Jh8vs/3mU4huxMLOXOW5ZgJOFXGUFDKc8/main.tf(1 hunks)test/fixtures/download/remote-ref/terragrunt.hcl(1 hunks)test/fixtures/download/remote-relative-with-slash/terragrunt.hcl(1 hunks)test/fixtures/download/remote-relative/terragrunt.hcl(1 hunks)test/fixtures/download/remote/terragrunt.hcl(1 hunks)test/fixtures/download/stdout/main.tf(1 hunks)test/fixtures/gcs-byo-bucket/main.tf(1 hunks)test/fixtures/gcs/main.tf(1 hunks)test/fixtures/hcl-filter/validate/stacks/syntax-error/stack2/terragrunt.stack.hcl(1 hunks)test/fixtures/hcl-filter/validate/stacks/valid/stack1/terragrunt.stack.hcl(1 hunks)test/fixtures/hooks/after-only/main.tf(1 hunks)test/fixtures/hooks/all/after-only/main.tf(1 hunks)test/fixtures/hooks/all/before-only/main.tf(1 hunks)test/fixtures/hooks/bad-arg-action/empty-command-list/main.tf(1 hunks)test/fixtures/hooks/bad-arg-action/empty-string-command/main.tf(1 hunks)test/fixtures/hooks/before-after-and-on-error/main.tf(1 hunks)test/fixtures/hooks/before-and-after/main.tf(1 hunks)test/fixtures/hooks/before-only/main.tf(1 hunks)test/fixtures/hooks/init-once/no-source-no-backend/main.tf(1 hunks)test/fixtures/hooks/interpolations/main.tf(1 hunks)test/fixtures/hooks/one-arg-action/main.tf(1 hunks)test/fixtures/hooks/skip-on-error/main.tf(1 hunks)test/fixtures/include/qa/my-app/main.tf(1 hunks)test/fixtures/include/stage/my-app/main.tf(1 hunks)test/fixtures/parent-folders/terragrunt-in-root/child/sub-child/sub-sub-child/main.tf(1 hunks)test/fixtures/report/first-failure/main.tf(1 hunks)test/fixtures/report/second-failure/main.tf(1 hunks)test/fixtures/run-cmd-flags/scripts/.gitignore(1 hunks)test/fixtures/stack/mgmt/bastion-host/main.tf(1 hunks)test/fixtures/stack/mgmt/kms-master-key/main.tf(1 hunks)test/fixtures/stack/mgmt/vpc/main.tf(1 hunks)test/fixtures/stack/stage/backend-app/main.tf(1 hunks)test/fixtures/stack/stage/frontend-app/main.tf(1 hunks)test/fixtures/stack/stage/mysql/main.tf(1 hunks)test/fixtures/stack/stage/redis/main.tf(1 hunks)test/fixtures/stack/stage/search-app/main.tf(1 hunks)test/fixtures/stack/stage/vpc/main.tf(1 hunks)test/fixtures/streaming/unit1/main.tf(1 hunks)test/fixtures/streaming/unit2/main.tf(1 hunks)test/fixtures/tflint/module-found/dummy_module/main.tf(1 hunks)test/helpers/package.go(1 hunks)test/integration_destroy_test.go(7 hunks)test/integration_download_test.go(4 hunks)test/integration_hcl_filter_test.go(5 hunks)test/integration_hooks_test.go(2 hunks)test/integration_run_cmd_flags_test.go(2 hunks)test/integration_runner_pool_test.go(2 hunks)test/integration_serial_test.go(2 hunks)test/integration_test.go(1 hunks)test/integration_unix_test.go(1 hunks)tf/cache/services/provider_cache.go(7 hunks)tf/source.go(8 hunks)util/file.go(1 hunks)util/file_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/integration-test.yml
✅ Files skipped from review due to trivial changes (3)
- test/fixtures/stack/stage/redis/main.tf
- test/fixtures/run-cmd-flags/scripts/.gitignore
- test/fixtures/hooks/one-arg-action/main.tf
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration_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:
test/integration_run_cmd_flags_test.gotest/integration_runner_pool_test.gointernal/discovery/discovery.gointernal/filter/filters.gotest/integration_download_test.gointernal/services/catalog/module/repo.goconfig/config.gotf/source.gotest/integration_hooks_test.goutil/file.gointernal/filter/ast.gotest/integration_hcl_filter_test.gotest/integration_unix_test.gotest/helpers/package.gotf/cache/services/provider_cache.gotest/integration_serial_test.gocli/commands/hcl/validate/validate.goconfig/stack.gotest/integration_destroy_test.gointernal/experiment/experiment.gocli/commands/run/download_source.goutil/file_test.go
🧠 Learnings (4)
📚 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:
test/integration_runner_pool_test.gointernal/services/catalog/module/repo.gotest/fixtures/download/remote-relative-with-slash/terragrunt.hclutil/file.gocli/commands/hcl/validate/validate.goconfig/stack.go
📚 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:
test/fixtures/download/remote-relative-with-slash/terragrunt.hcltest/integration_hcl_filter_test.gocli/commands/hcl/validate/validate.go
📚 Learning: 2025-09-10T04:41:35.652Z
Learnt from: jorhett
PR: gruntwork-io/terragrunt#1234
File: errors/multierror.go:35-39
Timestamp: 2025-09-10T04:41:35.652Z
Learning: In Terragrunt's MultiError.Error() method, checking for the exact string "exit status 2" and returning it directly is not a brittle hack but a semantic fix. Terraform's --detailed-exitcode flag uses exit code 2 to mean "plan succeeded with changes" (not an error), so when multiple modules return this status, it should not be wrapped in "Hit multiple errors:" formatting as that misrepresents successful operations as errors.
Applied to files:
test/integration_hooks_test.gotest/integration_destroy_test.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:
test/integration_hcl_filter_test.gocli/commands/hcl/validate/validate.go
🧬 Code graph analysis (15)
test/integration_runner_pool_test.go (1)
test/helpers/package.go (1)
RunTerragruntCommandWithOutput(1007-1011)
internal/filter/filters.go (1)
internal/component/component.go (2)
Components(302-302)NewUnit(55-61)
test/integration_download_test.go (2)
test/helpers/package.go (2)
CopyEnvironment(89-105)CleanupTerragruntFolder(891-895)util/file.go (1)
JoinPath(626-628)
internal/services/catalog/module/repo.go (1)
util/file.go (1)
WalkDirWithSymlinks(1046-1125)
config/config.go (2)
internal/experiment/experiment.go (1)
Symlinks(18-18)util/file.go (1)
WalkDirWithSymlinks(1046-1125)
tf/source.go (1)
util/file.go (3)
WalkDirWithSymlinks(1046-1125)TerraformLockFile(29-29)CanonicalPath(88-99)
test/integration_hooks_test.go (1)
test/helpers/package.go (1)
RunTerragruntCommand(965-969)
test/integration_hcl_filter_test.go (1)
test/helpers/package.go (1)
RunTerragruntCommandWithOutput(1007-1011)
test/integration_unix_test.go (2)
test/helpers/package.go (3)
CopyEnvironment(89-105)CleanupTerraformFolder(882-889)RunTerragrunt(979-983)util/file.go (1)
JoinPath(626-628)
tf/cache/services/provider_cache.go (3)
util/collections.go (1)
ListContainsElement(26-28)tf/cache/helpers/http.go (1)
Fetch(19-44)tf/getproviders/provider.go (1)
Provider(12-30)
cli/commands/hcl/validate/validate.go (7)
config/hclparse/options.go (1)
WithDiagnosticsHandler(84-92)config/hclparse/file.go (1)
File(17-21)internal/view/diagnostic/diagnostic.go (2)
Diagnostics(9-9)NewDiagnostic(29-63)config/stack.go (3)
Stack(69-76)ReadValues(995-1028)ParseStackConfig(888-927)internal/component/component.go (1)
Stack(176-184)config/config.go (1)
DefaultStackFile(49-49)config/parsing_context.go (1)
NewParsingContext(62-73)
config/stack.go (1)
util/file.go (2)
WalkDirWithSymlinks(1046-1125)IsDir(329-332)
test/integration_destroy_test.go (2)
test/helpers/package.go (4)
RunTerragruntCommandWithOutput(1007-1011)CopyEnvironment(89-105)CleanupTerraformFolder(882-889)RunTerragrunt(979-983)util/file.go (1)
JoinPath(626-628)
cli/commands/run/download_source.go (1)
tf/source.go (1)
IsLocalSource(316-318)
util/file_test.go (1)
util/file.go (1)
WalkDirWithSymlinks(1046-1125)
⏰ 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). (2)
- GitHub Check: integration_tests / Test (AWS Tofu)
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (60)
test/fixtures/hooks/init-once/no-source-no-backend/main.tf (1)
7-7: Provider version constraint update aligns with PR objectives.The update from a pinned version to a floating patch constraint (
~> 3.2.4) allows for patch-level updates while maintaining compatibility. This aligns with the PR's documented pattern of updating null provider version constraints across test fixtures.test/fixtures/hooks/bad-arg-action/empty-command-list/main.tf (1)
7-7: Provider version constraint updated appropriately.The floating constraint
~> 3.2.4allows for compatible patch-level updates while maintaining stability in test fixtures. No concerns.test/fixtures/hooks/bad-arg-action/empty-string-command/main.tf (2)
7-7: Provider version constraint updated appropriately.The floating constraint
~> 3.2.4aligns with the version updates across test fixtures. Consistent with file 1.
18-20: Note: Pre-existing undefined data reference.The output block references
data.template_file.example.renderedbut no corresponding data source is defined in this file. This appears to be a pre-existing issue (not introduced by this PR). If this fixture is used in tests, confirm that this reference doesn't cause test failures.internal/filter/ast.go (1)
123-133: LGTM! Bug fix and improved consistency.This change correctly checks both the left and right sides of the infix expression, fixing the previous bug where only the left side was evaluated. Returning the
InfixExpressionitself (rather than a sub-expression) is more consistent with how other expression types implement this method and makes the API more intuitive.internal/filter/filters.go (2)
121-124: LGTM! Type consistency improvement.The change from
[]component.Componenttocomponent.Componentsaligns with the canonical type used throughout the component package.
126-128: LGTM! Correct fix for empty filter handling.The early return when no filters are present ensures all constructed components are returned, fixing the bug where empty filters didn't return the full slice. This also avoids an unnecessary call to
Evaluate.test/fixtures/stack/mgmt/bastion-host/main.tf (1)
9-9: LGTM: Provider version constraint updated for test stability.The change from exact version "3.2.3" to pessimistic constraint "~> 3.2.4" allows automatic patch updates (3.2.x) while preventing minor/major version drift. This is appropriate for test fixtures and aligns with the PR's goal of stabilizing experimental tests in CI.
mise.toml (1)
3-3: LGTM: OpenTofu patch version update.Routine patch version bump from 1.10.5 to 1.10.6, consistent with the PR's test infrastructure updates.
test/fixtures/report/second-failure/main.tf (1)
7-7: LGTM: Consistent provider version update.Same provider constraint update as other fixtures in this PR, allowing patch-level updates within the 3.2.x range.
test/fixtures/hooks/after-only/main.tf (1)
7-7: LGTM: Provider version aligned with other test fixtures.test/fixtures/tflint/module-found/dummy_module/main.tf (1)
6-6: LGTM: Provider constraint tightened for test reproducibility.This change from ">= 3.2.3" (loose) to "~> 3.2.4" (pessimistic) actually restricts the allowed versions to the 3.2.x range, improving test stability and reproducibility.
internal/discovery/discovery.go (1)
625-625: LGTM: Formatting improvement.The added blank line improves readability by visually separating the variable initialization from the conditional logic below.
test/fixtures/download/local-windows/JZwoL6Viko8bzuRvTOQFx3Jh8vs/3mU4huxMLOXOW5ZgJOFXGUFDKc8/main.tf (1)
7-7: LGTM: Provider version update consistent with other fixtures.test/fixtures/include/stage/my-app/main.tf (1)
9-9: LGTM: Final provider version update completes the fixture alignment.test/integration_runner_pool_test.go (1)
4-4: LGTM! Symlink resolution improves test reliability.The changes correctly normalize the test path by resolving symlinks before running Terragrunt commands:
- Added
path/filepathimport forEvalSymlinks(line 4)- Proper error handling with
require.NoErrorafter symlink resolution (lines 147-148)- Correct reuse of the
errvariable with=instead of:=on line 150 (avoiding variable shadowing)This aligns with the broader pattern of symlink resolution improvements across the test suite mentioned in the AI summary.
Also applies to: 147-150
test/helpers/package.go (1)
101-102: LGTM! Good symlink resolution for test isolation.Resolving symlinks in the temporary directory path ensures consistent test working directory paths across different environments. This aligns with the broader PR changes for improved symlink handling in tests.
test/fixtures/download/remote/terragrunt.hcl (1)
6-6: Temporary commit pinning - convert to tag reference after merge.The source reference has been updated from a version tag to a specific commit hash. Per the PR objectives, this should be converted to a tag after merge.
Ensure this commit hash is converted to a proper version tag reference once the changes are merged to avoid drift in test fixtures.
test/fixtures/download/remote-relative-with-slash/terragrunt.hcl (1)
6-6: Temporary commit pinning - convert to tag reference after merge.Consistent with the other fixture updates, this commit hash should be replaced with a proper version tag reference post-merge.
test/fixtures/include/qa/my-app/main.tf (1)
7-7: LGTM! Provider version constraint updated consistently.The change from an exact version (
3.2.3) to a pessimistic constraint (~> 3.2.4) allows for patch-level updates while maintaining compatibility. This aligns with the PR objective to use floating version constraints for the null provider in test fixtures.test/integration_run_cmd_flags_test.go (1)
48-51: Good test hygiene - cleanup ensures clean state.Pre-test cleanup of counter files from previous runs ensures tests start with a clean slate.
test/fixtures/buffer-module-output/app2/main.tf (1)
6-7: LGTM! Provider configuration updated to use shorthand source and floating version.Two improvements here:
- Simplified provider source from full registry URL to idiomatic shorthand (
hashicorp/null)- Updated version constraint from exact to pessimistic (
~> 3.2.4) for patch-level flexibilityBoth changes align with Terraform best practices and the PR objectives.
test/fixtures/parent-folders/terragrunt-in-root/child/sub-child/sub-sub-child/main.tf (1)
9-9: LGTM! Consistent provider version constraint update.Version constraint updated from exact to pessimistic, consistent with the other test fixtures in this PR.
test/fixtures/hooks/all/after-only/main.tf (1)
7-7: LGTM! Provider version constraint updated consistently.Final fixture update maintains consistency with the pattern established across all test fixtures in this PR.
test/fixtures/hooks/before-only/main.tf (1)
7-7: LGTM: Provider version constraint updated to allow patch-level updates.The change from a pinned version to a pessimistic constraint (~> 3.2.4) is appropriate for test fixtures, as it allows patch-level updates while maintaining compatibility.
test/fixtures/hcl-filter/validate/stacks/valid/stack1/terragrunt.stack.hcl (1)
1-9: LGTM: Migration to explicit unit blocks.The change from map-like syntax to explicit unit blocks with both
sourceandpathattributes aligns with the Stack HCL parsing updates. The structure is consistent across both units.test/fixtures/gcs/main.tf (1)
9-9: LGTM: Consistent provider version constraint update.test/fixtures/report/first-failure/main.tf (1)
7-7: LGTM: Consistent provider version constraint update.test/fixtures/stack/mgmt/kms-master-key/main.tf (1)
9-9: LGTM: Consistent provider version constraint update.internal/services/catalog/module/repo.go (2)
6-6: LGTM: Added fs package import for DirEntry support.This import is required for the migration to
filepath.WalkDir.
99-112: LGTM: Efficient migration from Walk to WalkDir.The migration from
filepath.Walktofilepath.WalkDirimproves performance by avoiding unnecessaryLstatcalls. The callback signature is correctly updated to usefs.DirEntryinstead ofos.FileInfo, and theIsDir()check is properly adapted. The conditional use ofutil.WalkDirWithSymlinkswhen symlinks are enabled maintains the existing symlink-handling behavior.test/fixtures/hcl-filter/validate/stacks/syntax-error/stack2/terragrunt.stack.hcl (1)
1-5: LGTM: Appropriate negative test case.The intentional syntax error (missing value for
path) is clearly documented and serves as a valid test case for HCL validation error handling.internal/experiment/experiment.go (1)
124-128: LGTM: Simplified experiment mode enablement.The refactored logic is clearer and equivalent to the previous implementation. Directly iterating and checking
Status == StatusOngoingis more straightforward than usingFilterByStatus.test/integration_destroy_test.go (2)
353-354: LGTM: Symlink resolution for tmpDir.This change correctly evaluates symlinks for the temporary directory, which aligns with the updated
CopyEnvironmenthelper that now always evaluates symlinks. This is important for cross-platform compatibility, especially on macOS where/varis often symlinked to/private/var.
401-402: LGTM: Consistent symlink resolution.Same pattern as TestStorePlanFilesRunAllDestroy—correctly resolves symlinks for the temporary directory.
util/file.go (2)
834-851: LGTM: Migration to WalkDir API.The function correctly migrates from
filepath.Walk(withos.FileInfo) tofilepath.WalkDir(withfs.DirEntry). TheWalkDirAPI is more efficient as it avoids unnecessaryLstatcalls. The callback signature change from(path string, info os.FileInfo, err error)to(path string, d fs.DirEntry, err error)is properly handled.
1026-1039: LGTM: Helper for symlink resolution.The
evalRealPathForWalkDirhelper correctly evaluates symlinks and returns both the real path and whether it's a directory. Error handling is appropriate with descriptive error messages.test/fixtures/buffer-module-output/app3/main.tf (1)
4-5: LGTM: Provider version update.The provider source simplification (removing explicit
registry.terraform.io/prefix) and version bump to~> 3.2.4are correct. The~>constraint allows patch-level updates while preventing minor version changes.test/fixtures/stack/stage/backend-app/main.tf (1)
9-9: LGTM: Consistent provider version bump.Version update to
~> 3.2.4is consistent with other test fixtures in this PR.test/fixtures/stack/stage/vpc/main.tf (1)
9-9: LGTM: Consistent provider version bump.test/fixtures/stack/stage/mysql/main.tf (1)
9-9: LGTM: Consistent provider version bump.test/fixtures/download/remote-ref/terragrunt.hcl (1)
6-6: Temporary commit hash pinning.The change from tag
v0.77.22to commit hash4de37b32367af50a3d5612e42a0965d8e477cbecis intentional per the PR description: "pinning the Terragrunt commit used for remote modules (to be converted to a tag after merge)."test/fixtures/hooks/all/before-only/main.tf (1)
7-7: LGTM: Consistent provider version bump.test/fixtures/download/hello-world/main.tf (1)
32-32: Commit hash pinning noted for conversion.The PR description mentions this commit hash will be converted to a tag after merge. This is good for reproducibility in the interim.
test/fixtures/download/remote-relative/terragrunt.hcl (1)
6-6: Source pinning to commit hash is appropriate.Pinning to a specific commit ensures reproducible test behavior. As noted in the PR description, this will be converted to a tag after merge.
test/integration_hooks_test.go (2)
111-116: LGTM! Formatting improves readability.The multi-line formatting of the
RunTerragruntCommandcall enhances readability without changing logic.
126-132: LGTM! Assertion formatting improved.The multi-line formatting makes the assertions clearer and easier to read.
config/stack.go (3)
6-6: LGTM! Required import for WalkDir migration.The
io/fsimport is necessary for thefs.DirEntrytype used in the WalkDir callback.
1094-1097: LGTM! Correct migration to WalkDir API.The migration from
filepath.Walktofilepath.WalkDir(andutil.WalkWithSymlinkstoutil.WalkDirWithSymlinks) is appropriate. The WalkDir API is more efficient as it usesfs.DirEntrywhich doesn't require a separateLstatcall for each file.
1104-1112: LGTM! Callback signature correctly updated.The walk function callback signature has been properly updated from
(path string, info os.FileInfo, err error)to(path string, d fs.DirEntry, err error)to match thefs.WalkDirFuncsignature. Thed.IsDir()call is the correct way to check if an entry is a directory with the DirEntry API.test/integration_serial_test.go (2)
782-783: LGTM! Symlink resolution ensures consistent test paths.The addition of
filepath.EvalSymlinksaligns with the broader symlink-handling improvements in this PR. This ensures the test operates on the canonical path regardless of symlinks in the test environment.
801-802: LGTM! Consistent symlink resolution applied.This matches the pattern introduced in
TestVersionIsInvokedInDifferentDirectoryand ensures consistent behavior across both test functions.test/integration_download_test.go (2)
362-366: LGTM! Improved fixture availability for include tests.Copying the entire
fixtures/downloaddirectory ensures all referenced sources are available, addressing potential issues with missing dependencies. The symlink resolution is consistent with the broader PR changes.
463-467: LGTM! Consistent fixture handling applied.The same pattern from
TestIncludeDirsis correctly applied here, ensuring consistency across filter-based tests.config/config.go (1)
9-9: LGTM! Proper migration to filepath.WalkDir API.The migration from
filepath.Walktofilepath.WalkDiris correctly implemented:
- Uses the more efficient
fs.DirEntryinterface instead ofos.FileInfo- Properly switches to
WalkDirWithSymlinkswhen the symlinks experiment is enabled- Correctly updates the
IsDir()check to use the DirEntry interfaceAlso applies to: 1091-1102
util/file_test.go (3)
553-553: LGTM! Test updated to use WalkDirWithSymlinks API.The migration from
WalkWithSymlinkstoWalkDirWithSymlinkscorrectly updates the callback signature to usefs.DirEntry.
632-632: LGTM! Consistent API migration applied.
690-706: LGTM! Test function properly renamed and updated.The test function name correctly reflects the API change, and both error test cases are consistently updated to use
WalkDirWithSymlinks.cli/commands/run/download_source.go (2)
56-56: LGTM! Appropriate reuse of error variable.The change from
:=to=correctly reuses theerrvariable declared at line 51, avoiding variable shadowing.
313-348: LGTM! CAS integration with proper fallback logic.The CAS (Content Addressable Storage) feature is well-implemented with:
- Proper gating via the experiment flag
- Local source check to skip CAS for
file://sources (appropriate since CAS is for remote content)- Graceful fallback to standard getter on CAS failure
- Clear debug/warning messages for troubleshooting
- Setting
Pwdfor go-getter v2 to resolve relative paths correctlyThe local source skip is appropriate since CAS is designed for content-addressed remote sources, not local filesystem paths.
| // Diagnostics handler to collect validation errors | ||
| diagnosticsHandler := hclparse.WithDiagnosticsHandler(func(file *hcl.File, hclDiags hcl.Diagnostics) (hcl.Diagnostics, error) { | ||
| for _, hclDiag := range hclDiags { | ||
| // Only report diagnostics that are actually in the file being parsed, | ||
| // not errors from dependencies or other files | ||
| if hclDiag.Subject != nil && file != nil { | ||
| fileFilename := file.Body.MissingItemRange().Filename | ||
|
|
||
| diagFilename := hclDiag.Subject.Filename | ||
| if diagFilename != fileFilename { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| newDiag := diagnostic.NewDiagnostic(file, hclDiag) | ||
| if !diags.Contains(newDiag) { | ||
| diags = append(diags, newDiag) | ||
| } | ||
| newDiag := diagnostic.NewDiagnostic(file, hclDiag) | ||
| if !diags.Contains(newDiag) { | ||
| diags = append(diags, newDiag) | ||
| } | ||
| } | ||
|
|
||
| return nil, nil | ||
| }), | ||
| } | ||
| return nil, nil | ||
| }) |
There was a problem hiding this comment.
Return diagnostics from the handler so downstream logic still sees them.
WithDiagnosticsHandler composes handlers; whatever you return is forwarded to the rest of the pipeline. Returning nil here swallows the parser diagnostics, so the existing Terragrunt handlers stop seeing them (we lose the default diagnostic aggregation and file-tracking logic), which regresses validation behaviour. Keep collecting the filtered diagnostics for reporting, but forward them on.
- for _, hclDiag := range hclDiags {
+ filteredDiags := hcl.Diagnostics{}
+ for _, hclDiag := range hclDiags {
// Only report diagnostics that are actually in the file being parsed,
// not errors from dependencies or other files
if hclDiag.Subject != nil && file != nil {
fileFilename := file.Body.MissingItemRange().Filename
diagFilename := hclDiag.Subject.Filename
if diagFilename != fileFilename {
continue
}
}
newDiag := diagnostic.NewDiagnostic(file, hclDiag)
if !diags.Contains(newDiag) {
diags = append(diags, newDiag)
}
+ filteredDiags = append(filteredDiags, hclDiag)
}
- return nil, nil
+ return filteredDiags, nil📝 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.
| // Diagnostics handler to collect validation errors | |
| diagnosticsHandler := hclparse.WithDiagnosticsHandler(func(file *hcl.File, hclDiags hcl.Diagnostics) (hcl.Diagnostics, error) { | |
| for _, hclDiag := range hclDiags { | |
| // Only report diagnostics that are actually in the file being parsed, | |
| // not errors from dependencies or other files | |
| if hclDiag.Subject != nil && file != nil { | |
| fileFilename := file.Body.MissingItemRange().Filename | |
| diagFilename := hclDiag.Subject.Filename | |
| if diagFilename != fileFilename { | |
| continue | |
| } | |
| } | |
| newDiag := diagnostic.NewDiagnostic(file, hclDiag) | |
| if !diags.Contains(newDiag) { | |
| diags = append(diags, newDiag) | |
| } | |
| newDiag := diagnostic.NewDiagnostic(file, hclDiag) | |
| if !diags.Contains(newDiag) { | |
| diags = append(diags, newDiag) | |
| } | |
| } | |
| return nil, nil | |
| }), | |
| } | |
| return nil, nil | |
| }) | |
| // Diagnostics handler to collect validation errors | |
| diagnosticsHandler := hclparse.WithDiagnosticsHandler(func(file *hcl.File, hclDiags hcl.Diagnostics) (hcl.Diagnostics, error) { | |
| filteredDiags := hcl.Diagnostics{} | |
| for _, hclDiag := range hclDiags { | |
| // Only report diagnostics that are actually in the file being parsed, | |
| // not errors from dependencies or other files | |
| if hclDiag.Subject != nil && file != nil { | |
| fileFilename := file.Body.MissingItemRange().Filename | |
| diagFilename := hclDiag.Subject.Filename | |
| if diagFilename != fileFilename { | |
| continue | |
| } | |
| } | |
| newDiag := diagnostic.NewDiagnostic(file, hclDiag) | |
| if !diags.Contains(newDiag) { | |
| diags = append(diags, newDiag) | |
| } | |
| filteredDiags = append(filteredDiags, hclDiag) | |
| } | |
| return filteredDiags, nil | |
| }) |
🤖 Prompt for AI Agents
In cli/commands/hcl/validate/validate.go around lines 62 to 83, the diagnostics
handler currently returns nil which swallows parser diagnostics; modify the
handler to return the diagnostics (either the original hclDiags or a filtered
slice containing the diagnostics you want forwarded) instead of nil so
downstream handlers still receive them; specifically, collect the diagnostics
you keep during the loop into a new hcl.Diagnostics slice and return that (or
return hclDiags after filtering) as the first return value with nil error.
Description
Requires that experimental integration tests pass.
As a consequence of requiring that these tests, some bug fixes were made.
These include:
RequiresHCLParsinglogic to return the parent expression instead of any half of the expression (e.g. return./foo | external=trueinstead ofexternal=trueif we're preventing attribute based expressions).hashicorp/nullversion instead of3.2.3(helps with M series macbooks running the tests not having that version available).CopyEnvironmentto always evaluate symlinks, which helps folks on M series macbooks as they run tests (t.Tempdir() points to a symlinked directory, which breaks directory comparison throughout tests).hcl validateandhcl filter--filterintegration tests.runCmdFlagsFixtureon every run, and added a.gitignoreto prevent them from being accidentally committed going forward.TestVersionIsInvokedInDifferentDirectoryto no longer expect an extra version call, as the OpenTofu auto provider cache dir feature is now GA.ProviderCachefields synchronized using mutices to ensure they're accessed safely across goroutines.mise.toml.WalkWithSymlinkstoWalkDirWithSymlinks. This was necessary to avoid issues with thesymlinksexperiment causing the experiment mode integration test to fail.This also incidentally required adding support for
terragrunt.stack.hclfiles inhcl validate.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
Chores
New Features / Improvements
Tests