-
Notifications
You must be signed in to change notification settings - Fork 386
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
Resources for SQL Analytics queries and dashboards #553
Conversation
Codecov Report
@@ Coverage Diff @@
## master #553 +/- ##
==========================================
+ Coverage 82.55% 82.64% +0.08%
==========================================
Files 79 85 +6
Lines 7032 7765 +733
==========================================
+ Hits 5805 6417 +612
- Misses 812 878 +66
- Partials 415 470 +55
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of changes requested :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR drops coverage by 19% - https://app.codecov.io/gh/databrickslabs/terraform-provider-databricks/compare/553/tree/sqlanalytics, please bring code coverage to the level of resource_sql_endpoint.go
.
The other 3 things:
- comment in api package
- please add an acceptance test for each resource, like in https://github.com/databrickslabs/terraform-provider-databricks/blob/master/sqlanalytics/acceptance/sql_endpoint_test.go - i want to be sure that none will break it :)
- please add documentation for each resource, like in https://github.com/databrickslabs/terraform-provider-databricks/blob/master/docs/resources/sql_endpoint.md - how else users will know about it? :)
sqlanalytics/api/wrapper.go
Outdated
} | ||
|
||
// NewWrapper creates and returns an API wrapper. | ||
func NewWrapper(ctx context.Context, m interface{}) Wrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split Wrapper
into VisualizationAPI
, QueryAPI
, WidgetAPI
structs and move entities and API wrappers to respective resource files, e.g. resource_widget.go will have NewWidgetsAPI
factory, WidgetsAPI
struct, WidgetEntity
terraform entity for reflection, Widget
api entity, and ResourceWidget
. The main reason for this is that we have 82%+ code coverage requirement for all code that is written and gotestsum
calculates coverage on the package level. I've figured it, when i was confident that reflect_resource is pretty well tested by other packages, but the coverage report didn't show any good results for it. api package coverage is missing from the report, meaning that there are long-term risks of changes going untested to production. This may not be ideal, but it is consistent with the rest of the codebase. Even the special package, dedicated to Accounts API only, follows the same conventions as packages that create resources within the workspace. We may reconsider package layout conventions if we want to have databricks-go-sdk and refactor all 7 packages in a separate pull request, adjusting resource.RetryContext
along the way.
acceptance tests should have |
Output of
The pipeline failure is a known problem (erroneous template). The SQLA failures are all caused by a non-empty plan for the SQL endpoint:
|
New resources are: * `databricks_sql_query` * `databricks_sql_visualization`
If it isn't set, it's not possible to erase an existing description.
This commit makes it match the style of the visualization and widget resources, after they were converted to use composite IDs.
This commit makes it match the style of the visualization and widget resources, after they were converted to use composite IDs.
New resources are:
databricks_sql_query
databricks_sql_visualization
databricks_sql_dashboard
databricks_sql_widget