Skip to content

Commit

Permalink
added support for variables with double underscores
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewnester committed Jan 22, 2025
1 parent f0e8b7e commit 3116d64
Show file tree
Hide file tree
Showing 10 changed files with 129 additions and 48 deletions.
6 changes: 1 addition & 5 deletions acceptance/bundle/variables/double_underscore/output.txt
Original file line number Diff line number Diff line change
@@ -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!
35 changes: 35 additions & 0 deletions integration/bundle/basic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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"),
)
}
Original file line number Diff line number Diff line change
@@ -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": ""
}
}
}
Original file line number Diff line number Diff line change
@@ -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}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
print("Hello World!")
Original file line number Diff line number Diff line change
@@ -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!
Original file line number Diff line number Diff line change
@@ -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!
35 changes: 18 additions & 17 deletions libs/dyn/dynvar/ref.go
Original file line number Diff line number Diff line change
Expand Up @@ -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].
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
28 changes: 8 additions & 20 deletions libs/dyn/dynvar/ref_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand All @@ -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)
}
}

Expand Down
8 changes: 2 additions & 6 deletions libs/dyn/dynvar/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit 3116d64

Please sign in to comment.