-
Notifications
You must be signed in to change notification settings - Fork 181
Config changes for custom scopes support #1166
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
base: main
Are you sure you want to change the base?
Conversation
b50398b to
4fb6bd0
Compare
4fb6bd0 to
d6a9118
Compare
e9d4523 to
15d443c
Compare
15d443c to
73d2419
Compare
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
| ("sql, offline_access", None), | ||
| ("sql, offline_access", '{"type": "databricks_resource"}'), | ||
| ("sql", None), | ||
| ("sql offline_access all-apis", None), | ||
| ("sql, offline_access, all-apis", None), |
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 is probably a breaking change, to avoid making it we will have to also allow for whitespace as a delimiter as runtime oauth already uses that format.
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.
We should add that in this PR then. I see we keep the tests as they are and add new tests for comma.
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.
Side note: space delimited is the standard according to rfc6749. We may want to consider supporting them in other SDKs too.
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.
👍
| product_version=product_version, | ||
| token_audience=token_audience, | ||
| scopes=" ".join(scopes) if scopes else None, | ||
| scopes=scopes, |
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 is a generated file. Have you changed the template? Ping me the PR for the template update.
|
|
||
| # disable_oauth_refresh_token controls whether a refresh token should be requested | ||
| # during the U2M authentication flow (default to false). | ||
| disable_oauth_refresh_token: bool = ConfigAttribute(env="DATABRICKS_DISABLE_OAUTH_REFRESH_TOKEN") |
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.
Are we using this?
| def test_disable_oauth_refresh_token_defaults_to_false(mocker): | ||
| mocker.patch("databricks.sdk.config.Config.init_auth") | ||
| config = Config(host="https://test.databricks.com") | ||
| assert config.disable_oauth_refresh_token is None # ConfigAttribute returns None when not set |
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.
Why not checking
assert not config.disable_oauth_refresh_token
Is there a case which we want to differentiate between False and Not set?
| ("sql, offline_access", None), | ||
| ("sql, offline_access", '{"type": "databricks_resource"}'), | ||
| ("sql", None), | ||
| ("sql offline_access all-apis", None), | ||
| ("sql, offline_access, all-apis", None), |
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.
We should add that in this PR then. I see we keep the tests as they are and add new tests for comma.
| ("sql, offline_access", None), | ||
| ("sql, offline_access", '{"type": "databricks_resource"}'), | ||
| ("sql", None), | ||
| ("sql offline_access all-apis", None), | ||
| ("sql, offline_access, all-apis", None), |
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.
Side note: space delimited is the standard according to rfc6749. We may want to consider supporting them in other SDKs too.
hectorcast-db
left a comment
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.
A few more tests we could add:
- List as input
- Decuplication
- Edge cases: Empty strings, whitespace-only strings, None values
- Space separated input (backwards compatibility)
Summary
scopesfromstrtolist.scopesis empty.How is this tested?
disable_oauth_refresh_tokenis set when environment variableDATABRICKS_DISABLE_OAUTH_REFRESH_TOKENis set.NO_CHANGELOG=true