Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Add databricks_query resource instead of databricks_sql_query #4103

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

alexott
Copy link
Contributor

@alexott alexott commented Oct 14, 2024

Changes

This PR is built on top of #4051 which should be merged first.

The new resource uses the new Queries 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_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

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

@alexott alexott requested review from a team as code owners October 14, 2024 11:28
@alexott alexott requested review from renaudhartert-db and mgyucht and removed request for a team October 14, 2024 11:28
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems good to me. I'll wait to approve until databricks_alert is merged.

* `description` - (Optional, String) General description that conveys additional information about this query such as usage notes.
* `run_as_mode` - (Optional, String) Sets the "Run as" role for the object.
* `tags` - (Optional, List of strings) Tags that will be added to the query.
* `parameter` - (Optional, Block) Query parameter definition. Consists of following attributes (one of `*_value` is required):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: in other docs, these blocks are often documented as their own headings. Is there any need for consistency around these aspects?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good question - sometimes, moving to a separate block will add an unnecessary context switch. But agree, we need to unify this

docs/resources/query.md Outdated Show resolved Hide resolved
docs/resources/query.md Show resolved Hide resolved
internal/acceptance/query_test.go Outdated Show resolved Hide resolved
sql/resource_query.go Show resolved Hide resolved
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants