[Feature] Add `databricks_alert` resource to replace `databricks_sql_alert` #4051

alexott · 2024-09-27T11:19:41Z

Changes

The new resource uses the new Alerts API instead of the legacy one that will be deprecated. Since the new resource has a slightly different set of parameters, it was decided to create a new resource and deprecate the old one.

This resource uses old TF SDK to be compatible with TF exporter (until #4050 is implemented).

TODOs:

Need to discuss how to handle permissions - sql_alert permissions look like working, but not sure if we should continue to use that API
Support in the exporter will be in a separate PR

Tests

make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
relevant acceptance tests are passing
using Go SDK

…alert` New resource use the [new Alerts API](https://docs.databricks.com/api/workspace/alerts/create) instead of legacy one that will be deprecated. New resource has slightly different set of parameters, so it was decided to create new resource and deprecate old one. This resource uses old TF SDK to be compatible with TF exporter (until #4050 is implemented).

…query` This PR is built on top of #4051 that should be merged first. The new resource uses the new [Queries API](https://docs.databricks.com/api/workspace/queries/create) instead of the legacy one that will be deprecated. Since the new resource has a slightly different set of parameters, it was decided to create a new resource and deprecate the old one. This resource uses old TF SDK to be compatible with TF exporter (until #4050 is implemented). TODOs: - Add documentation - Need to discuss how to handle permissions - `sql_query` permissions look like working, but not sure if we should continue to use that API - Support in the exporter will be in a separate PR

mgyucht

Few comments, but otherwise mostly LGTM. Did you try to use the plugin framework, or was it just easier to implement as an SDK resource because the original resource used the SDK?

docs/resources/alert.md

internal/acceptance/alert_test.go

sql/resource_alert.go

Co-authored-by: Miles Yucht <miles@databricks.com>

mgyucht

LGTM, one small question about update mask but otherwise it's great.

mgyucht · 2024-10-17T11:45:14Z

sql/resource_alert.go

+				return err
+			}
+			var a sql.UpdateAlertRequestAlert
+			updateMask := "display_name,query_id,seconds_to_retrigger,condition,custom_body,custom_subject"


Do we use updateMask anywhere else in the TF provider? I wonder if we should always set it for all fields, even if fields don't have any change, since the *schema.ResourceData has the desired values for all of those fields. Mainly asking from the perspective of if we need logic around setting update masks or if we can avoid it to make things simpler.

that's a good question - we have updateMask in settings resources and in new alerts/queries.

…query` This PR is built on top of #4051 that should be merged first. The new resource uses the new [Queries API](https://docs.databricks.com/api/workspace/queries/create) instead of the legacy one that will be deprecated. Since the new resource has a slightly different set of parameters, it was decided to create a new resource and deprecate the old one. This resource uses old TF SDK to be compatible with TF exporter (until #4050 is implemented). TODOs: - Add documentation - Need to discuss how to handle permissions - `sql_query` permissions look like working, but not sure if we should continue to use that API - Support in the exporter will be in a separate PR

### New Features and Improvements * Add `databricks_alert` resource to replace `databricks_sql_alert` ([#4051](#4051)). * Added resource `databricks_custom_app_integration` ([#4124](#4124)). * Handle `schema` attribute in `databricks_pipeline` ([#4137](#4137)). ### Bug Fixes * Change repo used in test ([#4122](#4122)). ### Documentation * Clarify that `graviton` option of `databricks_node_type` could be used on Azure ([#4125](#4125)). * Fix argument in example for `databricks_custom_app_integration` ([#4132](#4132)). * Fix for UC on AWS guide - use `databricks_aws_unity_catalog_assume_role_policy` where necessary ([#4109](#4109)). ### Exporter * **Breaking change**: Move `databricks_workspace_file` to a separate service ([#4118](#4118)). * Exclude some system schemas from export ([#4121](#4121)). * Use `List` + iteration instead of call to `ListAll` ([#4123](#4123)).

…query` (#4103) ## Changes  This PR is built on top of #4051 which should be merged first. The new resource uses the new [Queries API](https://docs.databricks.com/api/workspace/queries/create) instead of the legacy one that will be deprecated. Since the new resource has a slightly different set of parameters, it was decided to create a new resource and deprecate the old one. This resource uses old TF SDK to be compatible with TF exporter (until #4050 is implemented). TODOs: - Need to discuss how to handle permissions - `sql_query` permissions look like working, but not sure if we should continue to use that API - Support in the exporter will be in a separate PR ## Tests  - [x] `make test` run locally - [x] relevant change in `docs/` folder - [x] covered with integration tests in `internal/acceptance` - [x] relevant acceptance tests are passing - [x] using Go SDK

### New Features and Improvements * Add `databricks_alert` resource to replace `databricks_sql_alert` ([#4051](#4051)). * Add `databricks_query` resource instead of `databricks_sql_query` ([#4103](#4103)). * Added resource `databricks_custom_app_integration` ([#4124](#4124)). * Handle `schema` attribute in `databricks_pipeline` ([#4137](#4137)). ### Bug Fixes * Change repo used in test ([#4122](#4122)). ### Documentation * Clarify that `graviton` option of `databricks_node_type` could be used on Azure ([#4125](#4125)). * Fix argument in example for `databricks_custom_app_integration` ([#4132](#4132)). * Fix for UC on AWS guide - use `databricks_aws_unity_catalog_assume_role_policy` where necessary ([#4109](#4109)). ### Exporter * **Breaking change**: Move `databricks_workspace_file` to a separate service ([#4118](#4118)). * Exclude some system schemas from export ([#4121](#4121)). * Use `List` + iteration instead of call to `ListAll` ([#4123](#4123)).

### New Features and Improvements * Add `databricks_alert` resource to replace `databricks_sql_alert` ([#4051](#4051)). * Add `databricks_query` resource instead of `databricks_sql_query` ([#4103](#4103)). * Added resource `databricks_custom_app_integration` ([#4124](#4124)). * Handle `schema` attribute in `databricks_pipeline` ([#4137](#4137)). ### Bug Fixes * Change repo used in test ([#4122](#4122)). ### Documentation * Clarify that `graviton` option of `databricks_node_type` could be used on Azure ([#4125](#4125)). * Fix argument in example for `databricks_custom_app_integration` ([#4132](#4132)). * Fix for UC on AWS guide - use `databricks_aws_unity_catalog_assume_role_policy` where necessary ([#4109](#4109)). ### Exporter * **Breaking change**: Move `databricks_workspace_file` to a separate service ([#4118](#4118)). * Exclude some system schemas from export ([#4121](#4121)). * Use `List` + iteration instead of call to `ListAll` ([#4123](#4123)). Co-authored-by: Omer Lachish <rauchy@users.noreply.github.com>

alexott force-pushed the new-alert-resource branch from 11fde34 to 355a791 Compare October 3, 2024 18:57

alexott added 11 commits October 3, 2024 19:31

Started to add unit tests

5910dfb

read and delete tests

12d5984

finish with the unit tests

c9c9822

Add deprecation message to sql alert resource

1ed658e

Added integration test

8b8e71c

Fix integration test template

2ab3db1

Merge branch 'main' into new-alert-resource

17dd2cc

Add suppress diff for workspace files

1a0ecf1

Add docs

06bf71c

Tune schema customization

eabde7a

Add setting the owner on creation

03243fc

alexott marked this pull request as ready for review October 8, 2024 10:42

alexott requested review from a team as code owners October 8, 2024 10:42

alexott requested review from parthban-db and mgyucht and removed request for a team October 8, 2024 10:42

alexott added 5 commits October 8, 2024 14:14

Add another custom diff for parent_path

85d0c09

Add migration guide

f42bc9b

Add an example to the migration guide

81af2f8

Merge branch 'main' into new-alert-resource

6688eee

Add an example of permissions for alerts

9b4fc63

alexott mentioned this pull request Oct 14, 2024

[Feature] Add databricks_query resource instead of databricks_sql_query #4103

Merged

5 tasks

mgyucht reviewed Oct 14, 2024

View reviewed changes

alexott and others added 2 commits October 14, 2024 17:29

Update docs/resources/alert.md

a6ee435

Co-authored-by: Miles Yucht <miles@databricks.com>

Start to address review comments

a9a747b

alexott mentioned this pull request Oct 14, 2024

[FEATURE] For Enums expose a function that will return a list of all possible constants defined databricks/databricks-sdk-go#1063

Open

alexott added 3 commits October 15, 2024 07:21

Update migration instructions for TF 1.7+

0aebe5a

reimplement permissions test

7f3e132

Fix integration test

0df79b2

alexott requested a review from mgyucht October 15, 2024 06:47

alexott added 2 commits October 16, 2024 16:29

Merge branch 'main' into new-alert-resource

2b9b5e0

Add support for notify_on_ok field

7147e71

mgyucht approved these changes Oct 17, 2024

View reviewed changes

alexott added this pull request to the merge queue Oct 18, 2024

Merged via the queue into main with commit b9fb47c Oct 18, 2024
7 checks passed

alexott deleted the new-alert-resource branch October 18, 2024 08:46

rauchy mentioned this pull request Oct 23, 2024

[Release] Release v1.55.0 #4146

Closed

rauchy mentioned this pull request Oct 24, 2024

[Release] Release v1.55.0 #4148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add `databricks_alert` resource to replace `databricks_sql_alert` #4051

[Feature] Add `databricks_alert` resource to replace `databricks_sql_alert` #4051

alexott commented Sep 27, 2024 •

edited

Loading

mgyucht left a comment

mgyucht left a comment

mgyucht Oct 17, 2024

alexott Oct 17, 2024

[Feature] Add databricks_alert resource to replace databricks_sql_alert #4051

[Feature] Add databricks_alert resource to replace databricks_sql_alert #4051

Conversation

alexott commented Sep 27, 2024 • edited Loading

Changes

Tests

mgyucht left a comment

Choose a reason for hiding this comment

mgyucht left a comment

Choose a reason for hiding this comment

mgyucht Oct 17, 2024

Choose a reason for hiding this comment

alexott Oct 17, 2024

Choose a reason for hiding this comment

[Feature] Add `databricks_alert` resource to replace `databricks_sql_alert` #4051

[Feature] Add `databricks_alert` resource to replace `databricks_sql_alert` #4051

alexott commented Sep 27, 2024 •

edited

Loading