Skip to content

fix: resolved issue with run_as_role not being applied on databricks_sql_query resource#2940

Merged
alexott merged 1 commit intodatabricks:masterfrom
KOR-Financial:fix/2939_sql_query_run_as_role
Nov 22, 2023
Merged

fix: resolved issue with run_as_role not being applied on databricks_sql_query resource#2940
alexott merged 1 commit intodatabricks:masterfrom
KOR-Financial:fix/2939_sql_query_run_as_role

Conversation

@SlevinBE
Copy link
Copy Markdown
Contributor

Fix for: #2939

Changes

The run_as_role property wasn't being applied correctly on a databricks_sql_query resource, as documented in #2939. This PR tries to solve this issue by aligning the TF code with the Databricks Query API, so that run_as_role is only specified and queried from the top-level object, and not from the options block anymore.

Tests

Specifically, the sql/resource_sql_query_test.go was updated and run to verify this fix. A proper integration test run is still required to validate the fix.

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

…sql_query resource

Co-authored-by: Matthias De Vriendt <mdevriendt@korfinancial.com>
@SlevinBE SlevinBE requested review from a team as code owners November 21, 2023 10:02
@SlevinBE SlevinBE requested review from hectorcast-db and removed request for a team November 21, 2023 10:02
Comment thread sql/api/query.go
Query string `json:"query"`
// Deprecated: Use databricks_job resource to schedule a Query
Schedule *QuerySchedule `json:"schedule"`
RunAsRole string `json:"run_as_role,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the API fills run_as_role with default owner, then it makes sense to add suppress_diff, like this: https://github.com/databricks/terraform-provider-databricks/blob/master/sql/resource_sql_dashboard.go#L19

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi Alex, can you explain what the suppress_diff exactly does? I've read the documentation in the Contributor docs, but it's not quite clear to me yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's suppressing the diff from a default value to an empty string. It's needed because if you declare the resource without run_as_role, then API will fill it with default owner, but when you run apply next time, because run_as_role is empty in the query, then it will be detected as drift, and will try to reset it back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, makes sense. However, suppress_diff is already defined on the QueryEntity's RunAsState property. Isn't the Query api object you're referencing here used to represent the Databricks API objects, so unrelated to the Terraform state, and therefore requiring no suppress_diff config here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, sorry - misread it

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #2940 (9c8a680) into master (2574e7c) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2940      +/-   ##
==========================================
- Coverage   83.68%   83.68%   -0.01%     
==========================================
  Files         153      153              
  Lines       13612    13610       -2     
==========================================
- Hits        11391    11389       -2     
  Misses       1593     1593              
  Partials      628      628              
Files Coverage Δ
sql/api/query.go 75.40% <ø> (ø)
sql/resource_sql_query.go 95.81% <100.00%> (-0.03%) ⬇️

Copy link
Copy Markdown
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

thank you for contribution!

Comment thread sql/api/query.go
Query string `json:"query"`
// Deprecated: Use databricks_job resource to schedule a Query
Schedule *QuerySchedule `json:"schedule"`
RunAsRole string `json:"run_as_role,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, sorry - misread it

@alexott alexott added this pull request to the merge queue Nov 22, 2023
Merged via the queue into databricks:master with commit ce9e324 Nov 22, 2023
@hectorcast-db hectorcast-db mentioned this pull request Dec 5, 2023
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.

4 participants