Override variables with lookup value even if values has default value…

… set (#1504)

## Changes

This PR fixes the behaviour when variables were not overridden with
lookup value from targets if these variables had any default value set
in the default target.

Fixes #1449 

## Tests
Added regression test

bundle/config/mutator/resolve_resource_references_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/bundle/config"
 	"github.com/databricks/cli/bundle/config/variable"
+	"github.com/databricks/cli/libs/env"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
@@ -194,3 +195,37 @@ func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) {
 	diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
 	require.ErrorContains(t, diags.Error(), "lookup variables cannot contain references to another lookup variables")
 }
+
+func TestNoResolveLookupIfVariableSetWithEnvVariable(t *testing.T) {
+	s := func(s string) *string {
+		return &s
+	}
+
+	b := &bundle.Bundle{
+		Config: config.Root{
+			Bundle: config.Bundle{
+				Target: "dev",
+			},
+			Variables: map[string]*variable.Variable{
+				"foo": {
+					Value: s("bar"),
+				},
+				"lookup": {
+					Lookup: &variable.Lookup{
+						Cluster: "cluster-${var.foo}-${bundle.target}",
+					},
+				},
+			},
+		},
+	}
+
+	m := mocks.NewMockWorkspaceClient(t)
+	b.SetWorkpaceClient(m.WorkspaceClient)
+
+	ctx := context.Background()
+	ctx = env.Set(ctx, "BUNDLE_VAR_lookup", "1234-5678-abcd")
+
+	diags := bundle.Apply(ctx, b, bundle.Seq(SetVariables(), ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
+	require.NoError(t, diags.Error())
+	require.Equal(t, "1234-5678-abcd", *b.Config.Variables["lookup"].Value)
+}

bundle/config/mutator/set_variables.go

@@ -37,6 +37,12 @@ func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Di
 		return nil
 	}
 
+	// case: Defined a variable for named lookup for a resource
+	// It will be resolved later in ResolveResourceReferences mutator
+	if v.Lookup != nil {
+		return nil
+	}
+
 	// case: Set the variable to its default value
 	if v.HasDefault() {
 		err := v.Set(*v.Default)
@@ -46,12 +52,6 @@ func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Di
 		return nil
 	}
 
-	// case: Defined a variable for named lookup for a resource
-	// It will be resolved later in ResolveResourceReferences mutator
-	if v.Lookup != nil {
-		return nil
-	}
-
 	// We should have had a value to set for the variable at this point.
 	return diag.Errorf(`no value assigned to required variable %s. Assignment can be done through the "--var" flag or by setting the %s environment variable`, name, bundleVarPrefix+name)
 }

bundle/tests/variables/env_overrides/databricks.yml

@@ -8,14 +8,16 @@ variables:
 
   d:
     description: variable with lookup
-    lookup:
-      cluster: some-cluster
+    default: ""
 
   e:
     description: variable with lookup
-    lookup:
-      instance_pool: some-pool
+    default: "some-value"
 
+  f:
+    description: variable with lookup
+    lookup:
+      cluster_policy: wrong-cluster-policy
 bundle:
   name: test bundle
 
@@ -49,4 +51,7 @@ targets:
       e:
         lookup:
           instance_pool: some-test-instance-pool
+      f:
+        lookup:
+          cluster_policy: some-test-cluster-policy
       b: prod-b

bundle/tests/variables_test.go

@@ -6,7 +6,10 @@ import (
 
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/bundle/config/mutator"
+	"github.com/databricks/databricks-sdk-go/experimental/mocks"
+	"github.com/databricks/databricks-sdk-go/service/compute"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
@@ -112,13 +115,34 @@ func TestVariablesWithoutDefinition(t *testing.T) {
 
 func TestVariablesWithTargetLookupOverrides(t *testing.T) {
 	b := load(t, "./variables/env_overrides")
+
+	mockWorkspaceClient := mocks.NewMockWorkspaceClient(t)
+	b.SetWorkpaceClient(mockWorkspaceClient.WorkspaceClient)
+	instancePoolApi := mockWorkspaceClient.GetMockInstancePoolsAPI()
+	instancePoolApi.EXPECT().GetByInstancePoolName(mock.Anything, "some-test-instance-pool").Return(&compute.InstancePoolAndStats{
+		InstancePoolId: "1234",
+	}, nil)
+
+	clustersApi := mockWorkspaceClient.GetMockClustersAPI()
+	clustersApi.EXPECT().GetByClusterName(mock.Anything, "some-test-cluster").Return(&compute.ClusterDetails{
+		ClusterId: "4321",
+	}, nil)
+
+	clusterPoliciesApi := mockWorkspaceClient.GetMockClusterPoliciesAPI()
+	clusterPoliciesApi.EXPECT().GetByName(mock.Anything, "some-test-cluster-policy").Return(&compute.Policy{
+		PolicyId: "9876",
+	}, nil)
+
 	diags := bundle.Apply(context.Background(), b, bundle.Seq(
 		mutator.SelectTarget("env-overrides-lookup"),
 		mutator.SetVariables(),
+		mutator.ResolveResourceReferences(),
 	))
+
 	require.NoError(t, diags.Error())
-	assert.Equal(t, "cluster: some-test-cluster", b.Config.Variables["d"].Lookup.String())
-	assert.Equal(t, "instance-pool: some-test-instance-pool", b.Config.Variables["e"].Lookup.String())
+	assert.Equal(t, "4321", *b.Config.Variables["d"].Value)
+	assert.Equal(t, "1234", *b.Config.Variables["e"].Value)
+	assert.Equal(t, "9876", *b.Config.Variables["f"].Value)
 }
 
 func TestVariableTargetOverrides(t *testing.T) {