Skip to content

Update SDK to OpenAPI spec + Fix tests#755

Merged
tanmay-db merged 8 commits intomainfrom
fix-backwards-incompatible-change
Jan 8, 2024
Merged

Update SDK to OpenAPI spec + Fix tests#755
tanmay-db merged 8 commits intomainfrom
fix-backwards-incompatible-change

Conversation

@tanmay-db
Copy link
Copy Markdown
Contributor

@tanmay-db tanmay-db commented Jan 4, 2024

Changes

SDK generation and fixing tests by passing a request when calling ListAll method

Tests

Successfully generated + Integration tests passed except 2 tests in aws-prod-acct due to Credentials limit exceeded

  • make test passing
  • make fmt applied
  • relevant integration tests applied

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 4, 2024

Codecov Report

Attention: 78 lines in your changes are missing coverage. Please review.

Comparison is base (508cfd5) 17.97% compared to head (a94da91) 17.93%.

Files Patch % Lines
service/catalog/api.go 0.00% 35 Missing ⚠️
service/catalog/model.go 0.00% 16 Missing ⚠️
experimental/mocks/mock_workspace_client.go 20.00% 12 Missing ⚠️
service/settings/api.go 0.00% 6 Missing ⚠️
service/settings/impl.go 0.00% 3 Missing ⚠️
service/catalog/impl.go 0.00% 2 Missing ⚠️
service/iam/model.go 0.00% 2 Missing ⚠️
service/sql/model.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
- Coverage   17.97%   17.93%   -0.04%     
==========================================
  Files         108      108              
  Lines       14649    14698      +49     
==========================================
+ Hits         2633     2636       +3     
- Misses      11802    11848      +46     
  Partials      214      214              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread internal/catalog_test.go
assert.NotEqual(t, "", byName.Id)

all, err := w.StorageCredentials.ListAll(ctx)
all, err := w.StorageCredentials.ListAll(ctx, catalog.ListStorageCredentialsRequest{})
Copy link
Copy Markdown
Contributor Author

@tanmay-db tanmay-db Jan 4, 2024

Choose a reason for hiding this comment

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

[Discussion]

This is a breaking change (as golang doesn't support optional parameters) so we will have to mention this explicitly in our release notes or revert the original change in openapi spec for not introducing this.

We could do this change since Go SDK isn't GA yet but it won't be a good user experience i.e sending empty request.

Other non breaking alternatives are:

  • Make ListAll as variadic on ListExternalLocationsRequest. Not sure if this is supported in SDK generator + if there are other different types of parameters in future for this method, then there might be more difficulties in maintenance.
func ListAll(ctx context.Context, requests ...ListExternalLocationsRequest) ([]ExternalLocationInfo, error) {
}
  • Have a wrapper over ListAll -- but this is something we would have to maintain externally.

Both of these are something that we could do to not have breakage. This type of problem is specific to golang and could arise for other methods as well so we would need a solution that will work for other such cases as well without extra overhead.

What do you think? @miles @hectorcast-db

Copy link
Copy Markdown
Contributor Author

@tanmay-db tanmay-db Jan 4, 2024

Choose a reason for hiding this comment

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

+ @pietern for visibility as CLI release is blocked on GoSDK release.

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.

If we decided to not revert the open api change: Java SDK: databricks/databricks-sdk-java#204. There are some issues with Python SDK -- will take a look at them tomorrow.

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.

Right now, since it's not GA we could go forward. List APIs should have pagination, so this is a good change.
But indeed, in general, adding an optional parameter to an API should not be a breaking change.

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.

Copy link
Copy Markdown
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.

Need to make sure .gitattributes is not regressing here.

@tanmay-db tanmay-db requested a review from mgyucht January 8, 2024 12:20
@tanmay-db tanmay-db added this pull request to the merge queue Jan 8, 2024
Merged via the queue into main with commit ffa8405 Jan 8, 2024
@tanmay-db tanmay-db deleted the fix-backwards-incompatible-change branch January 8, 2024 12:53
tanmay-db added a commit that referenced this pull request Jan 9, 2024
* Extract API interfaces for all services and generate mock clients ([#740](#740)).
* Handle empty response for errors ([#756](#756)).
* Update SDK to OpenAPI spec + Fix tests ([#755](#755)).
* Add utility to retry on specific errors ([#757](#757)).

API Changes:

 * Changed `List` method for [w.ExternalLocations](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ExternalLocationsAPI) workspace-level service to require request of [catalog.ListExternalLocationsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListExternalLocationsRequest).
 * Changed `List` method for [w.StorageCredentials](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#StorageCredentialsAPI) workspace-level service to require request of [catalog.ListStorageCredentialsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListStorageCredentialsRequest).
 * Added `NextPageToken` field for [catalog.ListExternalLocationsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListExternalLocationsResponse).
 * Added `MaxResults` field for [catalog.ListFunctionsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListFunctionsRequest).
 * Added `PageToken` field for [catalog.ListFunctionsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListFunctionsRequest).
 * Added `NextPageToken` field for [catalog.ListFunctionsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListFunctionsResponse).
 * Added `MaxResults` field for [catalog.ListSchemasRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListSchemasRequest).
 * Added `PageToken` field for [catalog.ListSchemasRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListSchemasRequest).
 * Added `NextPageToken` field for [catalog.ListSchemasResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListSchemasResponse).
 * Added `NextPageToken` field for [catalog.ListStorageCredentialsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListStorageCredentialsResponse).
 * Added `OmitColumns` field for [catalog.ListTablesRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListTablesRequest).
 * Added `OmitProperties` field for [catalog.ListTablesRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListTablesRequest).
 * Added [catalog.ListExternalLocationsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListExternalLocationsRequest).
 * Added [catalog.ListStorageCredentialsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListStorageCredentialsRequest).
 * Changed `List` method for [w.Tokens](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#TokensAPI) workspace-level service to return [settings.ListPublicTokensResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListPublicTokensResponse).
 * Added [settings.ListPublicTokensResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListPublicTokensResponse).
 * Added [dashboards](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/dashboards) package.
 * Added [vectorsearch](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/vectorsearch) package.

OpenAPI SHA: a7a9dc025bb80303e676bf3708942c6aa06689f1, Date: 2024-01-04
Dependency updates:

 * Bump google.golang.org/api from 0.153.0 to 0.154.0 ([#741](#741)).
 * Bump golang.org/x/crypto from 0.14.0 to 0.17.0 in /examples/slog ([#747](#747)).
 * Bump golang.org/x/crypto from 0.14.0 to 0.17.0 in /examples/zerolog ([#748](#748)).
 * Bump golang.org/x/crypto from 0.16.0 to 0.17.0 ([#749](#749)).
@tanmay-db tanmay-db mentioned this pull request Jan 9, 2024
github-merge-queue Bot pushed a commit that referenced this pull request Jan 10, 2024
* Extract API interfaces for all services and generate mock clients
([#740](#740)).
* Handle empty response for errors
([#756](#756)).
* Update SDK to OpenAPI spec + Fix tests
([#755](#755)).
* Add utility to retry on specific errors
([#757](#757)).
* Integration test fixes for TestMwsAccWorkspaces
([#763](#763)) and
TestMwsAccUsageDownload
([#764](#764)).

Note: This release contains breaking changes, please see below.

API Changes:

* [Breaking] Changed `List` method for
[w.ExternalLocations](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ExternalLocationsAPI)
workspace-level service to require request of
[catalog.ListExternalLocationsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListExternalLocationsRequest),
[w.StorageCredentials](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#StorageCredentialsAPI)
workspace-level service to require request of
[catalog.ListStorageCredentialsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListStorageCredentialsRequest)
and
[w.Tokens](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#TokensAPI)
workspace-level service to return
[settings.ListPublicTokensResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListPublicTokensResponse).
* Added `NextPageToken` field for
[catalog.ListExternalLocationsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListExternalLocationsResponse),
[catalog.ListFunctionsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListFunctionsResponse),
[catalog.ListSchemasResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListSchemasResponse)
and
[catalog.ListStorageCredentialsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListStorageCredentialsResponse).
* Added `MaxResults` field for
[catalog.ListFunctionsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListFunctionsRequest)
and
[catalog.ListSchemasRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListSchemasRequest).
* Added `PageToken` field for
[catalog.ListFunctionsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListFunctionsRequest)
and
[catalog.ListSchemasRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListSchemasRequest).
* Added `OmitColumns` and `OmitProperties` field for
[catalog.ListTablesRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListTablesRequest).
* Added
[catalog.ListExternalLocationsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListExternalLocationsRequest).
* Added
[catalog.ListStorageCredentialsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#ListStorageCredentialsRequest).
* Added
[settings.ListPublicTokensResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListPublicTokensResponse).
* Added
[dashboards](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/dashboards)
package.
* Added
[vectorsearch](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/vectorsearch)
package.

OpenAPI SHA: a7a9dc025bb80303e676bf3708942c6aa06689f1, Date: 2024-01-04
Dependency updates:

* Bump google.golang.org/api from 0.153.0 to 0.154.0
([#741](#741)).
* Bump golang.org/x/crypto from 0.14.0 to 0.17.0 in /examples/slog
([#747](#747)) and
/examples/zerolog
([#748](#748)).
* Bump golang.org/x/crypto from 0.16.0 to 0.17.0
([#749](#749)).
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