-
Notifications
You must be signed in to change notification settings - Fork 123
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
Bump python sdk version #827
base: 1.10.latest
Are you sure you want to change the base?
Conversation
…s into bump_python_sdk_version
…s into bump_python_sdk_version
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.
Looks good.
@@ -557,8 +556,7 @@ def create( | |||
http_headers = credentials.get_all_http_headers( | |||
connection_parameters.pop("http_headers", {}) | |||
) | |||
credentials_provider = credentials.authenticate(None) | |||
header_factory = credentials_provider(None) # type: ignore | |||
header_factory = credentials.authenticate().credentials_provider() # type: ignore |
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.
Do we still need the # type: ignore
?
elif self.azure_client_id and self.azure_client_secret: | ||
self._config = self.authenticate_with_azure_client_secret() | ||
elif not self.client_secret: | ||
self._config = self.authenticate_with_external_browser() |
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.
@ghjklw I think we will get there eventually; when we did internal design review for this migration path, we wanted to make sure we didn't break any users. At the time, we didn't have behavior flags, so we took this middle ground to achieve that goal. But it would be great to get to the point of unified client auth. Part of what has come out of this effort is getting some more of stuff that we had as custom code in dbt-databricks into the SDK auth stack :). At this point I'm focused on helping Eric finish off this PR, but unified client auth is probably in the future.
Upgrade python sdk version from 0.17.0 to 0.36.0
Create DatabricksCredentialManager to managed credentials, under the hood use Config from databricks.sdk.core to manage the auth part which comes from the Python SDK.
For dbt we support 4 different auth mode:
For oauth-m2m and azure-client-secret we set a preferred auth sequence based on if the clientSecret starts with "dose", so we can reduce the number of false trying as much as possible.
Remove the set/get sharedPassword part, since the python sdk already handles that, but with one defect which I already raise a PR against: databricks/databricks-sdk-py#823
With this change one thing that will no longer work is if customer are using non-databricks OAuth client for U2M case it will not work, since the python sdk does not support modify redirectUrl/scopes.
Description
Checklist
CHANGELOG.md
and added information about my change to the "dbt-databricks next" section.