diff --git a/.nextchanges/bundles/alert-sql-file-path.md b/.nextchanges/bundles/alert-sql-file-path.md new file mode 100644 index 00000000000..ca734a6e3c0 --- /dev/null +++ b/.nextchanges/bundles/alert-sql-file-path.md @@ -0,0 +1 @@ +* Support `.sql` files in an alert's `file_path`, so `query_text` can live in its own SQL file while other fields stay in the bundle configuration. ([#7330](https://github.com/databricks/cli/pull/7330)) diff --git a/acceptance/bundle/resources/alerts/with_sql_file/databricks.yml.tmpl b/acceptance/bundle/resources/alerts/with_sql_file/databricks.yml.tmpl new file mode 100644 index 00000000000..25507c924d0 --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file/databricks.yml.tmpl @@ -0,0 +1,26 @@ +bundle: + name: alerts-with-sql-file-$UNIQUE_NAME + +resources: + alerts: + myalert: + custom_summary: "My alert" + display_name: "My alert" + evaluation: + comparison_operator: "EQUAL" + notification: + notify_on_ok: false + retrigger_seconds: 1 + source: + aggregation: "MAX" + display: "2" + name: "2" + threshold: + value: + double_value: 2 + file_path: ./sql/failed_runs.sql + schedule: + pause_status: "UNPAUSED" + quartz_cron_schedule: "44 19 */1 * * ?" + timezone_id: "Europe/Amsterdam" + warehouse_id: $TEST_DEFAULT_WAREHOUSE_ID diff --git a/acceptance/bundle/resources/alerts/with_sql_file/out.test.toml b/acceptance/bundle/resources/alerts/with_sql_file/out.test.toml new file mode 100644 index 00000000000..0f167effef1 --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file/out.test.toml @@ -0,0 +1,2 @@ +Cloud = true +EnvMatrix.DMS = ["", "true"] diff --git a/acceptance/bundle/resources/alerts/with_sql_file/output.txt b/acceptance/bundle/resources/alerts/with_sql_file/output.txt new file mode 100644 index 00000000000..b73d50aa40b --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file/output.txt @@ -0,0 +1,52 @@ + +>>> [CLI] bundle validate -o json +{ + "display_name": "My alert", + "file_path": "sql/failed_runs.sql", + "query_text": "-- Alert query kept in its own file so it can be reviewed and linted as SQL.\nselect 2\n", + "custom_summary": "My alert" +} + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/alerts-with-sql-file-[UNIQUE_NAME]/default/files... +Created alerts.myalert +Files: 3 uploaded, 0 deleted +Resources: 1 created, 0 changed, 0 deleted, 0 unchanged + +>>> [CLI] alerts-v2 get-alert [ALERT_ID] +{ + "display_name": "My alert", + "lifecycle_state": "ACTIVE", + "custom_summary": "My alert", + "query_text": "-- Alert query kept in its own file so it can be reviewed and linted as SQL.\nselect 2\n", + "warehouse_id": "[TEST_DEFAULT_WAREHOUSE_ID]" +} + +=== assert that no permanent drift happens +>>> [CLI] bundle plan +Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged + +=== edit the .sql file and redeploy +>>> [CLI] bundle plan +update alerts.myalert + +Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged + +>>> [CLI] bundle deploy +Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/alerts-with-sql-file-[UNIQUE_NAME]/default/files... +Updated alerts.myalert +Files: 1 uploaded, 0 deleted +Resources: 0 created, 1 changed, 0 deleted, 0 unchanged + +>>> [CLI] alerts-v2 get-alert [ALERT_ID] +{ + "query_text": "select 3\n" +} + +>>> [CLI] bundle destroy --auto-approve +The following resources will be deleted: + delete resources.alerts.myalert + +All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/alerts-with-sql-file-[UNIQUE_NAME]/default + +Destroy: 1 deleted diff --git a/acceptance/bundle/resources/alerts/with_sql_file/script b/acceptance/bundle/resources/alerts/with_sql_file/script new file mode 100644 index 00000000000..4cf99fbec1d --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file/script @@ -0,0 +1,27 @@ +envsubst < databricks.yml.tmpl > databricks.yml + +cleanup() { + trace $CLI bundle destroy --auto-approve +} +trap cleanup EXIT + +trace $CLI bundle validate -o json | jq '.resources.alerts.myalert | {display_name, file_path, query_text, custom_summary}' + +trace $CLI bundle deploy + +alert_id=$($CLI bundle summary --output json | jq -r '.resources.alerts.myalert.id') + +add_repl "$alert_id" ALERT_ID + +trace $CLI alerts-v2 get-alert $alert_id | jq '{display_name, lifecycle_state, custom_summary, query_text, warehouse_id}' + +title "assert that no permanent drift happens" +trace $CLI bundle plan + +title "edit the .sql file and redeploy" +cat > sql/failed_runs.sql <>> [CLI] bundle validate +Error: query_text cannot be set when file_path points to a .sql file + at resources.alerts.myalert.query_text + in databricks.yml:11:19 + +The alert query is read from query.sql. Remove either query_text or file_path. + +Name: alerts-sql-file-query-text +Target: default +Workspace: + User: [USERNAME] + Path: /Workspace/Users/[USERNAME]/.bundle/alerts-sql-file-query-text/default + +Found 1 error diff --git a/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/query.sql b/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/query.sql new file mode 100644 index 00000000000..f488dd19e48 --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/query.sql @@ -0,0 +1 @@ +select 2 diff --git a/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/script b/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/script new file mode 100644 index 00000000000..277355fe918 --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/script @@ -0,0 +1 @@ +musterr trace $CLI bundle validate diff --git a/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/test.toml b/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/test.toml new file mode 100644 index 00000000000..e1290bad9f4 --- /dev/null +++ b/acceptance/bundle/resources/alerts/with_sql_file_query_text_error/test.toml @@ -0,0 +1,2 @@ +RecordRequests = false +Ignore = [".databricks"] diff --git a/bundle/config/mutator/load_dbalert_files.go b/bundle/config/mutator/load_dbalert_files.go index 437095aa218..1573acc03c3 100644 --- a/bundle/config/mutator/load_dbalert_files.go +++ b/bundle/config/mutator/load_dbalert_files.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/dyn" "github.com/databricks/cli/libs/dyn/dynvar" @@ -62,6 +63,13 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia return diag.FromErr(err) } + if isAlertQueryFile(alert.FilePath) { + if diags := loadAlertQueryFile(b, alertKey, alert, alertV); diags.HasError() { + return diags + } + continue + } + // No other fields other than allowedInYAML should be set in the bundle YAML. m, ok := alertV.AsMap() if !ok { @@ -92,15 +100,7 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia } } - // NormalizePaths rewrote file_path to be relative to the bundle root, but the - // process working directory can be any directory within the bundle, so anchor - // the read to the bundle root. Absolute paths are kept as-is by NormalizePaths. - filePath := alert.FilePath - if !filepath.IsAbs(filePath) { - filePath = filepath.Join(b.BundleRootPath, filePath) - } - - content, err := os.ReadFile(filePath) + content, err := os.ReadFile(alertFilePath(b, alert.FilePath)) if err != nil { return diag.Diagnostics{ { @@ -200,3 +200,86 @@ func (m *loadDBAlertFiles) Apply(ctx context.Context, b *bundle.Bundle) diag.Dia return nil } + +// alertFilePath anchors a file_path to the bundle root. NormalizePaths rewrote +// file_path to be relative to the bundle root, but the process working directory +// can be any directory within the bundle. Absolute paths are kept as-is by +// NormalizePaths. +func alertFilePath(b *bundle.Bundle, filePath string) string { + if filepath.IsAbs(filePath) { + return filePath + } + return filepath.Join(b.BundleRootPath, filePath) +} + +// isAlertQueryFile reports whether file_path points to a .sql file holding only +// the alert query, as opposed to a full .dbalert.json alert definition. +func isAlertQueryFile(filePath string) bool { + return strings.EqualFold(filepath.Ext(filePath), ".sql") +} + +// loadAlertQueryFile sets query_text from a .sql file. Unlike a .dbalert.json, the +// file only holds the query, so all other alert fields stay in the bundle YAML. +func loadAlertQueryFile(b *bundle.Bundle, alertKey string, alert *resources.Alert, alertV dyn.Value) diag.Diagnostics { + filePathV := alertV.Get("file_path") + filePathPaths := structpath.NewPathSlice("resources", "alerts", alertKey, "file_path") + + if q := alertV.Get("query_text"); q.Kind() != dyn.KindInvalid && q.Kind() != dyn.KindNil { + return diag.Diagnostics{ + { + ID: "", + Severity: diag.Error, + Summary: "query_text cannot be set when file_path points to a .sql file", + Detail: fmt.Sprintf("The alert query is read from %s. Remove either query_text or file_path.", alert.FilePath), + Paths: structpath.NewPathSlice("resources", "alerts", alertKey, "query_text"), + Locations: q.Locations(), + }, + } + } + + content, err := os.ReadFile(alertFilePath(b, alert.FilePath)) + if err != nil { + return diag.Diagnostics{ + { + ID: "", + Severity: diag.Error, + Summary: fmt.Sprintf("failed to read alert query file %s: %s", alert.FilePath, err), + Detail: "", + Paths: filePathPaths, + Locations: filePathV.Locations(), + }, + } + } + + query := string(content) + if strings.TrimSpace(query) == "" { + return diag.Diagnostics{ + { + ID: "", + Severity: diag.Error, + Summary: fmt.Sprintf("alert query file %s is empty", alert.FilePath), + Detail: "", + Paths: filePathPaths, + Locations: filePathV.Locations(), + }, + } + } + + // The file is read after variables are resolved, so references in it would be + // sent to the backend verbatim. Reject them, like for .dbalert.json files. + if dynvar.ContainsVariableReference(query) { + return diag.Diagnostics{ + { + ID: "", + Severity: diag.Error, + Summary: fmt.Sprintf("alert query file %s must not contain variable interpolations.", alert.FilePath), + Detail: "Please set query_text inline in the bundle configuration to use variables", + Paths: filePathPaths, + Locations: filePathV.Locations(), + }, + } + } + + alert.QueryText = query + return nil +} diff --git a/bundle/config/mutator/load_dbalert_files_test.go b/bundle/config/mutator/load_dbalert_files_test.go index 5e8a8d896e6..ba5c2ac32aa 100644 --- a/bundle/config/mutator/load_dbalert_files_test.go +++ b/bundle/config/mutator/load_dbalert_files_test.go @@ -118,3 +118,112 @@ func TestLoadDBAlertFilesRelativeToBundleRoot(t *testing.T) { assert.Equal(t, "SELECT 1\n", b.Config.Resources.Alerts["my_alert"].QueryText) } + +func newSQLFileAlertBundle(t *testing.T, dir, sqlFile string, alert sql.AlertV2) *bundle.Bundle { + t.Helper() + b := &bundle.Bundle{ + BundleRootPath: dir, + Config: config.Root{ + Resources: config.Resources{ + Alerts: map[string]*resources.Alert{ + "my_alert": { + FilePath: sqlFile, + AlertV2: alert, + }, + }, + }, + }, + } + bundletest.SetLocation(b, "resources.alerts.my_alert", []diag.Location{{ + File: filepath.Join(dir, "databricks.yml"), + }}) + return b +} + +func TestLoadDBAlertFilesSQLQueryFile(t *testing.T) { + dir := t.TempDir() + query := "SELECT count(*) AS failed\nFROM system.lakeflow.pipeline_update_timeline\nWHERE result_state = 'FAILED'\n" + require.NoError(t, os.MkdirAll(filepath.Join(dir, "sql"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "sql", "failed_runs.sql"), []byte(query), 0o644)) + + evaluation := sql.AlertV2Evaluation{ + ComparisonOperator: sql.ComparisonOperatorGreaterThan, + Source: sql.AlertV2OperandColumn{Name: "failed"}, + } + b := newSQLFileAlertBundle(t, dir, "sql/failed_runs.sql", sql.AlertV2{ + DisplayName: "Failed runs", + WarehouseId: "abc123", + CustomSummary: "Pipeline failures", + Evaluation: evaluation, + }) + + diags := bundle.Apply(t.Context(), b, mutator.LoadDBAlertFiles()) + require.NoError(t, diags.Error()) + + // The query comes from the file and every other field stays as configured in YAML. + alert := b.Config.Resources.Alerts["my_alert"] + assert.Equal(t, query, alert.QueryText) + assert.Equal(t, "Failed runs", alert.DisplayName) + assert.Equal(t, "abc123", alert.WarehouseId) + assert.Equal(t, "Pipeline failures", alert.CustomSummary) + assert.Equal(t, evaluation, alert.Evaluation) + assert.Equal(t, "sql/failed_runs.sql", alert.FilePath) +} + +func TestLoadDBAlertFilesSQLQueryFileUppercaseExtension(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "query.SQL"), []byte("SELECT 1"), 0o644)) + + b := newSQLFileAlertBundle(t, dir, "query.SQL", sql.AlertV2{DisplayName: "a", WarehouseId: "w"}) + + diags := bundle.Apply(t.Context(), b, mutator.LoadDBAlertFiles()) + require.NoError(t, diags.Error()) + assert.Equal(t, "SELECT 1", b.Config.Resources.Alerts["my_alert"].QueryText) +} + +func TestLoadDBAlertFilesSQLQueryFileErrors(t *testing.T) { + for _, tc := range []struct { + name string + content *string + alert sql.AlertV2 + summary string + }{ + { + name: "query_text also set", + content: new("SELECT 1"), + alert: sql.AlertV2{DisplayName: "a", WarehouseId: "w", QueryText: "SELECT 2"}, + summary: "query_text cannot be set when file_path points to a .sql file", + }, + { + name: "missing file", + alert: sql.AlertV2{DisplayName: "a", WarehouseId: "w"}, + summary: "failed to read alert query file query.sql", + }, + { + name: "empty file", + content: new(" \n"), + alert: sql.AlertV2{DisplayName: "a", WarehouseId: "w"}, + summary: "alert query file query.sql is empty", + }, + { + name: "variable reference", + content: new("SELECT * FROM ${var.catalog}.sales"), + alert: sql.AlertV2{DisplayName: "a", WarehouseId: "w"}, + summary: "alert query file query.sql must not contain variable interpolations.", + }, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + if tc.content != nil { + require.NoError(t, os.WriteFile(filepath.Join(dir, "query.sql"), []byte(*tc.content), 0o644)) + } + + b := newSQLFileAlertBundle(t, dir, "query.sql", tc.alert) + + diags := bundle.Apply(t.Context(), b, mutator.LoadDBAlertFiles()) + require.Len(t, diags, 1) + assert.Equal(t, diag.Error, diags[0].Severity) + assert.Contains(t, diags[0].Summary, tc.summary) + }) + } +} diff --git a/bundle/config/resources/alerts.go b/bundle/config/resources/alerts.go index 4416873d526..f93c243477a 100644 --- a/bundle/config/resources/alerts.go +++ b/bundle/config/resources/alerts.go @@ -18,9 +18,9 @@ type Alert struct { Permissions []Permission `json:"permissions,omitempty"` - // Filepath points to the local .dbalert.json file containing the alert definition. - // If specified, any fields that are part of the .dbalert.json file schema will not be allowed in - // the bundle config. + // Filepath points to a local file with the alert definition. It is either a .dbalert.json + // file, in which case any fields that are part of the .dbalert.json file schema are not + // allowed in the bundle config, or a .sql file that only provides query_text. FilePath string `json:"file_path,omitempty"` } diff --git a/bundle/internal/schema/annotations.yml b/bundle/internal/schema/annotations.yml index 39180db1852..31b5432300a 100644 --- a/bundle/internal/schema/annotations.yml +++ b/bundle/internal/schema/annotations.yml @@ -304,7 +304,7 @@ resources: PLACEHOLDER "file_path": "description": |- - PLACEHOLDER + The local path to a `.dbalert.json` file with the alert definition, or to a `.sql` file with the alert query. With a `.dbalert.json` file, only `warehouse_id`, `display_name`, `permissions` and `lifecycle` can also be set in the bundle configuration. With a `.sql` file, the file provides `query_text` and all other fields are set in the bundle configuration. "lifecycle": "description": |- Settings that control the deployment lifecycle of the resource, such as preventing it from being destroyed. diff --git a/bundle/schema/jsonschema.json b/bundle/schema/jsonschema.json index df4f1a753b4..26ef5211664 100644 --- a/bundle/schema/jsonschema.json +++ b/bundle/schema/jsonschema.json @@ -106,6 +106,7 @@ "x-databricks-launch-stage": "GA" }, "file_path": { + "description": "The local path to a `.dbalert.json` file with the alert definition, or to a `.sql` file with the alert query. With a `.dbalert.json` file, only `warehouse_id`, `display_name`, `permissions` and `lifecycle` can also be set in the bundle configuration. With a `.sql` file, the file provides `query_text` and all other fields are set in the bundle configuration.", "$ref": "#/$defs/string" }, "lifecycle": { diff --git a/python/databricks/bundles/alerts/_models/alert.py b/python/databricks/bundles/alerts/_models/alert.py index e6dbf9d8448..af6cd4cbf12 100644 --- a/python/databricks/bundles/alerts/_models/alert.py +++ b/python/databricks/bundles/alerts/_models/alert.py @@ -90,6 +90,9 @@ class Alert(Resource): """ file_path: VariableOrOptional[str] = None + """ + The local path to a `.dbalert.json` file with the alert definition, or to a `.sql` file with the alert query. With a `.dbalert.json` file, only `warehouse_id`, `display_name`, `permissions` and `lifecycle` can also be set in the bundle configuration. With a `.sql` file, the file provides `query_text` and all other fields are set in the bundle configuration. + """ lifecycle: VariableOrOptional[Lifecycle] = None """ @@ -195,6 +198,9 @@ class AlertDict(TypedDict, total=False): """ file_path: VariableOrOptional[str] + """ + The local path to a `.dbalert.json` file with the alert definition, or to a `.sql` file with the alert query. With a `.dbalert.json` file, only `warehouse_id`, `display_name`, `permissions` and `lifecycle` can also be set in the bundle configuration. With a `.sql` file, the file provides `query_text` and all other fields are set in the bundle configuration. + """ lifecycle: VariableOrOptional[LifecycleParam] """