Skip to content

Conversation

@vdonato
Copy link
Collaborator

@vdonato vdonato commented Apr 17, 2023

📚 Context

This one's pretty self-explanatory. In some runtime environments, we'll already have an active
Snowpark session available (grabbing it is easy thanks to the Snowpark library's get_active_session
function). We just want to use this active session when possible.

@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label Apr 17, 2023
@vdonato vdonato requested review from kajarenc and lukasmasuch April 17, 2023 23:44
@vdonato vdonato force-pushed the vdonato/snowpark-sis branch 2 times, most recently from 9a0b89c to e684760 Compare April 18, 2023 00:30
@vdonato vdonato force-pushed the vdonato/snowpark-sis branch from d979553 to 73b3c87 Compare April 18, 2023 00:53
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

# available, we just use that one. Otherwise, we fall back to attempting to
# create a new one from whatever credentials we have available.
try:
return get_active_session()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a situation where the session returned by get_active_session is actually not what the user is trying to connect to? E.g. if the user is logged in with a certain account/role, but actually wants to use a different one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not as far as I understand -- @sfc-gh-jcarroll might know if there's any weirdness that can go on here, though

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not aware of any either, I think this will work. Might be worth tagging Yijun on the main PR to look at the Snowpark connection all up for any concerns like this?

@vdonato vdonato merged this pull request into feature/st.experimental_connection Apr 19, 2023
@vdonato vdonato deleted the vdonato/snowpark-sis branch April 19, 2023 19:57
vdonato added a commit that referenced this pull request Apr 19, 2023
* Use the current active Snowpark session if one is available

* Only run Snowpark tests in Snowflake CI tests

* Add `type: ignore`s
vdonato added a commit that referenced this pull request Apr 20, 2023
* Use the current active Snowpark session if one is available

* Only run Snowpark tests in Snowflake CI tests

* Add `type: ignore`s
vdonato added a commit that referenced this pull request Apr 21, 2023
* Use the current active Snowpark session if one is available

* Only run Snowpark tests in Snowflake CI tests

* Add `type: ignore`s
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants