-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: performance improvement on -version execution #4288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6b2ebb3
65106c4
d245cc4
bc98d1f
5ef7abc
7546ab6
7e1a79f
12394e0
c2aab7e
ee0c28b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package run | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/gruntwork-io/terragrunt/internal/cache" | ||
) | ||
|
||
type configKey byte | ||
|
||
const ( | ||
versionCacheContextKey configKey = iota | ||
versionCacheName = "versionCache" | ||
) | ||
|
||
// WithRunVersionCache initializes the version cache in the context for the run package. | ||
func WithRunVersionCache(ctx context.Context) context.Context { | ||
ctx = context.WithValue(ctx, versionCacheContextKey, cache.NewCache[string](versionCacheName)) | ||
return ctx | ||
} | ||
|
||
// GetRunVersionCache retrieves the version cache from the context for the run package. | ||
func GetRunVersionCache(ctx context.Context) *cache.Cache[string] { | ||
return cache.ContextCache[string](ctx, versionCacheContextKey) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,16 @@ import ( | |
"context" | ||
"fmt" | ||
"io" | ||
"path/filepath" | ||
"regexp" | ||
"strings" | ||
|
||
"encoding/hex" | ||
|
||
"github.com/gruntwork-io/terragrunt/internal/errors" | ||
"github.com/gruntwork-io/terragrunt/options" | ||
"github.com/gruntwork-io/terragrunt/tf" | ||
"github.com/gruntwork-io/terragrunt/util" | ||
"github.com/hashicorp/go-version" | ||
) | ||
|
||
|
@@ -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"} | ||
|
||
// CheckVersionConstraints checks the version constraints of both terragrunt and terraform. Note that as a side effect this will set the | ||
// following settings on terragruntOptions: | ||
// - TerraformPath | ||
|
@@ -85,18 +91,36 @@ func CheckVersionConstraints(ctx context.Context, terragruntOptions *options.Ter | |
|
||
// PopulateTerraformVersion populates the currently installed version of Terraform into the given terragruntOptions. | ||
func PopulateTerraformVersion(ctx context.Context, terragruntOptions *options.TerragruntOptions) error { | ||
// Discard all log output to make sure we don't pollute stdout or stderr with this extra call to '--version' | ||
versionCache := GetRunVersionCache(ctx) | ||
cacheKey := computeVersionFilesCacheKey(terragruntOptions.WorkingDir) | ||
|
||
if cachedOutput, found := versionCache.Get(ctx, cacheKey); found { | ||
terraformVersion, err := ParseTerraformVersion(cachedOutput) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
tfImplementation, err := parseTerraformImplementationType(cachedOutput) | ||
|
||
if err != nil { | ||
return err | ||
} | ||
|
||
terragruntOptions.TerraformVersion = terraformVersion | ||
|
||
terragruntOptions.TerraformImplementation = tfImplementation | ||
|
||
return nil | ||
} | ||
|
||
terragruntOptionsCopy, err := terragruntOptions.CloneWithConfigPath(terragruntOptions.TerragruntConfigPath) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
terragruntOptionsCopy.Writer = io.Discard | ||
terragruntOptionsCopy.ErrWriter = io.Discard | ||
// Remove any TF_CLI_ARGS before version checking. These are appended to | ||
// the arguments supplied on the command line and cause issues when running | ||
// the --version command. | ||
// https://www.terraform.io/docs/commands/environment-variables.html#tf_cli_args-and-tf_cli_args_name | ||
|
||
for key := range terragruntOptionsCopy.Env { | ||
if strings.HasPrefix(key, "TF_CLI_ARGS") { | ||
delete(terragruntOptionsCopy.Env, key) | ||
|
@@ -108,6 +132,9 @@ func PopulateTerraformVersion(ctx context.Context, terragruntOptions *options.Te | |
return err | ||
} | ||
|
||
// Save output to cache | ||
versionCache.Put(ctx, cacheKey, output.Stdout.String()) | ||
|
||
terraformVersion, err := ParseTerraformVersion(output.Stdout.String()) | ||
if err != nil { | ||
return err | ||
|
@@ -212,6 +239,29 @@ func parseTerraformImplementationType(versionCommandOutput string) (options.Terr | |
} | ||
} | ||
|
||
// Helper to compute a cache key from the checksums of .terraform-version and .tool-versions | ||
func computeVersionFilesCacheKey(workingDir string) string { | ||
var hashes []string | ||
|
||
for _, file := range versionFiles { | ||
path := filepath.Join(workingDir, file) | ||
if util.FileExists(path) { | ||
hash, err := util.FileSHA256(path) | ||
if err == nil { | ||
hashes = append(hashes, file+":"+hex.EncodeToString(hash)) | ||
} | ||
} | ||
} | ||
|
||
cacheKey := "no-version-files" | ||
|
||
if len(hashes) != 0 { | ||
cacheKey = strings.Join(hashes, "|") | ||
} | ||
|
||
return util.EncodeBase64Sha1(cacheKey) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
} | ||
|
||
// Custom error types | ||
|
||
type InvalidTerraformVersionSyntax string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
variable "input_value" {} | ||
|
||
output "output_value" { | ||
value = var.input_value | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
|
||
dependency "dependency" { | ||
config_path = "../dependency" | ||
} | ||
|
||
dependency "dependency-with-custom-version" { | ||
config_path = "../dependency-with-custom-version" | ||
} | ||
|
||
inputs = { | ||
input_value = dependency.dependency.outputs.result | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
tofu 1.9.4 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
output "result" { | ||
|
||
value = "42" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
output "result" { | ||
|
||
value = "42" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ import ( | |
|
||
"github.com/gruntwork-io/terragrunt/cli/commands/common" | ||
"github.com/gruntwork-io/terragrunt/cli/commands/common/runall" | ||
print "github.com/gruntwork-io/terragrunt/cli/commands/info/print" | ||
"github.com/gruntwork-io/terragrunt/cli/commands/info/print" | ||
"github.com/gruntwork-io/terragrunt/cli/commands/run" | ||
"github.com/gruntwork-io/terragrunt/cli/flags" | ||
"github.com/gruntwork-io/terragrunt/codegen" | ||
|
@@ -113,6 +113,7 @@ const ( | |
testFixtureEphemeralInputs = "fixtures/ephemeral-inputs" | ||
testFixtureTfPath = "fixtures/tf-path" | ||
testFixtureTraceParent = "fixtures/trace-parent" | ||
testFixtureVersionInvocation = "fixtures/version-invocation" | ||
|
||
terraformFolder = ".terraform" | ||
|
||
|
@@ -1958,7 +1959,7 @@ func TestDependencyMockOutputRestricted(t *testing.T) { | |
// Verify that validate-all works as well. | ||
showStdout.Reset() | ||
showStderr.Reset() | ||
err = helpers.RunTerragruntCommand(t, "terragrunt validate-all --non-interactive --working-dir "+dependent2Path, &showStdout, &showStderr) | ||
err = helpers.RunTerragruntCommand(t, "terragrunt validate-all --non-interactive --working-dir "+rootPath, &showStdout, &showStderr) | ||
require.NoError(t, err) | ||
|
||
helpers.LogBufferContentsLineByLine(t, showStdout, "show stdout") | ||
|
@@ -4136,3 +4137,37 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should include a version where at least two fire from |
||
t.Parallel() | ||
|
||
tmpEnvPath := helpers.CopyEnvironment(t, testFixtureDependencyOutput) | ||
helpers.CleanupTerraformFolder(t, tmpEnvPath) | ||
testPath := util.JoinPath(tmpEnvPath, testFixtureDependencyOutput) | ||
|
||
_, stderr, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --all --log-level trace --non-interactive --working-dir "+testPath+" -- apply") | ||
require.NoError(t, err) | ||
|
||
// check that version command was invoked only once -version | ||
versionCmdPattern := regexp.MustCompile(`Running command: ` + regexp.QuoteMeta(wrappedBinary()) + ` -version`) | ||
matches := versionCmdPattern.FindAllStringIndex(stderr, -1) | ||
|
||
assert.Len(t, matches, 1, "Expected exactly one occurrence of '-version' command, found %d", len(matches)) | ||
} | ||
|
||
func TestVersionIsInvokedInDifferentDirectory(t *testing.T) { | ||
t.Parallel() | ||
|
||
tmpEnvPath := helpers.CopyEnvironment(t, testFixtureVersionInvocation) | ||
helpers.CleanupTerraformFolder(t, tmpEnvPath) | ||
testPath := util.JoinPath(tmpEnvPath, testFixtureVersionInvocation) | ||
|
||
_, stderr, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt run --all --log-level trace --non-interactive --working-dir "+testPath+" -- apply") | ||
require.NoError(t, err) | ||
|
||
versionCmdPattern := regexp.MustCompile(`Running command: ` + regexp.QuoteMeta(wrappedBinary()) + ` -version`) | ||
matches := versionCmdPattern.FindAllStringIndex(stderr, -1) | ||
|
||
assert.Len(t, matches, 2, "Expected exactly one occurrence of '-version' command, found %d", len(matches)) | ||
assert.Contains(t, stderr, "prefix=dependency-with-custom-version msg=Running command: "+wrappedBinary()+" -version") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will raise another ticket for this