-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat!: improve compatibility with databricks sql client #252
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
d2c2475
to
d9ef6f8
Compare
You can run |
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.
Changes look reasonable. Is it worth adding any unit tests along with this change?
# Use this environment variable to determine if the | ||
# client SDK was initialized from a piece of content running on a Connect server. |
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.
Could you move this to a docstring?
# client SDK was initialized from a piece of content running on a Connect server. | ||
def is_local() -> bool: | ||
return not os.getenv("RSTUDIO_PRODUCT") == "CONNECT" | ||
class PositOAuthIntegrationCredentialsStrategy(CredentialsStrategy): |
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.
I know this is superficial, but do we have options for making this class name shorter? As a user, giant names like this scare me and make me assume that the code is super complex.
Would PositCredentialsStrategy
work?
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.
Same here! That or ConnectCredentialsStrategy
.
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.
Just another option in the merry-go-round of word permutations - because this is already under the posit/connect
directory, would it be more informative to call it an OAuthCredentialsStrategy
? Then we could have other credentials strategies that also work for posit products / connect but we don't run into naming collisions.
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.
I opted to go with PositCredentialsStrategy
to avoid a naming conflict with the databricks sdk's OAuthCredentialsStrategy
. I think this is a good balance of brevity and descriptive-ness since we are still under the posit/connect
namespace
a2db2fb
to
6c0b775
Compare
6c0b775
to
bf3c946
Compare
assert cp() == {"Authorization": "Bearer static-pat-token"} | ||
|
||
# posit_strategy is used when the content is running on Connect | ||
os.environ["RSTUDIO_PRODUCT"] = "CONNECT" |
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.
It's possible to mock the environment variables using unittest.mock.patch
:
@patch.dict("os.environ", {"CONNECT_API_KEY": "foobar"}) |
This avoids modifying the developer's environment entirely.
bf3c946
to
710c6b1
Compare
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.
Everything looks good to me. Thanks for the additional documentation, tests, and explanations!
For reviewers: The important changes relevant for this PR's functionality are in
src/posit/connect/external/databricks.py
The new credentials_strategy takes a fallback option that will be used automatically during local development, this way content can run locally and on Connect w/o requiring any code changes.