From a43bcc4c63c1dddf8286a675aa4aeaab9203211c Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 13 Nov 2024 09:43:31 +0100 Subject: [PATCH 1/2] Allow overriding compute for non-development mode targets --- bundle/config/mutator/override_compute.go | 18 +++++++++++----- .../config/mutator/override_compute_test.go | 21 ++++++++++++------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/bundle/config/mutator/override_compute.go b/bundle/config/mutator/override_compute.go index 5700cdf2..ab697b42 100644 --- a/bundle/config/mutator/override_compute.go +++ b/bundle/config/mutator/override_compute.go @@ -38,18 +38,26 @@ func overrideJobCompute(j *resources.Job, compute string) { } func (m *overrideCompute) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - if b.Config.Bundle.Mode != config.Development { + var diags diag.Diagnostics + + if b.Config.Bundle.Mode == config.Production { if b.Config.Bundle.ClusterId != "" { - return diag.Errorf("cannot override compute for an target that does not use 'mode: development'") + diags = diags.Extend(diag.Warningf("overriding compute for a target that uses 'mode: production' is not recommended")) + } + if env.Get(ctx, "DATABRICKS_CLUSTER_ID") != "" { + // The DATABRICKS_CLUSTER_ID may be set by accident when doing a production deploy. + // Overriding the cluster in production is almost always a mistake since customers + // want consistency in production and not compute that is different each deploy. + // For this reason we log a warning and ignore the environment variable. + return diag.Warningf("the DATABRICKS_CLUSTER_ID variable is set but is ignored since the current target uses 'mode: production'") } - return nil } if v := env.Get(ctx, "DATABRICKS_CLUSTER_ID"); v != "" { b.Config.Bundle.ClusterId = v } if b.Config.Bundle.ClusterId == "" { - return nil + return diags } r := b.Config.Resources @@ -57,5 +65,5 @@ func (m *overrideCompute) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag overrideJobCompute(r.Jobs[i], b.Config.Bundle.ClusterId) } - return nil + return diags } diff --git a/bundle/config/mutator/override_compute_test.go b/bundle/config/mutator/override_compute_test.go index 369447d7..c9f079cd 100644 --- a/bundle/config/mutator/override_compute_test.go +++ b/bundle/config/mutator/override_compute_test.go @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestOverrideDevelopment(t *testing.T) { +func TestOverrideComputeModeDevelopment(t *testing.T) { t.Setenv("DATABRICKS_CLUSTER_ID", "") b := &bundle.Bundle{ Config: config.Root{ @@ -62,10 +62,13 @@ func TestOverrideDevelopment(t *testing.T) { assert.Empty(t, b.Config.Resources.Jobs["job1"].Tasks[3].JobClusterKey) } -func TestOverrideDevelopmentEnv(t *testing.T) { +func TestOverrideComputeModeDefault(t *testing.T) { t.Setenv("DATABRICKS_CLUSTER_ID", "newClusterId") b := &bundle.Bundle{ Config: config.Root{ + Bundle: config.Bundle{ + Mode: "", + }, Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job1": {JobSettings: &jobs.JobSettings{ @@ -86,11 +89,12 @@ func TestOverrideDevelopmentEnv(t *testing.T) { m := mutator.OverrideCompute() diags := bundle.Apply(context.Background(), b, m) - require.NoError(t, diags.Error()) - assert.Equal(t, "cluster2", b.Config.Resources.Jobs["job1"].Tasks[1].ExistingClusterId) + require.Empty(t, diags) + assert.Equal(t, "newClusterId", b.Config.Resources.Jobs["job1"].Tasks[0].ExistingClusterId) + assert.Equal(t, "newClusterId", b.Config.Resources.Jobs["job1"].Tasks[1].ExistingClusterId) } -func TestOverridePipelineTask(t *testing.T) { +func TestOverrideComputePipelineTask(t *testing.T) { t.Setenv("DATABRICKS_CLUSTER_ID", "newClusterId") b := &bundle.Bundle{ Config: config.Root{ @@ -115,7 +119,7 @@ func TestOverridePipelineTask(t *testing.T) { assert.Empty(t, b.Config.Resources.Jobs["job1"].Tasks[0].ExistingClusterId) } -func TestOverrideForEachTask(t *testing.T) { +func TestOverrideComputeForEachTask(t *testing.T) { t.Setenv("DATABRICKS_CLUSTER_ID", "newClusterId") b := &bundle.Bundle{ Config: config.Root{ @@ -140,10 +144,11 @@ func TestOverrideForEachTask(t *testing.T) { assert.Empty(t, b.Config.Resources.Jobs["job1"].Tasks[0].ForEachTask.Task) } -func TestOverrideProduction(t *testing.T) { +func TesOverrideComputeProductionMode(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Bundle: config.Bundle{ + Mode: config.Production, ClusterId: "newClusterID", }, Resources: config.Resources{ @@ -167,6 +172,8 @@ func TestOverrideProduction(t *testing.T) { m := mutator.OverrideCompute() diags := bundle.Apply(context.Background(), b, m) require.True(t, diags.HasError()) + assert.ErrorContains(t, diags.Error(), "overriding compute is not allowed in production mode") + assert.Equal(t, "cluster2", b.Config.Resources.Jobs["job1"].Tasks[0].ExistingClusterId) } func TestOverrideProductionEnv(t *testing.T) { From c4301b7a44356e5bef14a7c0f6e8d0ae782c252b Mon Sep 17 00:00:00 2001 From: Lennart Kats Date: Wed, 13 Nov 2024 09:49:05 +0100 Subject: [PATCH 2/2] Fix tests --- bundle/config/mutator/override_compute.go | 1 + bundle/config/mutator/override_compute_test.go | 17 +++++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/bundle/config/mutator/override_compute.go b/bundle/config/mutator/override_compute.go index ab697b42..4804e6a3 100644 --- a/bundle/config/mutator/override_compute.go +++ b/bundle/config/mutator/override_compute.go @@ -42,6 +42,7 @@ func (m *overrideCompute) Apply(ctx context.Context, b *bundle.Bundle) diag.Diag if b.Config.Bundle.Mode == config.Production { if b.Config.Bundle.ClusterId != "" { + // Overriding compute via a command-line flag for production works, but is not recommended. diags = diags.Extend(diag.Warningf("overriding compute for a target that uses 'mode: production' is not recommended")) } if env.Get(ctx, "DATABRICKS_CLUSTER_ID") != "" { diff --git a/bundle/config/mutator/override_compute_test.go b/bundle/config/mutator/override_compute_test.go index c9f079cd..47f9af96 100644 --- a/bundle/config/mutator/override_compute_test.go +++ b/bundle/config/mutator/override_compute_test.go @@ -144,7 +144,7 @@ func TestOverrideComputeForEachTask(t *testing.T) { assert.Empty(t, b.Config.Resources.Jobs["job1"].Tasks[0].ForEachTask.Task) } -func TesOverrideComputeProductionMode(t *testing.T) { +func TestOverrideComputeModeProduction(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Bundle: config.Bundle{ @@ -171,15 +171,18 @@ func TesOverrideComputeProductionMode(t *testing.T) { m := mutator.OverrideCompute() diags := bundle.Apply(context.Background(), b, m) - require.True(t, diags.HasError()) - assert.ErrorContains(t, diags.Error(), "overriding compute is not allowed in production mode") - assert.Equal(t, "cluster2", b.Config.Resources.Jobs["job1"].Tasks[0].ExistingClusterId) + require.Len(t, diags, 1) + assert.Equal(t, "overriding compute for a target that uses 'mode: production' is not recommended", diags[0].Summary) + assert.Equal(t, "newClusterID", b.Config.Resources.Jobs["job1"].Tasks[0].ExistingClusterId) } -func TestOverrideProductionEnv(t *testing.T) { +func TestOverrideComputeModeProductionIgnoresVariable(t *testing.T) { t.Setenv("DATABRICKS_CLUSTER_ID", "newClusterId") b := &bundle.Bundle{ Config: config.Root{ + Bundle: config.Bundle{ + Mode: config.Production, + }, Resources: config.Resources{ Jobs: map[string]*resources.Job{ "job1": {JobSettings: &jobs.JobSettings{ @@ -200,5 +203,7 @@ func TestOverrideProductionEnv(t *testing.T) { m := mutator.OverrideCompute() diags := bundle.Apply(context.Background(), b, m) - require.NoError(t, diags.Error()) + require.Len(t, diags, 1) + assert.Equal(t, "the DATABRICKS_CLUSTER_ID variable is set but is ignored since the current target uses 'mode: production'", diags[0].Summary) + assert.Equal(t, "cluster2", b.Config.Resources.Jobs["job1"].Tasks[1].ExistingClusterId) }