diff --git a/.nextchanges/bundles/5818.md b/.nextchanges/bundles/5818.md new file mode 100644 index 00000000000..5156b3b2a35 --- /dev/null +++ b/.nextchanges/bundles/5818.md @@ -0,0 +1 @@ +* `bundle validate` now reports a clear error when a `sql_warehouse` is missing a `name` (including whitespace-only names), and a warning when a grant is missing a `principal` ([#5818](https://github.com/databricks/cli/pull/5818)). diff --git a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt index be722ce5e1a..0ff8111602e 100644 --- a/acceptance/bundle/validate/empty_resources/empty_dict/output.txt +++ b/acceptance/bundle/validate/empty_resources/empty_dict/output.txt @@ -161,6 +161,10 @@ app resource 'rname' should have either source_code_path or git_source field } === resources.sql_warehouses.rname === +Error: sql_warehouse name is required + at resources.sql_warehouses.rname + in databricks.yml:6:12 + { "sql_warehouses": { "rname": { diff --git a/acceptance/bundle/validate/empty_resources/with_grants/output.txt b/acceptance/bundle/validate/empty_resources/with_grants/output.txt index e4eba64c059..38c4dc55d19 100644 --- a/acceptance/bundle/validate/empty_resources/with_grants/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_grants/output.txt @@ -202,6 +202,10 @@ Warning: unknown field: grants at resources.sql_warehouses.rname in databricks.yml:7:7 +Error: sql_warehouse name is required + at resources.sql_warehouses.rname + in databricks.yml:7:7 + { "sql_warehouses": { "rname": { diff --git a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt index 9a2895d068d..cef0b18baa3 100644 --- a/acceptance/bundle/validate/empty_resources/with_permissions/output.txt +++ b/acceptance/bundle/validate/empty_resources/with_permissions/output.txt @@ -177,6 +177,10 @@ app resource 'rname' should have either source_code_path or git_source field } === resources.sql_warehouses.rname === +Error: sql_warehouse name is required + at resources.sql_warehouses.rname + in databricks.yml:7:7 + { "sql_warehouses": { "rname": { diff --git a/acceptance/bundle/validate/grants_required_principal/databricks.yml b/acceptance/bundle/validate/grants_required_principal/databricks.yml new file mode 100644 index 00000000000..832ec6ec0bd --- /dev/null +++ b/acceptance/bundle/validate/grants_required_principal/databricks.yml @@ -0,0 +1,20 @@ +bundle: + name: test-bundle + +resources: + catalogs: + my_catalog: + name: my_catalog + grants: + # Missing principal (warns). + - privileges: + - ALL_PRIVILEGES + schemas: + my_schema: + catalog_name: my_catalog + name: my_schema + grants: + # Valid grant (principal set). + - principal: alice@example.com + privileges: + - USE_SCHEMA diff --git a/acceptance/bundle/validate/grants_required_principal/out.test.toml b/acceptance/bundle/validate/grants_required_principal/out.test.toml new file mode 100644 index 00000000000..1a3e24fa574 --- /dev/null +++ b/acceptance/bundle/validate/grants_required_principal/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false +GOOSOnPR.darwin = false +GOOSOnPR.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/validate/grants_required_principal/output.txt b/acceptance/bundle/validate/grants_required_principal/output.txt new file mode 100644 index 00000000000..79629ec21d5 --- /dev/null +++ b/acceptance/bundle/validate/grants_required_principal/output.txt @@ -0,0 +1,13 @@ + +>>> [CLI] bundle validate +Warning: grant principal is required + at resources.catalogs.my_catalog.grants[0] + in databricks.yml:10:11 + +Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Found 1 warning diff --git a/acceptance/bundle/validate/grants_required_principal/script b/acceptance/bundle/validate/grants_required_principal/script new file mode 100644 index 00000000000..5350876150f --- /dev/null +++ b/acceptance/bundle/validate/grants_required_principal/script @@ -0,0 +1 @@ +trace $CLI bundle validate diff --git a/acceptance/bundle/validate/grants_required_principal/test.toml b/acceptance/bundle/validate/grants_required_principal/test.toml new file mode 100644 index 00000000000..fc2b3f50667 --- /dev/null +++ b/acceptance/bundle/validate/grants_required_principal/test.toml @@ -0,0 +1,3 @@ +# Catalogs and schemas are only supported by the direct engine. +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/bundle/validate/sql_warehouse_required_name/databricks.yml b/acceptance/bundle/validate/sql_warehouse_required_name/databricks.yml new file mode 100644 index 00000000000..9941a3085a4 --- /dev/null +++ b/acceptance/bundle/validate/sql_warehouse_required_name/databricks.yml @@ -0,0 +1,12 @@ +bundle: + name: test-bundle + +resources: + sql_warehouses: + # Missing name (required by the backend). + my_warehouse: + cluster_size: "2X-Small" + # Whitespace-only name, treated as missing (name.trim.nonEmpty). + blank_warehouse: + name: " " + cluster_size: "2X-Small" diff --git a/acceptance/bundle/validate/sql_warehouse_required_name/out.test.toml b/acceptance/bundle/validate/sql_warehouse_required_name/out.test.toml new file mode 100644 index 00000000000..67759662971 --- /dev/null +++ b/acceptance/bundle/validate/sql_warehouse_required_name/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false +GOOSOnPR.darwin = false +GOOSOnPR.windows = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/validate/sql_warehouse_required_name/output.txt b/acceptance/bundle/validate/sql_warehouse_required_name/output.txt new file mode 100644 index 00000000000..fc66fed24e1 --- /dev/null +++ b/acceptance/bundle/validate/sql_warehouse_required_name/output.txt @@ -0,0 +1,19 @@ + +>>> [CLI] bundle validate +Error: sql_warehouse name is required + at resources.sql_warehouses.blank_warehouse + in databricks.yml:11:7 + +Error: sql_warehouse name is required + at resources.sql_warehouses.my_warehouse + in databricks.yml:8:7 + +Name: test-bundle +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default + +Found 2 errors + +Exit code: 1 diff --git a/acceptance/bundle/validate/sql_warehouse_required_name/script b/acceptance/bundle/validate/sql_warehouse_required_name/script new file mode 100644 index 00000000000..5350876150f --- /dev/null +++ b/acceptance/bundle/validate/sql_warehouse_required_name/script @@ -0,0 +1 @@ +trace $CLI bundle validate diff --git a/bundle/config/validate/required.go b/bundle/config/validate/required.go index 6f886caab44..a5a1b611357 100644 --- a/bundle/config/validate/required.go +++ b/bundle/config/validate/required.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "slices" + "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/internal/validation/generated" @@ -68,7 +69,14 @@ func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostic return diag.FromErr(err) } - // Sort diagnostics to make them deterministic + sortDiagnostics(diags) + + return diags +} + +// sortDiagnostics orders diagnostics deterministically, since they are collected +// by walking maps with random iteration order. +func sortDiagnostics(diags diag.Diagnostics) { slices.SortFunc(diags, func(a, b diag.Diagnostic) int { // First sort by summary if n := cmp.Compare(a.Summary, b.Summary); n != 0 { @@ -78,8 +86,6 @@ func warnForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnostic // Finally sort by locations as a tie breaker if summaries are the same. return cmp.Compare(fmt.Sprintf("%v", a.Locations), fmt.Sprintf("%v", b.Locations)) }) - - return diags } // Bespoke code to error for fields that are not marked as required in the Go SDK / OpenAPI spec. @@ -119,14 +125,73 @@ func errorForMissingFields(ctx context.Context, b *bundle.Bundle) diag.Diagnosti }) } + // sql_warehouses.name is optional in the SDK (json:"name,omitempty") but required + // by the backend, which rejects whitespace-only names (name.trim.nonEmpty). + for key, warehouse := range b.Config.Resources.SqlWarehouses { + if strings.TrimSpace(warehouse.Name) == "" { + path := "resources.sql_warehouses." + key + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Error, + Summary: "sql_warehouse name is required", + Locations: b.Config.GetLocations(path), + Paths: []dyn.Path{dyn.MustPathFromString(path)}, + }) + } + } + + sortDiagnostics(diags) + + return diags +} + +// warnForMissingGrantPrincipals warns for any grant that is missing a principal. +// grants[*].principal is optional in the SDK (json:"principal,omitempty") but the +// backend requires it. Grants exist on every securable, so match any resource type. +func warnForMissingGrantPrincipals(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + diags := diag.Diagnostics{} + + _, err := dyn.MapByPattern( + b.Config.Value(), + dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey(), dyn.Key("grants"), dyn.AnyIndex()), + func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + if isMissingOrEmptyString(v.Get("principal")) { + diags = diags.Append(diag.Diagnostic{ + Severity: diag.Warning, + Summary: "grant principal is required", + Locations: v.Locations(), + Paths: []dyn.Path{slices.Clone(p)}, + }) + } + return v, nil + }, + ) + if err != nil { + return diag.FromErr(err) + } + + sortDiagnostics(diags) + return diags } +// isMissingOrEmptyString reports whether v is unset, null, or an empty string. +func isMissingOrEmptyString(v dyn.Value) bool { + switch v.Kind() { + case dyn.KindInvalid, dyn.KindNil: + return true + case dyn.KindString: + return v.MustString() == "" + default: + return false + } +} + func (f *required) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { diags := errorForMissingFields(ctx, b) if diags.HasError() { return diags } diags = diags.Extend(warnForMissingFields(ctx, b)) + diags = diags.Extend(warnForMissingGrantPrincipals(ctx, b)) return diags }