Skip to content

Commit d3e5dde

Browse files
denikIsaac
andauthored
bundle: read and edit the typed config in five mutators (#7145)
## Changes Five mutators now read and edit the typed config directly instead of going through `b.Config.Mutate`: - `PrependWorkspacePrefix`: the `workspace.*_path` fields. - `ApplyBundlePermissions`: the `permissions` field of each supported resource. - `ExpandPipelineGlobPaths`: pipeline `libraries`. - `ProcessStaticResources`: lists resource keys from the typed `Resources`. - `ProcessRootIncludes`: writes the expanded include list after the included files are merged, because merging reloads the typed config from the dynamic tree. Split out of #7098. ## Why This is a step toward removing `dyn` from the mutator pipeline. Mutators that edit typed config are simpler and don't need to convert between the two forms. ## Intentional behavior notes - (a) Invalid-permission-level warnings now show their source location (`in databricks.yml:L:C`). Before, `ApplyBundlePermissions` rebuilt the permissions without locations. One golden changed because of this. - (b) `ApplyBundlePermissions` now walks resource names in sorted order. This only changes the order of overlap warnings. - (c) Expanded include items, and pipeline libraries after glob expansion, now get the location of the original entry at the same index. Before, they had no location. - (d) In typed config, a library with both notebook and file and an explicit `notebook.path: ""` looks the same as one with no notebook path. ## Tests - New unit tests for the edge cases: empty and unset permissions/libraries, library order, includes of included files, and resource keys for every resource type. They pass on both the old and the new code. - The full `TestAccept/bundle` suite ran locally. The only golden diff is the one in `validate/secret_scope_invalid_permission_level`. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
1 parent 51afc3b commit d3e5dde

10 files changed

Lines changed: 326 additions & 201 deletions

File tree

‎acceptance/bundle/validate/secret_scope_invalid_permission_level/output.txt‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
>>> [CLI] bundle validate
33
Warning: invalid value "12" for enum field. Valid values are [READ WRITE MANAGE]
44
at resources.secret_scopes.bad_levels.permissions[0].level
5+
in databricks.yml:10:18
56

67
Warning: invalid value "fuzz_badlevel" for enum field. Valid values are [READ WRITE MANAGE]
78
at resources.secret_scopes.bad_levels.permissions[1].level
9+
in databricks.yml:12:18
810

911
Name: test-bundle
1012
Target: default
@@ -17,9 +19,11 @@ Found 2 warnings
1719
>>> [CLI] bundle validate --strict
1820
Warning: invalid value "12" for enum field. Valid values are [READ WRITE MANAGE]
1921
at resources.secret_scopes.bad_levels.permissions[0].level
22+
in databricks.yml:10:18
2023

2124
Warning: invalid value "fuzz_badlevel" for enum field. Valid values are [READ WRITE MANAGE]
2225
at resources.secret_scopes.bad_levels.permissions[1].level
26+
in databricks.yml:12:18
2327

2428
Name: test-bundle
2529
Target: default

‎bundle/config/loader/process_root_includes.go‎

Lines changed: 9 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"github.com/databricks/cli/bundle"
1111
"github.com/databricks/cli/bundle/config"
1212
"github.com/databricks/cli/libs/diag"
13-
"github.com/databricks/cli/libs/dyn"
1413
)
1514

1615
type processRootIncludes struct{}
@@ -42,7 +41,8 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) diag.
4241

4342
// Preserve the raw include patterns before they are replaced below with the
4443
// expanded list of loaded files, so IsFileIncluded can re-match against them.
45-
b.SetIncludePatterns(b.Config.Include)
44+
patterns := b.Config.Include
45+
b.SetIncludePatterns(patterns)
4646

4747
// Map with files we've already seen to avoid loading them twice.
4848
seen := map[string]bool{}
@@ -131,27 +131,6 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) diag.
131131
}
132132
}
133133

134-
// Swap out the original includes list with the expanded globs. This goes through
135-
// Mutate so the dynamic tree is updated too: the includes below are applied without
136-
// their own mutator scope, so nothing converts the typed field back into the dynamic
137-
// tree afterwards, and the next ToTyped would otherwise restore the raw patterns.
138-
err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) {
139-
// Include is omitempty in the typed configuration, so an empty list must stay
140-
// absent from the dynamic tree rather than be written as [].
141-
if len(files) == 0 {
142-
return dyn.DropKeys(root, []string{"include"})
143-
}
144-
145-
includeValues := make([]dyn.Value, 0, len(files))
146-
for _, file := range files {
147-
includeValues = append(includeValues, dyn.V(file))
148-
}
149-
return dyn.Set(root, "include", dyn.NewValue(includeValues, root.Get("include").Locations()))
150-
})
151-
if err != nil {
152-
return diag.FromErr(err)
153-
}
154-
155134
// Track number of bundle YAML (or JSON) files in the configuration. The +1 is there
156135
// to account for the root databricks.yaml file.
157136
b.Metrics.ConfigurationFileCount = int64(len(files)) + 1
@@ -161,5 +140,12 @@ func (m *processRootIncludes) Apply(ctx context.Context, b *bundle.Bundle) diag.
161140
// whole accumulated configuration per file, making load quadratic in the number of
162141
// included files (~20 minutes for 6000 files).
163142
bundle.ApplySeqInScopeContext(ctx, b, out...)
143+
144+
// Swap out the original include patterns with the expanded globs. This is done after
145+
// the includes are merged: [config.Root.Merge] reloads the typed configuration from
146+
// the dynamic tree, which still holds the patterns, followed by the include sections
147+
// of included files (merging concatenates sequences). Those are kept as they are.
148+
// Include is omitempty, so an empty list is absent from the configuration.
149+
b.Config.Include = append(files, b.Config.Include[len(patterns):]...)
164150
return nil
165151
}

‎bundle/config/loader/process_root_includes_test.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package loader_test
22

33
import (
4+
"path/filepath"
45
"runtime"
56
"testing"
67

@@ -100,8 +101,8 @@ func TestProcessRootIncludesRemoveDups(t *testing.T) {
100101
}
101102

102103
// The expanded include list must be visible in both the typed and the dynamic
103-
// configuration: the per-file includes are applied without their own mutator scope, so
104-
// nothing converts the typed field back into the dynamic tree afterwards.
104+
// configuration: merging the per-file includes reloads the typed configuration from the
105+
// dynamic tree, so the expanded list must not be overwritten by the raw patterns.
105106
func TestProcessRootIncludesUpdatesDynamicValue(t *testing.T) {
106107
b := &bundle.Bundle{
107108
BundleRootPath: t.TempDir(),
@@ -142,6 +143,28 @@ func TestProcessRootIncludesEmptyOmitsDynamicValue(t *testing.T) {
142143
assert.Equal(t, dyn.KindInvalid, b.Config.Value().Get("include").Kind())
143144
}
144145

146+
// Include sections of included files are not applied, but they are merged into the
147+
// include list after the expanded globs.
148+
func TestProcessRootIncludesKeepsIncludesOfIncludedFiles(t *testing.T) {
149+
b := &bundle.Bundle{
150+
BundleRootPath: t.TempDir(),
151+
Config: config.Root{
152+
Include: []string{
153+
"*.yml",
154+
},
155+
},
156+
}
157+
158+
testutil.Touch(t, b.BundleRootPath, "databricks.yml")
159+
testutil.WriteFile(t, filepath.Join(b.BundleRootPath, "a.yml"), "include:\n - c.yml\n")
160+
testutil.Touch(t, b.BundleRootPath, "b.yml")
161+
162+
diags := bundle.Apply(t.Context(), b, loader.ProcessRootIncludes())
163+
require.NoError(t, diags.Error())
164+
assert.Equal(t, []string{"a.yml", "b.yml", "c.yml"}, b.Config.Include)
165+
assert.Equal(t, []any{"a.yml", "b.yml", "c.yml"}, b.Config.Value().Get("include").AsAny())
166+
}
167+
145168
func TestProcessRootIncludesNotExists(t *testing.T) {
146169
b := &bundle.Bundle{
147170
BundleRootPath: t.TempDir(),

‎bundle/config/mutator/prepend_workspace_prefix.go‎

Lines changed: 20 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,10 @@ package mutator
22

33
import (
44
"context"
5-
"fmt"
65
"strings"
76

87
"github.com/databricks/cli/bundle"
98
"github.com/databricks/cli/libs/diag"
10-
"github.com/databricks/cli/libs/dyn"
119
)
1210

1311
type prependWorkspacePrefix struct{}
@@ -27,46 +25,31 @@ var skipPrefixes = []string{
2725
}
2826

2927
func (m *prependWorkspacePrefix) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
30-
patterns := []dyn.Pattern{
31-
dyn.NewPattern(dyn.Key("workspace"), dyn.Key("root_path")),
32-
dyn.NewPattern(dyn.Key("workspace"), dyn.Key("file_path")),
33-
dyn.NewPattern(dyn.Key("workspace"), dyn.Key("artifact_path")),
34-
dyn.NewPattern(dyn.Key("workspace"), dyn.Key("state_path")),
35-
dyn.NewPattern(dyn.Key("workspace"), dyn.Key("resource_path")),
28+
w := &b.Config.Workspace
29+
for _, path := range []*string{
30+
&w.RootPath,
31+
&w.FilePath,
32+
&w.ArtifactPath,
33+
&w.StatePath,
34+
&w.ResourcePath,
35+
} {
36+
*path = prependWorkspacePrefixTo(*path)
3637
}
3738

38-
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
39-
var err error
40-
for _, pattern := range patterns {
41-
v, err = dyn.MapByPattern(v, pattern, func(p dyn.Path, pv dyn.Value) (dyn.Value, error) {
42-
path, ok := pv.AsString()
43-
if !ok {
44-
return dyn.InvalidValue, fmt.Errorf("expected string, got %s", pv.Kind())
45-
}
46-
47-
// Skip prefixing if the path does not start with /, it might be variable reference or smth else.
48-
if !strings.HasPrefix(path, "/") {
49-
return pv, nil
50-
}
39+
return nil
40+
}
5141

52-
for _, prefix := range skipPrefixes {
53-
if strings.HasPrefix(path, prefix) {
54-
return pv, nil
55-
}
56-
}
42+
func prependWorkspacePrefixTo(path string) string {
43+
// Skip prefixing if the path does not start with /, it might be variable reference or smth else.
44+
if !strings.HasPrefix(path, "/") {
45+
return path
46+
}
5747

58-
// Use pv's locations, not the root v's, so diagnostics point at the original config line.
59-
return dyn.NewValue("/Workspace"+path, pv.Locations()), nil
60-
})
61-
if err != nil {
62-
return dyn.InvalidValue, err
63-
}
48+
for _, prefix := range skipPrefixes {
49+
if strings.HasPrefix(path, prefix) {
50+
return path
6451
}
65-
return v, nil
66-
})
67-
if err != nil {
68-
return diag.FromErr(err)
6952
}
7053

71-
return nil
54+
return "/Workspace" + path
7255
}

‎bundle/config/mutator/resourcemutator/apply_bundle_permissions.go‎

Lines changed: 54 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package resourcemutator
33
import (
44
"context"
55
"fmt"
6+
"maps"
7+
"reflect"
68
"slices"
79
"strings"
810

@@ -11,8 +13,6 @@ import (
1113
"github.com/databricks/cli/bundle"
1214
"github.com/databricks/cli/bundle/config/resources"
1315
"github.com/databricks/cli/libs/diag"
14-
"github.com/databricks/cli/libs/dyn"
15-
"github.com/databricks/cli/libs/dyn/convert"
1616
"github.com/databricks/cli/libs/logdiag"
1717
"github.com/databricks/databricks-sdk-go/service/iam"
1818
)
@@ -108,63 +108,65 @@ func (m *bundlePermissions) Apply(ctx context.Context, b *bundle.Bundle) diag.Di
108108
return diag.FromErr(err)
109109
}
110110

111-
patterns := make(map[string]dyn.Pattern, 0)
112-
for key := range levelsMap {
113-
patterns[key] = dyn.NewPattern(
114-
dyn.Key("resources"),
115-
dyn.Key(key),
116-
dyn.AnyKey(),
117-
)
118-
}
119-
120-
err = b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
121-
for key, pattern := range patterns {
122-
v, err = dyn.MapByPattern(v, pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
123-
var permissions []resources.Permission
124-
pv, err := dyn.Get(v, "permissions")
125-
// If the permissions field is not found, we set to an empty array
126-
if err != nil {
127-
pv = dyn.V([]dyn.Value{})
128-
}
129-
130-
err = convert.ToTyped(&permissions, pv)
131-
if err != nil {
132-
return dyn.InvalidValue, fmt.Errorf("failed to convert permissions: %w", err)
133-
}
134-
135-
permissions = append(permissions, convertPermissions(
136-
ctx,
137-
b.Config.Permissions,
138-
permissions,
139-
key,
140-
levelsMap[key],
141-
)...)
142-
143-
if len(permissions) == 0 {
144-
permissions = nil
145-
}
146-
147-
pv, err = convert.FromTyped(permissions, dyn.NilValue)
148-
if err != nil {
149-
return dyn.InvalidValue, fmt.Errorf("failed to convert permissions: %w", err)
150-
}
151-
152-
return dyn.Set(v, "permissions", pv)
153-
})
154-
if err != nil {
155-
return dyn.InvalidValue, err
156-
}
111+
for _, group := range b.Config.Resources.AllResources() {
112+
key := group.Description.PluralName
113+
lm, ok := levelsMap[key]
114+
if !ok {
115+
continue
157116
}
158117

159-
return v, nil
160-
})
161-
if err != nil {
162-
return diag.FromErr(err)
118+
for _, name := range slices.Sorted(maps.Keys(group.Resources)) {
119+
field := reflect.ValueOf(group.Resources[name]).Elem().FieldByName("Permissions")
120+
permissions := readPermissions(field)
121+
permissions = append(permissions, convertPermissions(
122+
ctx,
123+
b.Config.Permissions,
124+
permissions,
125+
key,
126+
lm,
127+
)...)
128+
writePermissions(field, permissions)
129+
}
163130
}
164131

165132
return nil
166133
}
167134

135+
// readPermissions converts the permissions field of a resource to []resources.Permission.
136+
// The element type of the field differs between resources, but all have the same fields.
137+
func readPermissions(field reflect.Value) []resources.Permission {
138+
var permissions []resources.Permission
139+
for i := range field.Len() {
140+
p := field.Index(i)
141+
permissions = append(permissions, resources.Permission{
142+
Level: iam.PermissionLevel(p.FieldByName("Level").String()),
143+
UserName: p.FieldByName("UserName").String(),
144+
ServicePrincipalName: p.FieldByName("ServicePrincipalName").String(),
145+
GroupName: p.FieldByName("GroupName").String(),
146+
})
147+
}
148+
return permissions
149+
}
150+
151+
// writePermissions sets the permissions field of a resource to permissions.
152+
// An empty list is stored as nil, so that the field is absent from the configuration.
153+
func writePermissions(field reflect.Value, permissions []resources.Permission) {
154+
if len(permissions) == 0 {
155+
field.SetZero()
156+
return
157+
}
158+
159+
out := reflect.MakeSlice(field.Type(), len(permissions), len(permissions))
160+
for i, p := range permissions {
161+
e := out.Index(i)
162+
e.FieldByName("Level").SetString(string(p.Level))
163+
e.FieldByName("UserName").SetString(p.UserName)
164+
e.FieldByName("ServicePrincipalName").SetString(p.ServicePrincipalName)
165+
e.FieldByName("GroupName").SetString(p.GroupName)
166+
}
167+
field.Set(out)
168+
}
169+
168170
func validatePermissions(b *bundle.Bundle) error {
169171
for _, p := range b.Config.Permissions {
170172
if !slices.Contains(allowedLevels, string(p.Level)) {

‎bundle/config/mutator/resourcemutator/apply_bundle_permissions_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/databricks/cli/bundle/config"
1212
"github.com/databricks/cli/bundle/config/resources"
1313
"github.com/databricks/cli/libs/diag"
14+
"github.com/databricks/cli/libs/dyn"
1415
"github.com/databricks/databricks-sdk-go/service/jobs"
1516
"github.com/stretchr/testify/assert"
1617
"github.com/stretchr/testify/require"
@@ -237,3 +238,32 @@ func TestAllResourcesExplicitlyDefinedForPermissionsSupport(t *testing.T) {
237238
}
238239
}
239240
}
241+
242+
func TestApplyBundlePermissionsEmpty(t *testing.T) {
243+
b := &bundle.Bundle{
244+
Config: config.Root{
245+
Resources: config.Resources{
246+
Jobs: map[string]*resources.Job{
247+
"empty": {
248+
JobSettings: jobs.JobSettings{Name: "empty"},
249+
Permissions: []resources.JobPermission{},
250+
},
251+
"unset": {
252+
JobSettings: jobs.JobSettings{Name: "unset"},
253+
},
254+
},
255+
},
256+
},
257+
}
258+
259+
diags := bundle.Apply(t.Context(), b, ApplyBundlePermissions())
260+
require.NoError(t, diags.Error())
261+
262+
// An empty list of permissions is dropped, an unset one stays unset.
263+
for _, name := range []string{"empty", "unset"} {
264+
require.Nil(t, b.Config.Resources.Jobs[name].Permissions)
265+
v, err := dyn.GetByPath(b.Config.Value(), dyn.MustPathFromString("resources.jobs."+name))
266+
require.NoError(t, err)
267+
assert.False(t, v.Get("permissions").IsValid(), name)
268+
}
269+
}

0 commit comments

Comments
 (0)