steve/databricks-cli
steve
/
databricks-cli
mirror of https://github.com/databricks/cli.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Change SetVariables mutator to mutate dynamic configuration instead (#1573) 

## Changes
Previously `SetVariables` mutator mutated typed configuration by using
`v.Set` for variables. This lead to variables `value` field not having
location information.

By using dynamic configuration mutation, we keep the same functionality
but also preserve location information for value when it's set from
default.

Fixes #1568 #1538

## Tests
Added unit tests
This commit is contained in:
Andrew Nester 2024-07-09 13:12:42 +02:00 committed by GitHub
parent 4d13c7fbe3
commit 8b468b423f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 143 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
bundle/config/mutator/set_variables.go
View File

@ -2,10 +2,12 @@ package mutator
import (
"context"
"fmt"
"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/variable"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/dyn"
"github.com/databricks/cli/libs/env"
)
@ -21,52 +23,63 @@ func (m *setVariables) Name() string {
return "SetVariables"
}
func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Diagnostics {
func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, name string) (dyn.Value, error) {
// case: variable already has value initialized, so skip
if v.HasValue() {
return nil
if variable.HasValue() {
return v, nil
}
// case: read and set variable value from process environment
envVarName := bundleVarPrefix + name
if val, ok := env.Lookup(ctx, envVarName); ok {
if v.IsComplex() {
return diag.Errorf(`setting via environment variables (%s) is not supported for complex variable %s`, envVarName, name)
if variable.IsComplex() {
return dyn.InvalidValue, fmt.Errorf(`setting via environment variables (%s) is not supported for complex variable %s`, envVarName, name)
}
err := v.Set(val)
v, err := dyn.Set(v, "value", dyn.V(val))
if err != nil {
return diag.Errorf(`failed to assign value "%s" to variable %s from environment variable %s with error: %v`, val, name, envVarName, err)
return dyn.InvalidValue, fmt.Errorf(`failed to assign value "%s" to variable %s from environment variable %s with error: %v`, val, name, envVarName, err)
}
return nil
return v, 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
if variable.Lookup != nil {
return v, nil
}
// case: Set the variable to its default value
if v.HasDefault() {
err := v.Set(v.Default)
if variable.HasDefault() {
vDefault, err := dyn.Get(v, "default")
if err != nil {
return diag.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, v.Default, name, err)
return dyn.InvalidValue, fmt.Errorf(`failed to get default value from config "%s" for variable %s with error: %v`, variable.Default, name, err)
}
return nil
v, err := dyn.Set(v, "value", vDefault)
if err != nil {
return dyn.InvalidValue, fmt.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, variable.Default, name, err)
}
return v, 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)
return dyn.InvalidValue, fmt.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)
}
func (m *setVariables) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
var diags diag.Diagnostics
for name, variable := range b.Config.Variables {
diags = diags.Extend(setVariable(ctx, variable, name))
if diags.HasError() {
return diags
}
}
return diags
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
return dyn.Map(v, "variables", dyn.Foreach(func(p dyn.Path, variable dyn.Value) (dyn.Value, error) {
name := p[1].Key()
v, ok := b.Config.Variables[name]
if !ok {
return dyn.InvalidValue, fmt.Errorf(`variable "%s" is not defined`, name)
}
return setVariable(ctx, variable, v, name)
}))
})
return diag.FromErr(err)
}

55
bundle/config/mutator/set_variables_test.go
View File

@ -7,6 +7,8 @@ import (
"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/config/variable"
"github.com/databricks/cli/libs/dyn"
"github.com/databricks/cli/libs/dyn/convert"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@ -20,9 +22,14 @@ func TestSetVariableFromProcessEnvVar(t *testing.T) {
// set value for variable as an environment variable
t.Setenv("BUNDLE_VAR_foo", "process-env")
v, err := convert.FromTyped(variable, dyn.NilValue)
require.NoError(t, err)
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
v, err = setVariable(context.Background(), v, &variable, "foo")
require.NoError(t, err)
err = convert.ToTyped(&variable, v)
require.NoError(t, err)
assert.Equal(t, variable.Value, "process-env")
}
@ -33,8 +40,14 @@ func TestSetVariableUsingDefaultValue(t *testing.T) {
Default: defaultVal,
}
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
v, err := convert.FromTyped(variable, dyn.NilValue)
require.NoError(t, err)
v, err = setVariable(context.Background(), v, &variable, "foo")
require.NoError(t, err)
err = convert.ToTyped(&variable, v)
require.NoError(t, err)
assert.Equal(t, variable.Value, "default")
}
@ -49,8 +62,14 @@ func TestSetVariableWhenAlreadyAValueIsAssigned(t *testing.T) {
// since a value is already assigned to the variable, it would not be overridden
// by the default value
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
v, err := convert.FromTyped(variable, dyn.NilValue)
require.NoError(t, err)
v, err = setVariable(context.Background(), v, &variable, "foo")
require.NoError(t, err)
err = convert.ToTyped(&variable, v)
require.NoError(t, err)
assert.Equal(t, variable.Value, "assigned-value")
}
@ -68,8 +87,14 @@ func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
// since a value is already assigned to the variable, it would not be overridden
// by the value from environment
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
v, err := convert.FromTyped(variable, dyn.NilValue)
require.NoError(t, err)
v, err = setVariable(context.Background(), v, &variable, "foo")
require.NoError(t, err)
err = convert.ToTyped(&variable, v)
require.NoError(t, err)
assert.Equal(t, variable.Value, "assigned-value")
}
@ -79,8 +104,11 @@ func TestSetVariablesErrorsIfAValueCouldNotBeResolved(t *testing.T) {
}
// fails because we could not resolve a value for the variable
diags := setVariable(context.Background(), &variable, "foo")
assert.ErrorContains(t, diags.Error(), "no value assigned to required variable foo. Assignment can be done through the \"--var\" flag or by setting the BUNDLE_VAR_foo environment variable")
v, err := convert.FromTyped(variable, dyn.NilValue)
require.NoError(t, err)
_, err = setVariable(context.Background(), v, &variable, "foo")
assert.ErrorContains(t, err, "no value assigned to required variable foo. Assignment can be done through the \"--var\" flag or by setting the BUNDLE_VAR_foo environment variable")
}
func TestSetVariablesMutator(t *testing.T) {
@ -126,6 +154,9 @@ func TestSetComplexVariablesViaEnvVariablesIsNotAllowed(t *testing.T) {
// set value for variable as an environment variable
t.Setenv("BUNDLE_VAR_foo", "process-env")
diags := setVariable(context.Background(), &variable, "foo")
assert.ErrorContains(t, diags.Error(), "setting via environment variables (BUNDLE_VAR_foo) is not supported for complex variable foo")
v, err := convert.FromTyped(variable, dyn.NilValue)
require.NoError(t, err)
_, err = setVariable(context.Background(), v, &variable, "foo")
assert.ErrorContains(t, err, "setting via environment variables (BUNDLE_VAR_foo) is not supported for complex variable foo")
}

64
bundle/config/mutator/translate_paths_test.go
View File

@ -11,7 +11,10 @@ import (
"github.com/databricks/cli/bundle/config"
"github.com/databricks/cli/bundle/config/mutator"
"github.com/databricks/cli/bundle/config/resources"
"github.com/databricks/cli/bundle/config/variable"
"github.com/databricks/cli/bundle/internal/bundletest"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/dyn"
"github.com/databricks/cli/libs/vfs"
"github.com/databricks/databricks-sdk-go/service/compute"
"github.com/databricks/databricks-sdk-go/service/jobs"
@ -708,3 +711,64 @@ func TestTranslatePathJobEnvironments(t *testing.T) {
assert.Equal(t, "simplejson", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[2])
assert.Equal(t, "/Workspace/Users/foo@bar.com/test.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[3])
}
func TestTranslatePathWithComplexVariables(t *testing.T) {
dir := t.TempDir()
b := &bundle.Bundle{
RootPath: dir,
BundleRoot: vfs.MustNew(dir),
Config: config.Root{
Variables: map[string]*variable.Variable{
"cluster_libraries": {
Type: variable.VariableTypeComplex,
Default: [](map[string]string){
{
"whl": "./local/whl.whl",
},
},
},
},
Resources: config.Resources{
Jobs: map[string]*resources.Job{
"job": {
JobSettings: &jobs.JobSettings{
Tasks: []jobs.Task{
{
TaskKey: "test",
},
},
},
},
},
},
},
}
bundletest.SetLocation(b, "variables", filepath.Join(dir, "variables/variables.yml"))
bundletest.SetLocation(b, "resources.jobs", filepath.Join(dir, "job/resource.yml"))
ctx := context.Background()
// Assign the variables to the dynamic configuration.
diags := bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
p := dyn.MustPathFromString("resources.jobs.job.tasks[0]")
return dyn.SetByPath(v, p.Append(dyn.Key("libraries")), dyn.V("${var.cluster_libraries}"))
})
return diag.FromErr(err)
})
require.NoError(t, diags.Error())
diags = bundle.Apply(ctx, b,
bundle.Seq(
mutator.SetVariables(),
mutator.ResolveVariableReferences("variables"),
mutator.TranslatePaths(),
))
require.NoError(t, diags.Error())
assert.Equal(
t,
filepath.Join("variables", "local", "whl.whl"),
b.Config.Resources.Jobs["job"].Tasks[0].Libraries[0].Whl,
)
}