feat: performance improvement on -version execution #4288
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough""" WalkthroughA version cache mechanism was introduced to optimize the retrieval of the Terraform version in the Terragrunt CLI. Context management utilities for the cache were added, and the version check logic was updated to use this cache. Integration tests verify the number of Terraform version command invocations. New Terraform and Terragrunt configuration fixtures support these tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant RunContext
participant RunVersionCache
participant VersionCheck
participant Terraform
App->>RunContext: Start application
RunContext->>RunVersionCache: WithRunVersionCache(ctx)
RunContext->>VersionCheck: PopulateTerraformVersion(ctx, options)
VersionCheck->>RunVersionCache: GetRunVersionCache(ctx)
alt Cache hit
RunVersionCache-->>VersionCheck: Return cached version output
VersionCheck-->>RunContext: Use cached version info
else Cache miss
VersionCheck->>Terraform: Run terraform --version
Terraform-->>VersionCheck: Return version output
VersionCheck->>RunVersionCache: Store version output in cache
VersionCheck-->>RunContext: Use new version info
end
Suggested reviewers
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (5)
🧰 Additional context used📓 Path-based instructions (1)`**/*.go`: 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.
🧬 Code Graph Analysis (1)test/integration_test.go (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
cli/commands/run/version_check.go
Outdated
| func computeVersionFilesCacheKey(workingDir string) string { | ||
| var hashes []string | ||
|
|
||
| files := []string{".terraform-version", ".tool-versions"} |
There was a problem hiding this comment.
This could be many more files including mise.toml or .mise.toml.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/app.go (1)
111-111: Approve caching of Terraform version & suggest documenting it
Wrapping the context withrun.WithRunVersionCache(ctx)ensures that the-versioncommand is invoked only once per run, delivering the intended performance improvement. Its placement—after engine context setup and before command execution—is appropriate.
Consider adding a brief comment above this line to explain the purpose of the version cache.cli/commands/run/version_check.go (1)
240-263: Consider some enhancements to the cache key computation.The function is well-implemented, but has a few areas for improvement:
- It silently ignores errors from FileSHA256 without logging
- The version file names are hard-coded within the function
Consider these improvements:
+// Version files that are checked to compute the cache key +var VersionFiles = []string{".terraform-version", ".tool-versions"} // Helper to compute a cache key from the checksums of .terraform-version and .tool-versions func computeVersionFilesCacheKey(workingDir string) string { var hashes []string - files := []string{".terraform-version", ".tool-versions"} + files := VersionFiles for _, file := range files { path := filepath.Join(workingDir, file) if util.FileExists(path) { hash, err := util.FileSHA256(path) if err == nil { hashes = append(hashes, file+":"+hex.EncodeToString(hash)) + } else { + // Log error for debugging but continue + // terragruntOptions.Logger.Debugf("Error hashing file %s: %v", path, err) } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/app.go(2 hunks)cli/commands/run/context.go(1 hunks)cli/commands/run/version_check.go(4 hunks)test/integration_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: 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.
**/*.go: 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.
cli/app.gocli/commands/run/version_check.gotest/integration_test.gocli/commands/run/context.go
🧬 Code Graph Analysis (4)
cli/app.go (1)
cli/commands/run/context.go (1)
WithRunVersionCache(17-20)
cli/commands/run/version_check.go (3)
cli/commands/run/context.go (1)
GetRunVersionCache(23-25)util/file.go (2)
FileExists(57-60)FileSHA256(805-831)util/hash.go (1)
EncodeBase64Sha1(16-19)
test/integration_test.go (2)
test/helpers/package.go (4)
RunTerragruntCommand(785-789)CopyEnvironment(83-96)CleanupTerraformFolder(719-726)RunTerragruntCommandWithOutput(827-831)util/file.go (1)
JoinPath(472-474)
cli/commands/run/context.go (1)
internal/cache/cache.go (3)
NewCache(24-30)Cache(17-21)ContextCache(119-126)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test (macos)
- GitHub Check: build-and-test
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (8)
cli/app.go (1)
10-10: Approve import of run package
The new import"github.com/gruntwork-io/terragrunt/cli/commands/run"is necessary to enable the version cache in the CLI and is correctly added.test/integration_test.go (3)
19-19: Import statement correctedRemoved erroneous alias from the import of the print package, making the code cleaner and more maintainable.
1961-1961: Fixed test working directory pathChanged the working directory argument in
TestDependencyMockOutputRestrictedfromdependent2PathtorootPath. This properly sets the validation test to run from the root directory rather than from a subdirectory.
4140-4155: Good test for version caching mechanismThis new test validates that the
-versioncommand is invoked only once during a multi-module run, confirming that the caching mechanism is working correctly. The test effectively:
- Sets up a test environment with multiple modules
- Runs a terragrunt command with tracing enabled
- Verifies via regex that the version command appears exactly once in the logs
This directly tests the feature described in the PR objectives.
cli/commands/run/context.go (1)
1-26: Clean implementation of context-based version caching.The context management is well-structured, using a typed key and constants for the cache. The function names are clear and follow Go conventions. The use of generics for the cache implementation is appropriate for type safety.
cli/commands/run/version_check.go (3)
92-112: Well-implemented caching mechanism.The caching implementation effectively checks for cached version output before running the Terraform command, which should provide the intended performance improvement for the
-versioncommand execution.
133-134: Good cache update approach.Storing the command output in the cache after execution ensures it will be available for subsequent calls.
7-8: Good import organization and hex encoding usage.The addition of the hex encoding import and path/filepath packages correctly supports the new caching functionality.
Also applies to: 11-12
| @@ -4136,3 +4136,20 @@ func TestTfPath(t *testing.T) { | |||
|
|
|||
| assert.Regexp(t, "(?i)(terraform|opentofu)", stdout+stderr) | |||
| } | |||
|
|
|||
| func TestVersionIsInvokedOnlyOnce(t *testing.T) { | |||
There was a problem hiding this comment.
We should include a version where at least two fire from .tool-version files, etc.
| cacheKey = strings.Join(hashes, "|") | ||
| } | ||
|
|
||
| return util.EncodeBase64Sha1(cacheKey) |
There was a problem hiding this comment.
NIT: Should we be SHA-ing this? It's going to get hashed by the standard library when used for the key of the map, right? I don't think it's super important, just trying to confirm my understanding right.
There was a problem hiding this comment.
util.FileSHA256(path) - returns 8kb byte array, encoded in hex it is quite large key.
sha1.Sum([]byte(str)) - return 20 bytes
There was a problem hiding this comment.
I was just saying that I don't know that we need to hash this, as it's used as the key for the hashmap, which is going to get hashed by the standard library, right?
| @@ -27,6 +31,8 @@ var TerraformVersionRegex = regexp.MustCompile(`^(\S+)\s(v?\d+\.\d+\.\d+)`) | |||
|
|
|||
| const versionParts = 3 | |||
|
|
|||
| var versionFiles = []string{".terraform-version", ".tool-versions", "mise.toml", ".mise.toml"} | |||
There was a problem hiding this comment.
Do you think we should make a ticket to backlog making this configurable? Maybe a user can just feed Terragrunt the files that might be used to control TF versions outside these defaults?
There was a problem hiding this comment.
Will raise another ticket for this
Description
Test results on running 10 times a stack of 20 units on different versions vs current branch
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit