feat!: Implement snowflake auth helpers #268
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
| def __init__(self, posit_oauth: OAuthIntegration, user_session_token: str): | ||
| self.posit_oauth = posit_oauth | ||
| self.user_session_token = user_session_token | ||
| def __init__(self, client: Client, user_session_token: str): |
There was a problem hiding this comment.
breaking: PositCredentialsProvider now accepts a Client instead of an OAuthIntegration resource. This fits better with the API changes that @zackverham is about to make.
| local_strategy: CredentialsStrategy, | ||
| user_session_token: Optional[str] = None, | ||
| client: Optional[Client] = None, | ||
| user_session_token: Optional[str] = None, |
There was a problem hiding this comment.
potentially breaking? These are named arguments but I did change the order. Callers who are using args instead of kwargs may break
| self.user_session_token = user_session_token | ||
| def __init__(self, client: Client, user_session_token: str): | ||
| self._client = client | ||
| self._user_session_token = user_session_token |
There was a problem hiding this comment.
breaking: These fields are now _internal
There was a problem hiding this comment.
Thanks! If you can capture the breaking changes in the PR description, it will be easy to pull into the release notes.
| self.client = client | ||
| self._local_strategy = local_strategy | ||
| self._client = client | ||
| self._user_session_token = user_session_token |
There was a problem hiding this comment.
breaking: These fields are now _internal
dbkegley
changed the title
dbkegley
changed the title
dbkegley
force-pushed
the
kegs-snowflake
branch
from
66b7099 to
9ba7992
Compare
|
not sure if this has come up anywhere else - but currently we don't indicate w/ the directory structure that our examples are specifically for the oauth integration pieces of the SDK. As we're breaking out sub-directories for the type of integration (snowflake vs databricks) I wonder if we need to do more to flag exactly what these are examples of? |
These are well suited for cookbook recipes or extensions as well. |
I'm happy to start moving these over to cookbooks before the Sept Connect release. I think we should have at least 1 release where they exist in both places and the README in the examples dir should start pointing folks toward the cookbooks ASAP. Next Connect release we can remove the examples from the SDK and just maintain the cookbooks going forward. |
Awesome. Yes, I agree. It's best to hold off on deleting them until the recipes make it to docs.posit.co. |
tdstein
left a comment
tdstein
left a comment
There was a problem hiding this comment.
Looks great overall! I have a few minor questions and comments.
| def _is_local() -> bool: | ||
| """Returns true if called from a piece of content running on a Connect server. | ||
| The connect server will always set the environment variable `RSTUDIO_PRODUCT=CONNECT`. | ||
| We can use this environment variable to determine if the content is running locally | ||
| or on a Connect server. | ||
| """ | ||
| return not os.getenv("RSTUDIO_PRODUCT") == "CONNECT" |
There was a problem hiding this comment.
I haven't found a clear answer to best practices, but I've recently started reducing init.py to only include named exports. Then placing any shared module code in a file of the same name; external/external.py here.
This also eliminates the need to mark the method as private since it won't be declared in the init.py file.
| self._client = client | ||
| self._user_session_token = user_session_token | ||
|
|
||
| def authenticator(self) -> Optional[str]: |
There was a problem hiding this comment.
Should this be a @property?
| self.user_session_token = user_session_token | ||
| def __init__(self, client: Client, user_session_token: str): | ||
| self._client = client | ||
| self._user_session_token = user_session_token |
There was a problem hiding this comment.
Thanks! If you can capture the breaking changes in the PR description, it will be easy to pull into the release notes.
- use @Property for getter methods - move _is_local to external.py module - remove old .qmd - add examples README.md pointing to cookbook
databricksandsnowflakesub-directoriesexternal.snowflakehelpers_is_localto__init__.pyBreaking changes:
posit.connect.external.PositCredentialsProvider.__init__()now accepts aClientinstead of anOAuthIntegrationresource.posit.connect.external.PositCredentialsProviderandposit.connect.external.PositCredentialsStrategyare now_internalposit.connect.external.PositCredentialsStrategy.__init__(). Only breaking for callers who use*argsinstead of**kwargscloses #269