diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 9d26552d64..30bdd6ae5a 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -3,10 +3,13 @@ ## Release v0.283.0 ### Notable Changes +* Bundle commands now cache the user's account details to improve command latency. +To disable this, set the environment variable DATABRICKS_CACHE_ENABLED to false. ### CLI ### Bundles +* Enable caching user identity by default ([#4202](https://github.com/databricks/cli/pull/4202)) ### Dependency updates diff --git a/acceptance/bundle/resource_deps/bad_ref_string_to_int/test.toml b/acceptance/bundle/resource_deps/bad_ref_string_to_int/test.toml index 969a5a5cd1..364eb9f840 100644 --- a/acceptance/bundle/resource_deps/bad_ref_string_to_int/test.toml +++ b/acceptance/bundle/resource_deps/bad_ref_string_to_int/test.toml @@ -1,3 +1,6 @@ +[Env] +DATABRICKS_CACHE_ENABLED = 'false' + [[Repls]] Old = 'terraform.tfstate' New = 'resources.json' diff --git a/acceptance/bundle/resource_deps/missing_ingestion_definition/test.toml b/acceptance/bundle/resource_deps/missing_ingestion_definition/test.toml index 2826f398d0..deca119444 100644 --- a/acceptance/bundle/resource_deps/missing_ingestion_definition/test.toml +++ b/acceptance/bundle/resource_deps/missing_ingestion_definition/test.toml @@ -1,5 +1,8 @@ Badness = "In TF error message talks about ingestion_definition being a list which is not the case. In direct error message says 'on invalid' which is confusing, should say about ingestion_definition being nil" +[Env] +DATABRICKS_CACHE_ENABLED = 'false' + [[Repls]] Old = 'terraform.tfstate' New = 'resources.json' diff --git a/acceptance/cache/clear/test.toml b/acceptance/cache/clear/test.toml index 0b1b2fe5e7..e7fe814fc9 100644 --- a/acceptance/cache/clear/test.toml +++ b/acceptance/cache/clear/test.toml @@ -1,9 +1,6 @@ Cloud = false Local = true -[Env] -DATABRICKS_CACHE_ENABLED = 'true' - # Redact structured logging fields from debug output [[Repls]] Old = ' pid=[0-9]+' diff --git a/acceptance/cache/simple/test.toml b/acceptance/cache/simple/test.toml index 07f6a81177..08cabc87be 100644 --- a/acceptance/cache/simple/test.toml +++ b/acceptance/cache/simple/test.toml @@ -3,9 +3,6 @@ Local = true RecordRequests = true -[Env] -DATABRICKS_CACHE_ENABLED = 'true' - # Redact structured logging fields from debug output [[Repls]] Old = ' pid=[0-9]+' diff --git a/bundle/bundle.go b/bundle/bundle.go index 837521f03f..b0c7f40e47 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -151,7 +151,7 @@ type Bundle struct { Tagging tags.Cloud // Cache is used for caching API responses (e.g., current user). - // By default, operates in measurement-only mode. Set DATABRICKS_CACHE_ENABLED=true to enable actual caching. + // By default, caching is enabled. Set DATABRICKS_CACHE_ENABLED=false to disable caching. Cache *cache.Cache Metrics Metrics diff --git a/libs/cache/file_cache.go b/libs/cache/file_cache.go index deb9780b65..44e9c5aba5 100644 --- a/libs/cache/file_cache.go +++ b/libs/cache/file_cache.go @@ -109,12 +109,12 @@ func getCacheBaseDir(ctx context.Context) (string, error) { // NewCache creates a new file-based cache using UserCacheDir() + "databricks" + version + cached component name. // Including the CLI version in the path ensures cache isolation across different CLI versions. -// By default, the cache operates in measurement-only mode (cacheEnabled=false), which means it will: +// By default, caching is enabled. Set DATABRICKS_CACHE_ENABLED to false/0/no/off to disable caching. +// When caching is disabled (measurement-only mode), the cache will: // - Check if cached values exist // - Measure how much time would have been saved // - Emit metrics about potential savings // - Always compute the value (never actually use the cache) -// Set DATABRICKS_CACHE_ENABLED=true to enable actual caching. // The returned cache can handle multiple types through the generic GetOrCompute function. func NewCache(ctx context.Context, component string, expiry time.Duration, metrics Metrics) *Cache { cacheBaseDir, err := getCacheBaseDir(ctx) @@ -132,9 +132,14 @@ func NewCache(ctx context.Context, component string, expiry time.Duration, metri } fc.metrics = metrics - // Check if cache is enabled; default is false (measurement-only mode) - // Only "true" enables caching; any other value (including "false", "1", etc.) keeps it disabled - fc.cacheEnabled = env.Get(ctx, "DATABRICKS_CACHE_ENABLED") == "true" + // Check if cache is enabled + // Default is true (enabled) when DATABRICKS_CACHE_ENABLED is not set + // When set, parse it as a boolean + if enabled, ok := env.GetBool(ctx, "DATABRICKS_CACHE_ENABLED"); ok { + fc.cacheEnabled = enabled + } else { + fc.cacheEnabled = true + } return &Cache{impl: fc} } diff --git a/libs/cache/file_cache_env_test.go b/libs/cache/file_cache_env_test.go index a2e2db4aed..0852385574 100644 --- a/libs/cache/file_cache_env_test.go +++ b/libs/cache/file_cache_env_test.go @@ -2,6 +2,7 @@ package cache import ( "context" + "fmt" "os" "path/filepath" "runtime" @@ -22,43 +23,92 @@ func TestCacheEnabledEnvVar(t *testing.T) { tests := []struct { name string envValue string + setEnv bool expectCached bool }{ { name: "cache enabled with 'true'", envValue: "true", + setEnv: true, + expectCached: true, + }, + { + name: "cache enabled with 'TRUE'", + envValue: "TRUE", + setEnv: true, + expectCached: true, + }, + { + name: "cache enabled with '1'", + envValue: "1", + setEnv: true, + expectCached: true, + }, + { + name: "cache enabled with 'yes'", + envValue: "yes", + setEnv: true, + expectCached: true, + }, + { + name: "cache enabled with 'on'", + envValue: "on", + setEnv: true, expectCached: true, }, { name: "cache disabled with 'false'", envValue: "false", + setEnv: true, expectCached: false, }, { - name: "cache disabled when empty", + name: "cache disabled with 'FALSE'", + envValue: "FALSE", + setEnv: true, + expectCached: false, + }, + { + name: "cache disabled with '0'", + envValue: "0", + setEnv: true, + expectCached: false, + }, + { + name: "cache disabled with 'no'", + envValue: "no", + setEnv: true, + expectCached: false, + }, + { + name: "cache disabled with empty string", envValue: "", + setEnv: true, expectCached: false, }, { name: "cache disabled with invalid value", - envValue: "yes", + envValue: "invalid", + setEnv: true, expectCached: false, }, { - name: "cache disabled with '1'", - envValue: "1", - expectCached: false, + name: "cache enabled by default when not set", + envValue: "", + setEnv: false, + expectCached: true, }, } - for _, tt := range tests { + for i, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Create a unique subdirectory for this test - testDir := filepath.Join(tempDir, tt.name) + // Use index to avoid case-sensitivity issues on macOS + testDir := filepath.Join(tempDir, fmt.Sprintf("test-%d-%s", i, tt.envValue)) // Set up context with environment variable testCtx := ctx - if tt.envValue != "" { + if tt.setEnv { testCtx = env.Set(testCtx, "DATABRICKS_CACHE_ENABLED", tt.envValue) } testCtx = env.Set(testCtx, "DATABRICKS_CACHE_DIR", testDir) diff --git a/libs/env/context.go b/libs/env/context.go index 37b76147a3..3a7ce5284e 100644 --- a/libs/env/context.go +++ b/libs/env/context.go @@ -5,6 +5,7 @@ import ( "errors" "os" "runtime" + "strconv" "strings" ) @@ -55,6 +56,41 @@ func Get(ctx context.Context, key string) string { return v } +// GetBool gets a boolean value from the context or environment. +// Returns (value, true) if the key is set, or (false, false) if not set. +// It accepts various boolean-like values: +// - True: "1", "t", "T", "true", "TRUE", "True", "yes", "YES", "Yes", "on", "ON", "On" +// - False: "0", "f", "F", "false", "FALSE", "False", "no", "NO", "No", "off", "OFF", "Off", "" (empty string) +// Invalid values are treated as false but still return ok=true. +func GetBool(ctx context.Context, key string) (bool, bool) { + v, ok := Lookup(ctx, key) + if !ok { + return false, false + } + + // Empty string is treated as false + if v == "" { + return false, true + } + + // Handle additional boolean-like values not covered by strconv.ParseBool + switch strings.ToLower(v) { + case "yes", "on": + return true, true + case "no", "off": + return false, true + } + + // Use strconv.ParseBool for standard boolean parsing + // It handles: "1", "t", "T", "true", "TRUE", "True" (true) + // and: "0", "f", "F", "false", "FALSE", "False" (false) + b, err := strconv.ParseBool(v) + if err != nil { + return false, true + } + return b, true +} + // Set key on the context. // // Note: this does NOT mutate the processes' actual environment variables. diff --git a/libs/env/context_test.go b/libs/env/context_test.go index 28a8d88053..0886961362 100644 --- a/libs/env/context_test.go +++ b/libs/env/context_test.go @@ -55,3 +55,57 @@ func TestHome(t *testing.T) { assert.Equal(t, "...", home) assert.NoError(t, err) } + +func TestGetBool(t *testing.T) { + testutil.CleanupEnvironment(t) + ctx := context.Background() + + // Test true values + trueValues := []string{"true", "TRUE", "True", "1", "t", "T", "yes", "YES", "Yes", "on", "ON", "On"} + for _, v := range trueValues { + t.Run("true_"+v, func(t *testing.T) { + ctx := Set(ctx, "TEST_BOOL", v) + val, ok := GetBool(ctx, "TEST_BOOL") + assert.True(t, ok, "expected key to be set") + assert.True(t, val, "expected %q to be true", v) + }) + } + + // Test false values + falseValues := []string{"false", "FALSE", "False", "0", "f", "F", "no", "NO", "No", "off", "OFF", "Off", ""} + for _, v := range falseValues { + t.Run("false_"+v, func(t *testing.T) { + ctx := Set(ctx, "TEST_BOOL", v) + val, ok := GetBool(ctx, "TEST_BOOL") + assert.True(t, ok, "expected key to be set") + assert.False(t, val, "expected %q to be false", v) + }) + } + + // Test invalid/unknown values default to false but ok=true + invalidValues := []string{"invalid", "random", "2", "maybe"} + for _, v := range invalidValues { + t.Run("invalid_"+v, func(t *testing.T) { + ctx := Set(ctx, "TEST_BOOL", v) + val, ok := GetBool(ctx, "TEST_BOOL") + assert.True(t, ok, "expected key to be set") + assert.False(t, val, "expected %q to be false (invalid)", v) + }) + } + + // Test missing key returns ok=false + val, ok := GetBool(ctx, "NON_EXISTENT_KEY") + assert.False(t, ok, "expected key to not be set") + assert.False(t, val, "expected value to be false when not set") + + // Test from actual environment variable + t.Setenv("TEST_ENV_BOOL", "true") + val, ok = GetBool(context.Background(), "TEST_ENV_BOOL") + assert.True(t, ok) + assert.True(t, val) + + t.Setenv("TEST_ENV_BOOL_FALSE", "0") + val, ok = GetBool(context.Background(), "TEST_ENV_BOOL_FALSE") + assert.True(t, ok) + assert.False(t, val) +}