diff --git a/acceptance/bundle/state/bad_env/output.txt b/acceptance/bundle/state/bad_env/output.txt index a77cd778d0..5a88b62f95 100644 --- a/acceptance/bundle/state/bad_env/output.txt +++ b/acceptance/bundle/state/bad_env/output.txt @@ -10,8 +10,3 @@ Error: unexpected setting for DATABRICKS_BUNDLE_ENGINE="abc" (expected 'terrafor >>> musterr [CLI] bundle destroy --auto-approve Error: unexpected setting for DATABRICKS_BUNDLE_ENGINE="abc" (expected 'terraform' or 'direct') - ->>> musterr [CLI] bundle validate -Error: unexpected setting for DATABRICKS_BUNDLE_ENGINE="abc" (expected 'terraform' or 'direct') - -Found 1 error diff --git a/acceptance/bundle/state/bad_env/script b/acceptance/bundle/state/bad_env/script index 5f27e956d6..fce3a3a0f8 100644 --- a/acceptance/bundle/state/bad_env/script +++ b/acceptance/bundle/state/bad_env/script @@ -3,4 +3,3 @@ trace musterr $CLI bundle plan trace musterr $CLI bundle deploy trace musterr $CLI bundle summary trace musterr $CLI bundle destroy --auto-approve -trace musterr $CLI bundle validate diff --git a/acceptance/bundle/state/engine_mismatch/output.txt b/acceptance/bundle/state/engine_mismatch/output.txt index fba64dfff1..365f4ef27a 100644 --- a/acceptance/bundle/state/engine_mismatch/output.txt +++ b/acceptance/bundle/state/engine_mismatch/output.txt @@ -9,22 +9,12 @@ Available state files: [TEST_TMP_DIR]/.databricks/bundle/default/terraform/terraform.tfstate: local terraform state serial=1 lineage="test-lineage" -=== Env var confirms config, both mismatch state: warning expected +=== Invalid env var with config set: no error, config takes priority ->>> DATABRICKS_BUNDLE_ENGINE=direct [CLI] bundle debug states -Warning: Deployment engine "direct" configured in DATABRICKS_BUNDLE_ENGINE environment variable does not match the existing state (engine "terraform"). Using "terraform" engine from the existing state. +>>> DATABRICKS_BUNDLE_ENGINE=bla [CLI] bundle debug states +Warning: Deployment engine "direct" configured in bundle.engine setting at [TEST_TMP_DIR]/databricks.yml:3:11 does not match the existing state (engine "terraform"). Using "terraform" engine from the existing state. Available state files: - [TEST_TMP_DIR]/.databricks/bundle/default/terraform/terraform.tfstate: local terraform state serial=1 lineage="test-lineage" [TEST_TMP_DIR]/.databricks/bundle/default/terraform/terraform.tfstate: local terraform state serial=1 lineage="test-lineage" - -=== Env var overrides config to match state: no warning expected - ->>> DATABRICKS_BUNDLE_ENGINE=terraform [CLI] bundle debug states -[TEST_TMP_DIR]/.databricks/bundle/default/terraform/terraform.tfstate: local terraform state serial=1 lineage="test-lineage" - -=== Invalid env var: error expected - ->>> musterr [CLI] bundle debug states -Error: unexpected setting for DATABRICKS_BUNDLE_ENGINE="bla" (expected 'terraform' or 'direct') diff --git a/acceptance/bundle/state/engine_mismatch/script b/acceptance/bundle/state/engine_mismatch/script index 762bc16426..4486677247 100644 --- a/acceptance/bundle/state/engine_mismatch/script +++ b/acceptance/bundle/state/engine_mismatch/script @@ -5,12 +5,5 @@ title 'Config mismatch with state: warning expected\n' unset DATABRICKS_BUNDLE_ENGINE trace $CLI bundle debug states -title 'Env var confirms config, both mismatch state: warning expected\n' -trace DATABRICKS_BUNDLE_ENGINE=direct $CLI bundle debug states - -title 'Env var overrides config to match state: no warning expected\n' -trace DATABRICKS_BUNDLE_ENGINE=terraform $CLI bundle debug states - -title 'Invalid env var: error expected\n' -export DATABRICKS_BUNDLE_ENGINE=bla -trace musterr $CLI bundle debug states +title 'Invalid env var with config set: no error, config takes priority\n' +trace DATABRICKS_BUNDLE_ENGINE=bla $CLI bundle debug states diff --git a/bundle/config/engine/engine.go b/bundle/config/engine/engine.go index a24443d579..c2c251e8f9 100644 --- a/bundle/config/engine/engine.go +++ b/bundle/config/engine/engine.go @@ -46,21 +46,11 @@ func FromEnv(ctx context.Context) (EngineType, error) { // EngineSetting represents a requested engine type along with the source of the request. type EngineSetting struct { - Type EngineType // effective: env var if set, else config + Type EngineType // effective resolved engine Source string // human-readable source of Type ConfigType EngineType // from bundle config (EngineNotSet if not configured) } -// SettingFromEnv returns an EngineSetting from the environment variable. -// ConfigType is left as EngineNotSet and populated later by ResolveEngineSetting. -func SettingFromEnv(ctx context.Context) (EngineSetting, error) { - e, err := FromEnv(ctx) - if err != nil { - return EngineSetting{}, err - } - return EngineSetting{Type: e, Source: EnvVar + " environment variable"}, nil -} - func (e EngineType) ThisOrDefault() EngineType { if e == EngineNotSet { return Default diff --git a/bundle/config/engine/engine_test.go b/bundle/config/engine/engine_test.go index cf7c409703..03d6608e34 100644 --- a/bundle/config/engine/engine_test.go +++ b/bundle/config/engine/engine_test.go @@ -8,23 +8,21 @@ import ( "github.com/stretchr/testify/require" ) -func TestSettingFromEnv(t *testing.T) { - ctx := t.Context() - ctx = env.Set(ctx, EnvVar, "direct") - req, err := SettingFromEnv(ctx) +func TestFromEnv(t *testing.T) { + ctx := env.Set(t.Context(), EnvVar, "direct") + e, err := FromEnv(ctx) require.NoError(t, err) - assert.Equal(t, EngineDirect, req.Type) - assert.Contains(t, req.Source, EnvVar) + assert.Equal(t, EngineDirect, e) } -func TestSettingFromEnvNotSet(t *testing.T) { - req, err := SettingFromEnv(t.Context()) +func TestFromEnvNotSet(t *testing.T) { + e, err := FromEnv(t.Context()) require.NoError(t, err) - assert.Equal(t, EngineNotSet, req.Type) + assert.Equal(t, EngineNotSet, e) } -func TestSettingFromEnvInvalid(t *testing.T) { +func TestFromEnvInvalid(t *testing.T) { ctx := env.Set(t.Context(), EnvVar, "invalid") - _, err := SettingFromEnv(ctx) + _, err := FromEnv(ctx) assert.Error(t, err) } diff --git a/bundle/internal/schema/annotations.yml b/bundle/internal/schema/annotations.yml index 97b17eb8a7..5d90fbebba 100644 --- a/bundle/internal/schema/annotations.yml +++ b/bundle/internal/schema/annotations.yml @@ -44,7 +44,7 @@ github.com/databricks/cli/bundle/config.Bundle: The definition of the bundle deployment. For supported attributes see [\_](/dev-tools/bundles/deployment-modes.md). "engine": "description": |- - The deployment engine to use. Valid values are `terraform` and `direct`. Can be overridden with the `DATABRICKS_BUNDLE_ENGINE` environment variable. + The deployment engine to use. Valid values are `terraform` and `direct`. Takes priority over `DATABRICKS_BUNDLE_ENGINE` environment variable. Default is "terraform". "git": "description": |- The Git version control details that are associated with your bundle. diff --git a/bundle/schema/jsonschema.json b/bundle/schema/jsonschema.json index 79090e5be5..e398ecf2b3 100644 --- a/bundle/schema/jsonschema.json +++ b/bundle/schema/jsonschema.json @@ -2159,7 +2159,7 @@ "markdownDescription": "The definition of the bundle deployment. For supported attributes see [link](https://docs.databricks.com/dev-tools/bundles/deployment-modes.html)." }, "engine": { - "description": "The deployment engine to use. Valid values are `terraform` and `direct`. Can be overridden with the `DATABRICKS_BUNDLE_ENGINE` environment variable.", + "description": "The deployment engine to use. Valid values are `terraform` and `direct`. Takes priority over `DATABRICKS_BUNDLE_ENGINE` environment variable. Default is \"terraform\".", "$ref": "#/$defs/github.com/databricks/cli/bundle/config/engine.EngineType" }, "git": { diff --git a/bundle/schema/jsonschema_for_docs.json b/bundle/schema/jsonschema_for_docs.json index 4ce01550cd..10a58f37b0 100644 --- a/bundle/schema/jsonschema_for_docs.json +++ b/bundle/schema/jsonschema_for_docs.json @@ -2151,7 +2151,7 @@ "x-since-version": "v0.229.0" }, "engine": { - "description": "The deployment engine to use. Valid values are `terraform` and `direct`. Can be overridden with the `DATABRICKS_BUNDLE_ENGINE` environment variable.", + "description": "The deployment engine to use. Valid values are `terraform` and `direct`. Takes priority over `DATABRICKS_BUNDLE_ENGINE` environment variable. Default is \"terraform\".", "$ref": "#/$defs/github.com/databricks/cli/bundle/config/engine.EngineType" }, "git": { diff --git a/cmd/bundle/generate/dashboard.go b/cmd/bundle/generate/dashboard.go index 8b95e63ee6..ce5902cd8e 100644 --- a/cmd/bundle/generate/dashboard.go +++ b/cmd/bundle/generate/dashboard.go @@ -14,7 +14,6 @@ import ( "time" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/engine" "github.com/databricks/cli/bundle/generate" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/resources" @@ -374,18 +373,16 @@ func (d *dashboard) initialize(ctx context.Context, b *bundle.Bundle) { } func (d *dashboard) runForResource(ctx context.Context, b *bundle.Bundle) { - envEngine, err := engine.SettingFromEnv(ctx) - if err != nil { - logdiag.LogError(ctx, err) - return - } - phases.Initialize(ctx, b) if logdiag.HasError(ctx) { return } - requiredEngine := utils.ResolveEngineSetting(b, envEngine) + requiredEngine, err := utils.ResolveEngineSetting(ctx, b) + if err != nil { + logdiag.LogError(ctx, err) + return + } ctx, stateDesc := statemgmt.PullResourcesState(ctx, b, statemgmt.AlwaysPull(true), requiredEngine) if logdiag.HasError(ctx) { return diff --git a/cmd/bundle/run.go b/cmd/bundle/run.go index a9145461c8..ec4d894b1f 100644 --- a/cmd/bundle/run.go +++ b/cmd/bundle/run.go @@ -8,7 +8,6 @@ import ( "os" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/engine" "github.com/databricks/cli/bundle/env" "github.com/databricks/cli/bundle/phases" "github.com/databricks/cli/bundle/resources" @@ -133,11 +132,6 @@ Example usage: cmd.Flags().BoolVar(&restart, "restart", false, "Restart the run if it is already running.") cmd.RunE = func(cmd *cobra.Command, args []string) error { - envEngine, err := engine.SettingFromEnv(cmd.Context()) - if err != nil { - return err - } - b, err := utils.ProcessBundle(cmd, utils.ProcessOptions{ SkipInitialize: true, }) @@ -172,7 +166,10 @@ Example usage: return executeScript(content, cmd, b) } - requiredEngine := utils.ResolveEngineSetting(b, envEngine) + requiredEngine, err := utils.ResolveEngineSetting(ctx, b) + if err != nil { + return err + } ctx, stateDesc := statemgmt.PullResourcesState(ctx, b, statemgmt.AlwaysPull(true), requiredEngine) if logdiag.HasError(ctx) { return root.ErrAlreadyPrinted diff --git a/cmd/bundle/utils/process.go b/cmd/bundle/utils/process.go index a09db23c6c..8390548912 100644 --- a/cmd/bundle/utils/process.go +++ b/cmd/bundle/utils/process.go @@ -92,11 +92,6 @@ func ProcessBundleRet(cmd *cobra.Command, opts ProcessOptions) (*bundle.Bundle, cmd.SetContext(ctx) } - envEngine, err := engine.SettingFromEnv(ctx) - if err != nil { - return nil, nil, err - } - // Load bundle config and apply target b := root.MustConfigureBundle(cmd) if logdiag.HasError(ctx) { @@ -157,7 +152,10 @@ func ProcessBundleRet(cmd *cobra.Command, opts ProcessOptions) (*bundle.Bundle, shouldReadState := opts.ReadState || opts.AlwaysPull || opts.InitIDs || opts.ErrorOnEmptyState || opts.PreDeployChecks || opts.Deploy || opts.ReadPlanPath != "" if shouldReadState { - requiredEngine := ResolveEngineSetting(b, envEngine) + requiredEngine, err := ResolveEngineSetting(ctx, b) + if err != nil { + return b, nil, err + } // PullResourcesState depends on stateFiler which needs b.Config.Workspace.StatePath which is set in phases.Initialize ctx, stateDesc = statemgmt.PullResourcesState(ctx, b, statemgmt.AlwaysPull(opts.AlwaysPull), requiredEngine) @@ -299,13 +297,11 @@ func ProcessBundleRet(cmd *cobra.Command, opts ProcessOptions) (*bundle.Bundle, return b, stateDesc, nil } -// ResolveEngineSetting combines the env var engine with the bundle config engine setting. -// Environment variable takes priority over config. ConfigType always reflects the config value. -func ResolveEngineSetting(b *bundle.Bundle, envSetting engine.EngineSetting) engine.EngineSetting { +// ResolveEngineSetting determines the effective engine setting by combining bundle config and env var. +// Priority: bundle.engine config > DATABRICKS_BUNDLE_ENGINE env var. +func ResolveEngineSetting(ctx context.Context, b *bundle.Bundle) (engine.EngineSetting, error) { configEngine := b.Config.Bundle.Engine - if envSetting.Type != engine.EngineNotSet { - return engine.EngineSetting{Type: envSetting.Type, Source: envSetting.Source, ConfigType: configEngine} - } + if configEngine != engine.EngineNotSet { source := "bundle.engine setting" v := dyn.GetValue(b.Config.Value(), "bundle.engine") @@ -313,9 +309,18 @@ func ResolveEngineSetting(b *bundle.Bundle, envSetting engine.EngineSetting) eng loc := locs[0] source = fmt.Sprintf("bundle.engine setting at %s:%d:%d", filepath.ToSlash(loc.File), loc.Line, loc.Column) } - return engine.EngineSetting{Type: configEngine, Source: source, ConfigType: configEngine} + return engine.EngineSetting{Type: configEngine, Source: source, ConfigType: configEngine}, nil + } + + envEngine, err := engine.FromEnv(ctx) + if err != nil { + return engine.EngineSetting{}, err + } + if envEngine != engine.EngineNotSet { + return engine.EngineSetting{Type: envEngine, Source: engine.EnvVar + " environment variable"}, nil } - return engine.EngineSetting{} + + return engine.EngineSetting{}, nil } func rejectDefinitions(ctx context.Context, b *bundle.Bundle) { diff --git a/cmd/bundle/utils/resolve_engine_test.go b/cmd/bundle/utils/resolve_engine_test.go new file mode 100644 index 0000000000..feca507433 --- /dev/null +++ b/cmd/bundle/utils/resolve_engine_test.go @@ -0,0 +1,52 @@ +package utils + +import ( + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/cli/bundle/config/engine" + "github.com/databricks/cli/libs/env" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveEngineSettingConfigTakesPriority(t *testing.T) { + ctx := env.Set(t.Context(), engine.EnvVar, "terraform") + b := &bundle.Bundle{Config: config.Root{Bundle: config.Bundle{Engine: engine.EngineDirect}}} + result, err := ResolveEngineSetting(ctx, b) + require.NoError(t, err) + assert.Equal(t, engine.EngineDirect, result.Type) + assert.Equal(t, engine.EngineDirect, result.ConfigType) +} + +func TestResolveEngineSettingEnvVarUsedWhenNoConfig(t *testing.T) { + ctx := env.Set(t.Context(), engine.EnvVar, "direct") + b := &bundle.Bundle{Config: config.Root{}} + result, err := ResolveEngineSetting(ctx, b) + require.NoError(t, err) + assert.Equal(t, engine.EngineDirect, result.Type) + assert.Contains(t, result.Source, engine.EnvVar) +} + +func TestResolveEngineSettingNothingSet(t *testing.T) { + b := &bundle.Bundle{Config: config.Root{}} + result, err := ResolveEngineSetting(t.Context(), b) + require.NoError(t, err) + assert.Equal(t, engine.EngineNotSet, result.Type) +} + +func TestResolveEngineSettingInvalidEnvVar(t *testing.T) { + ctx := env.Set(t.Context(), engine.EnvVar, "invalid") + b := &bundle.Bundle{Config: config.Root{}} + _, err := ResolveEngineSetting(ctx, b) + assert.Error(t, err) +} + +func TestResolveEngineSettingInvalidEnvVarIgnoredWhenConfigSet(t *testing.T) { + ctx := env.Set(t.Context(), engine.EnvVar, "invalid") + b := &bundle.Bundle{Config: config.Root{Bundle: config.Bundle{Engine: engine.EngineDirect}}} + result, err := ResolveEngineSetting(ctx, b) + require.NoError(t, err) + assert.Equal(t, engine.EngineDirect, result.Type) +}