diff --git a/acceptance/bundle/variables/double_underscore/output.txt b/acceptance/bundle/variables/double_underscore/output.txt index e87ce5345b..8f319c3690 100644 --- a/acceptance/bundle/variables/double_underscore/output.txt +++ b/acceptance/bundle/variables/double_underscore/output.txt @@ -1,13 +1,9 @@ >>> $CLI bundle validate -Error: incorrect variable name: [${var.double__underscore}] - Name: double_underscore Target: default Workspace: User: $USERNAME Path: /Workspace/Users/$USERNAME/.bundle/double_underscore/default -Found 1 error - -Exit code: 1 +Validation OK! diff --git a/integration/bundle/basic_test.go b/integration/bundle/basic_test.go index 79301b850a..250a66b904 100644 --- a/integration/bundle/basic_test.go +++ b/integration/bundle/basic_test.go @@ -6,7 +6,9 @@ import ( "testing" "github.com/databricks/cli/integration/internal/acc" + "github.com/databricks/cli/internal/testcli" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/testdiff" "github.com/google/uuid" "github.com/stretchr/testify/require" ) @@ -35,3 +37,36 @@ func TestBasicBundleDeployWithFailOnActiveRuns(t *testing.T) { // deploy empty bundle again deployBundleWithFlags(t, ctx, root, []string{"--fail-on-active-runs"}) } + +func TestBasicBundleDeployWithDoubleUnderscoreVariables(t *testing.T) { + ctx, _ := acc.WorkspaceTest(t) + + nodeTypeId := testutil.GetCloud(t).NodeTypeID() + uniqueId := uuid.New().String() + root := initTestTemplate(t, ctx, "basic_with_variables", map[string]any{ + "unique_id": uniqueId, + "node_type_id": nodeTypeId, + "spark_version": defaultSparkVersion, + }) + + ctx, replacements := testdiff.WithReplacementsMap(ctx) + replacements.Set(uniqueId, "$UNIQUE_PRJ") + + t.Cleanup(func() { + destroyBundle(t, ctx, root) + }) + + testutil.Chdir(t, root) + testcli.AssertOutput( + t, + ctx, + []string{"bundle", "validate"}, + testutil.TestData("testdata/basic_with_variables/bundle_validate.txt"), + ) + testcli.AssertOutput( + t, + ctx, + []string{"bundle", "deploy", "--force-lock", "--auto-approve"}, + testutil.TestData("testdata/basic_with_variables/bundle_deploy.txt"), + ) +} diff --git a/integration/bundle/bundles/basic_with_variables/databricks_template_schema.json b/integration/bundle/bundles/basic_with_variables/databricks_template_schema.json new file mode 100644 index 0000000000..41a723b0fb --- /dev/null +++ b/integration/bundle/bundles/basic_with_variables/databricks_template_schema.json @@ -0,0 +1,21 @@ +{ + "properties": { + "unique_id": { + "type": "string", + "description": "Unique ID for job name" + }, + "spark_version": { + "type": "string", + "description": "Spark version used for job cluster" + }, + "node_type_id": { + "type": "string", + "description": "Node type id for job cluster" + }, + "root_path": { + "type": "string", + "description": "Root path to deploy bundle to", + "default": "" + } + } +} diff --git a/integration/bundle/bundles/basic_with_variables/template/databricks.yml.tmpl b/integration/bundle/bundles/basic_with_variables/template/databricks.yml.tmpl new file mode 100644 index 0000000000..cb02c9e2f6 --- /dev/null +++ b/integration/bundle/bundles/basic_with_variables/template/databricks.yml.tmpl @@ -0,0 +1,32 @@ +bundle: + name: basic + +workspace: + {{ if .root_path }} + root_path: "{{.root_path}}/.bundle/{{.unique_id}}" + {{ else }} + root_path: "~/.bundle/{{.unique_id}}" + {{ end }} + +variables: + task__key: # Note: the variable has double underscore + default: my_notebook_task + +resources: + jobs: + foo__bar: # Note: the resource has double underscore to check that TF provider can use such names + name: test-job-basic-{{.unique_id}} + tasks: + - task_key: ${var.task__key} + new_cluster: + num_workers: 1 + spark_version: "{{.spark_version}}" + node_type_id: "{{.node_type_id}}" + spark_python_task: + python_file: ./hello_world.py + foo: + name: test-job-basic-ref-{{.unique_id}} + tasks: + - task_key: job_task + run_job_task: + job_id: ${resources.jobs.foo__bar.id} diff --git a/integration/bundle/bundles/basic_with_variables/template/hello_world.py b/integration/bundle/bundles/basic_with_variables/template/hello_world.py new file mode 100644 index 0000000000..f301245e24 --- /dev/null +++ b/integration/bundle/bundles/basic_with_variables/template/hello_world.py @@ -0,0 +1 @@ +print("Hello World!") diff --git a/integration/bundle/testdata/basic_with_variables/bundle_deploy.txt b/integration/bundle/testdata/basic_with_variables/bundle_deploy.txt new file mode 100644 index 0000000000..0490eaa0f0 --- /dev/null +++ b/integration/bundle/testdata/basic_with_variables/bundle_deploy.txt @@ -0,0 +1,4 @@ +Uploading bundle files to /Workspace/Users/5e3ea482-cb45-4da7-b139-b9eef101910e/.bundle/$UNIQUE_PRJ/files... +Deploying resources... +Updating deployment state... +Deployment complete! diff --git a/integration/bundle/testdata/basic_with_variables/bundle_validate.txt b/integration/bundle/testdata/basic_with_variables/bundle_validate.txt new file mode 100644 index 0000000000..6238dabb2d --- /dev/null +++ b/integration/bundle/testdata/basic_with_variables/bundle_validate.txt @@ -0,0 +1,7 @@ +Name: basic +Target: default +Workspace: + User: 5e3ea482-cb45-4da7-b139-b9eef101910e + Path: /Workspace/Users/5e3ea482-cb45-4da7-b139-b9eef101910e/.bundle/$UNIQUE_PRJ + +Validation OK! diff --git a/libs/dyn/dynvar/ref.go b/libs/dyn/dynvar/ref.go index 697dca87a4..11c8d05e64 100644 --- a/libs/dyn/dynvar/ref.go +++ b/libs/dyn/dynvar/ref.go @@ -2,14 +2,12 @@ package dynvar import ( "regexp" + "strings" "github.com/databricks/cli/libs/dyn" ) -var ( - re = regexp.MustCompile(`\$\{([a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\[[0-9]+\])*)*(\[[0-9]+\])*)\}`) - potentialVarRef = regexp.MustCompile(`\$\{[a-zA-Z0-9\.-_]+}`) -) +var re = regexp.MustCompile(`\$\{([a-zA-Z]+([-_]*[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]*[a-zA-Z0-9]+)*(\[[0-9]+\])*)*(\[[0-9]+\])*)\}`) // ref represents a variable reference. // It is a string [dyn.Value] contained in a larger [dyn.Value]. @@ -25,6 +23,8 @@ type ref struct { matches [][]string } +var invalidSeq = []string{"-_", "_-"} + // newRef returns a new ref if the given [dyn.Value] contains a string // with one or more variable references. It returns false if the given // [dyn.Value] does not contain variable references. @@ -33,32 +33,33 @@ type ref struct { // - "${a.b}" // - "${a.b.c}" // - "${a} ${b} ${c}" -func newRef(v dyn.Value) (ref, ref, bool) { +func newRef(v dyn.Value) (ref, bool) { s, ok := v.AsString() if !ok { - return ref{}, ref{}, false + return ref{}, false } // Check if the string contains any variable references. m := re.FindAllStringSubmatch(s, -1) if len(m) == 0 { - // Check if the string contains any potential variable references but they are not valid. - pm := potentialVarRef.FindAllStringSubmatch(s, -1) - if len(pm) > 0 { - return ref{}, ref{ - value: v, - str: s, - matches: pm, - }, false + return ref{}, false + } + + // Check that it does not have invalid sequences such as "-_" or "_-". + + for _, match := range m { + for _, seq := range invalidSeq { + if strings.Contains(match[1], seq) { + return ref{}, false + } } - return ref{}, ref{}, false } return ref{ value: v, str: s, matches: m, - }, ref{}, true + }, true } // isPure returns true if the variable reference contains a single @@ -87,7 +88,7 @@ func IsPureVariableReference(s string) bool { // If s is a pure variable reference, this function returns the corresponding // dyn.Path. Otherwise, it returns false. func PureReferenceToPath(s string) (dyn.Path, bool) { - ref, _, ok := newRef(dyn.V(s)) + ref, ok := newRef(dyn.V(s)) if !ok { return nil, false } diff --git a/libs/dyn/dynvar/ref_test.go b/libs/dyn/dynvar/ref_test.go index 07d168e2e4..96d8b26bbb 100644 --- a/libs/dyn/dynvar/ref_test.go +++ b/libs/dyn/dynvar/ref_test.go @@ -9,17 +9,19 @@ import ( ) func TestNewRefNoString(t *testing.T) { - _, _, ok := newRef(dyn.V(1)) + _, ok := newRef(dyn.V(1)) require.False(t, ok, "should not match non-string") } func TestNewRefValidPattern(t *testing.T) { for in, refs := range map[string][]string{ - "${hello_world.world_world}": {"hello_world.world_world"}, - "${helloworld.world-world}": {"helloworld.world-world"}, - "${hello-world.world-world}": {"hello-world.world-world"}, + "${hello_world.world_world}": {"hello_world.world_world"}, + "${helloworld.world-world}": {"helloworld.world-world"}, + "${hello-world.world-world}": {"hello-world.world-world"}, + "${hello_world.world__world}": {"hello_world.world__world"}, + "${hello_world.world--world}": {"hello_world.world--world"}, } { - ref, _, ok := newRef(dyn.V(in)) + ref, ok := newRef(dyn.V(in)) require.True(t, ok, "should match valid pattern: %s", in) assert.Equal(t, refs, ref.references()) } @@ -37,24 +39,10 @@ func TestNewRefInvalidPattern(t *testing.T) { "${0helloworld.world-world}", // interpolated first section shouldn't start with number "${helloworld.9world-world}", // interpolated second section shouldn't start with number "${a-a.a-_a-a.id}", // fails because of -_ in the second segment - "${a-a.a--a-a.id}", // fails because of -- in the second segment } for _, v := range invalid { - _, pr, ok := newRef(dyn.V(v)) + _, ok := newRef(dyn.V(v)) require.False(t, ok, "should not match invalid pattern: %s", v) - require.Empty(t, pr.matches) - } -} - -func TestNewRefInvalidPatternWithDoubleUnderscore(t *testing.T) { - invalid := []string{ - "${hello__world.world_world}", - "${hello_world.world__world}", - } - for _, v := range invalid { - _, pr, ok := newRef(dyn.V(v)) - require.False(t, ok, "should not match invalid pattern: %s", v) - require.NotEmpty(t, pr.matches) } } diff --git a/libs/dyn/dynvar/resolve.go b/libs/dyn/dynvar/resolve.go index 148a259b0b..111da25c87 100644 --- a/libs/dyn/dynvar/resolve.go +++ b/libs/dyn/dynvar/resolve.go @@ -78,12 +78,8 @@ func (r *resolver) collectVariableReferences() (err error) { // First walk the input to gather all values with a variable reference. _, err = dyn.Walk(r.in, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - ref, potentialRef, ok := newRef(v) + ref, ok := newRef(v) if !ok { - if len(potentialRef.matches) > 0 { - // If the value contains a potential variable reference, we should skip it. - return dyn.InvalidValue, fmt.Errorf("incorrect variable name: %s", potentialRef.matches[0]) - } // Skip values without variable references. return v, nil } @@ -210,7 +206,7 @@ func (r *resolver) resolveKey(key string, seen []string) (dyn.Value, error) { } // If the returned value is a valid variable reference, resolve it. - ref, _, ok := newRef(v) + ref, ok := newRef(v) if ok { v, err = r.resolveRef(ref, seen) }