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

Migrate client to github.com/databricks/databricks-sdk-go #1848

Merged
merged 27 commits into from
Feb 20, 2023
Merged

Migrate client to github.com/databricks/databricks-sdk-go #1848

merged 27 commits into from
Feb 20, 2023

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Dec 14, 2022 •
edited
Loading

This PR starts migration to https://github.com/databricks/databricks-sdk-go by replacing the configuration and HTTP client layer with the one defined in Go SDK. SDK inherits a lot of original terraform client implementations.

@nfx nfx marked this pull request as ready for review February 16, 2023 12:12
@nfx nfx requested review from pietern and alexott February 16, 2023 12:20
@nfx
Copy link
Contributor Author

nfx commented Feb 16, 2023

current status:

DONE 1239 tests, 79 skipped, 15 failures in 20.123s

@nfx nfx mentioned this pull request Feb 16, 2023
Closed
@nfx
Copy link
Contributor Author

nfx commented Feb 16, 2023

current status:

DONE 1238 tests, 79 skipped, 4 failures in 18.152s

@nfx nfx mentioned this pull request Feb 16, 2023
Merged
@nfx
Copy link
Contributor Author

nfx commented Feb 17, 2023

current status (with go sdk v0.3.1 upstream)

DONE 1238 tests, 79 skipped in 16.687s

nfx added a commit to databricks/databricks-sdk-go that referenced this pull request Feb 17, 2023
@nfx
Skip loading config if auth is already explicitly configured directly (…
e1242ef 
…#306)

Fix #304

This issue unblocks
databricks/terraform-provider-databricks#1848
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2023 •
edited
Loading

Codecov Report

Merging #1848 (22b9012) into master (6c943f0) will decrease coverage by 0.82%.
The diff coverage is 58.47%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1848      +/-   ##
==========================================
- Coverage   90.33%   89.51%   -0.82%     
==========================================
  Files         146      140       -6     
  Lines       11911    11038     -873     
==========================================
- Hits        10760     9881     -879     
- Misses        743      767      +24     
+ Partials      408      390      -18
Impacted Files Coverage Δ
catalog/resource_grants.go 96.22% <0.00%> (ø)
clusters/resource_library.go 84.84% <0.00%> (ø)
common/env.go 100.00% <ø> (+4.34%) ⬆️
exporter/context.go 86.58% <ø> (-0.04%) ⬇️
internal/acceptance/acceptance.go 0.00% <0.00%> (ø)
sql/resource_sql_global_config.go 85.18% <0.00%> (ø)
common/client.go 42.35% <39.47%> (-55.31%) ⬇️
access/resource_sql_permissions.go 86.11% <50.00%> (ø)
mws/data_mws_workspaces.go 83.33% <50.00%> (ø)
storage/adls_gen1_mount.go 97.01% <50.00%> (-2.99%) ⬇️
... and 38 more

alexott
alexott approved these changes Feb 17, 2023
View reviewed changes
Copy link
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.

lgtm

aws/resource_instance_profile_test.go Show resolved Hide resolved
@alexott
Copy link
Contributor

alexott commented Feb 17, 2023

Need to fix test although

pietern
pietern reviewed Feb 20, 2023
View reviewed changes
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

This removes a few acceptance tests that look legit, most notably the storage ones.

The change in error message on invalid authentication is not optimal. The original message hinted at the provider configuration where the new one doesn't. I think it's worth intercepting and wrapping it to stick to the original message.

aws/resource_instance_profile_test.go Show resolved Hide resolved
common/client.go Outdated Show resolved Hide resolved
common/resource.go Outdated Show resolved Hide resolved
docs/data-sources/catalogs.md Show resolved Hide resolved
internal/acceptance/acceptance_test.go Outdated Show resolved Hide resolved
provider/provider_test.go Show resolved Hide resolved
secrets/acceptance/secret_scope_test.go Show resolved Hide resolved
storage/adls_gen1_mount.go Show resolved Hide resolved
storage/acceptance/azure_adls_gen2_mount_test.go Show resolved Hide resolved
tokens/resource_obo_token_test.go Show resolved Hide resolved
@nfx nfx mentioned this pull request Feb 20, 2023
Open
@nfx nfx changed the title Migrate to Go SDK client Migrate client to github.com/databricks/databricks-sdk-go Feb 20, 2023
@nfx nfx merged commit 1c18f51 into master Feb 20, 2023
@nfx nfx deleted the migrate/sdk branch February 20, 2023 11:57
This was referenced Feb 20, 2023
Merged
Merged
@BostjanBozic BostjanBozic mentioned this pull request Feb 24, 2023
Closed
@nkvuong nkvuong mentioned this pull request Apr 4, 2023
Closed
@ehab-eb ehab-eb mentioned this pull request May 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietern pietern pietern left review comments

@alexott alexott alexott approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@nfx @codecov-commenter @alexott @pietern