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.

Sign up for GitHub

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

Jump to bottom

[Fix] Set ID for online table resource if creation succeeds but it isn't available yet #4072

Merged
merged 2 commits into from
Oct 4, 2024
Merged

[Fix] Set ID for online table resource if creation succeeds but it isn't available yet #4072

merged 2 commits into from
Oct 4, 2024

Conversation

tanmay-db
Copy link
Contributor

@tanmay-db tanmay-db commented Oct 4, 2024 •
edited
Loading

Changes

We should set the id right after creation and before waiting for online table to be available. This is because in case when online table isn't available, we still should have that resource in the state.

Also the timeout has been increased to 2x (I am going to following up with online tables team for suitable timeout but since we have to do a release, going ahead with small time increase should be good)

Note: We should add setting id right after creation for similar resources to CONTRIBUTING guide (which I will do in a separate PR)

Tests

Added unit test to check that pathway, id is set (which wasn't the case before)

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

@tanmay-db tanmay-db requested review from a team as code owners October 4, 2024 11:26
@tanmay-db tanmay-db requested review from hectorcast-db and removed request for a team October 4, 2024 11:26
@alexott
Copy link
Contributor

alexott commented Oct 4, 2024

if it will fail to initialize online table during waiting, it will be in the broken state. We really need to get the definitive answer from the dev team about condition when table is considered successfully created and initialized.

@tanmay-db
Copy link
Contributor Author

@alexott that is true but the resource would still be there in the failed state and we should put this in the state even if the resources is not available (but creation succeeds).

@tanmay-db tanmay-db requested review from mgyucht and alexott October 4, 2024 11:52
mgyucht
mgyucht approved these changes Oct 4, 2024
View reviewed changes
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.

LGTM, just a small suggestion on the comment.

Could you add a note about this in the contributing guide?

@tanmay-db
Copy link
Contributor Author

Hi @mgyucht, yes regarding contributing guide, I will be doing that in a separate PR so we isolate the changes.

@tanmay-db @mgyucht
Update catalog/resource_online_table.go
5c6d82c 
Co-authored-by: Miles Yucht <miles@databricks.com>
@tanmay-db tanmay-db enabled auto-merge October 4, 2024 12:34
@tanmay-db tanmay-db added this pull request to the merge queue Oct 4, 2024
Merged via the queue into main with commit 60b8a6f Oct 4, 2024
7 checks passed
@tanmay-db tanmay-db deleted the fix-online-tables branch October 4, 2024 12:40
parthban-db added a commit that referenced this pull request Oct 6, 2024
@parthban-db
[Release] Release v1.53.0
dc6819d 
### New Features and Improvements

 * Add `databricks_budget` resource ([#3955](#3955)).
 * Add `databricks_mlflow_models` data source ([#3874](#3874)).
 * Add computed attribute `table_serving_url` to `databricks_online_table` ([#4048](#4048)).
 * Add support for Identity Column in `databricks_sql_table` ([#4035](#4035)).

### Bug Fixes

 * Fix Permissions Dashboard Test ([#4071](#4071)).
 * Ignore presence or absence of `/Workspace` prefix for dashboard resource ([#4061](#4061)).
 * Refactor `databricks_permissions` and allow the current user to set their own permissions ([#3956](#3956)).
 * Set ID for online table resource if creation succeeds but it isn't available yet ([#4072](#4072)).

### Documentation

 * Add guide for OIDC authentication ([#4016](#4016)).
 * Correctly use native markdown callouts supported by TF Registry ([#4073](#4073)).
 * Fixing links to `databricks_service_principal` in TF guides ([#4020](#4020)).

### Internal Changes

 * Bump Go SDK to latest and generate TF structs  ([#4062](#4062)).
 * Skip Budget tests on GCP ([#4070](#4070)).
 * Update to latest OpenAPI spec and bump Go SDK ([#4069](#4069)).
@parthban-db parthban-db mentioned this pull request Oct 6, 2024
Merged
parthban-db added a commit that referenced this pull request Oct 7, 2024
@parthban-db
[Release] Release v1.53.0
318dc65 
### New Features and Improvements

 * Add `databricks_budget` resource ([#3955](#3955)).
 * Add `databricks_mlflow_models` data source ([#3874](#3874)).
 * Add computed attribute `table_serving_url` to `databricks_online_table` ([#4048](#4048)).
 * Add support for Identity Column in `databricks_sql_table` ([#4035](#4035)).

### Bug Fixes

 * Fix Permissions Dashboard Test ([#4071](#4071)).
 * Ignore presence or absence of `/Workspace` prefix for dashboard resource ([#4061](#4061)).
 * Refactor `databricks_permissions` and allow the current user to set their own permissions ([#3956](#3956)).
 * Set ID for online table resource if creation succeeds but it isn't available yet ([#4072](#4072)).

### Documentation

 * Add guide for OIDC authentication ([#4016](#4016)).
 * Correctly use native markdown callouts supported by TF Registry ([#4073](#4073)).
 * Fixing links to `databricks_service_principal` in TF guides ([#4020](#4020)).

### Internal Changes

 * Bump Go SDK to latest and generate TF structs  ([#4062](#4062)).
 * Skip Budget tests on GCP ([#4070](#4070)).
 * Update to latest OpenAPI spec and bump Go SDK ([#4069](#4069)).
github-merge-queue bot pushed a commit that referenced this pull request Oct 7, 2024
@parthban-db
[Release] Release v1.53.0 (#4076)
2557380 
### New Features and Improvements

* Add `databricks_budget` resource
([#3955](#3955)).
* Add `databricks_mlflow_models` data source
([#3874](#3874)).
* Add computed attribute `table_serving_url` to
`databricks_online_table`
([#4048](#4048)).
* Add support for Identity Column in `databricks_sql_table`
([#4035](#4035)).


### Bug Fixes

* Add Sufficient Network Privileges to the Databricks Default Cross
Account Policy
([#4027](#4027))
* Ignore presence or absence of `/Workspace` prefix for dashboard
resource
([#4061](#4061)).
* Refactor `databricks_permissions` and allow the current user to set
their own permissions
([#3956](#3956)).
* Set ID for online table resource if creation succeeds but it isn't
available yet
([#4072](#4072)).


### Documentation

* Update CONTRIBUTING guide for plugin framework resources
([#4078](#4078))
* Add guide for OIDC authentication
([#4016](#4016)).
* Correctly use native markdown callouts supported by TF Registry
([#4073](#4073)).
* Fixing links to `databricks_service_principal` in TF guides
([#4020](#4020)).


### Internal Changes

* Fix Permissions Dashboard Test
([#4071](#4071)).
* Bump Go SDK to latest and generate TF structs
([#4062](#4062)).
* Skip Budget tests on GCP
([#4070](#4070)).
* Update to latest OpenAPI spec and bump Go SDK
([#4069](#4069)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgyucht mgyucht mgyucht approved these changes

@hectorcast-db hectorcast-db Awaiting requested review from hectorcast-db hectorcast-db is a code owner automatically assigned from databricks/eng-plat-auto-exp-reviewers

@alexott alexott Awaiting requested review from alexott

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tanmay-db @alexott @mgyucht