Defer fetching role and warehouse until they're needed #1631
Conversation
| if cli_context.connection.warehouse: | ||
| self.default_warehouse = to_identifier(cli_context.connection.warehouse) | ||
| self._default_role = self._UNSET | ||
| self._default_warehouse = self._UNSET |
There was a problem hiding this comment.
self.default_warehouse wasn't used externally, so it's safe to change it to self._default_warehouse
sfc-gh-fcampbell
force-pushed
the
frank-defer-role-warehouse
branch
from
bb3394b
to
c0d90c6
Compare
sfc-gh-fcampbell
force-pushed
the
frank-defer-role-warehouse
branch
2 times, most recently
from
fe74040
to
c09aaa6
Compare
| self._default_role = self._UNSET | ||
| self._default_warehouse = self._UNSET | ||
|
|
||
| def _get_default_role(self): |
There was a problem hiding this comment.
nit: could you add a return type annotation? I think it's important here to label whether the return value is optional or not
| if self._default_warehouse is self._UNSET: | ||
| cli_context = get_cli_context() | ||
| if cli_context.connection.warehouse: | ||
| self._default_warehouse = to_identifier( | ||
| cli_context.connection.warehouse | ||
| ) | ||
| return self._default_warehouse |
There was a problem hiding this comment.
there is no connection warehouse, isn't this going to return UNSET? That doesn't sound right. The role logic doesn't have this issue because if assigns None even if the fallback logic fails to produce a valid role.
There was a problem hiding this comment.
@sfc-gh-bdufour good catch, I updated the implementation to use @cached_property in the ActionContext since we don't need to be caching them in WorkspaceManager
sfc-gh-fcampbell
force-pushed
the
frank-defer-role-warehouse
branch
2 times, most recently
from
e8a07c1
to
06d701a
Compare
sfc-gh-fcampbell
force-pushed
the
frank-defer-role-warehouse
branch
from
06d701a
to
5e54a5c
Compare
We have some commands, like `snow app bundle`/`snow ws bundle` that don't even need a connection, so let's defer connecting to Snowflake until it's actually needed by the particular action being run. For now, all `snow ws` commands are still marked as requiring a connection, that will be fixed separately.
Pre-review checklist
Changes description
We have some commands, like
snow app bundle/snow ws bundlethat don't even need a connection, so let's defer connecting to Snowflake until it's actually needed by the particular action being run.For now, all
snow wscommands are still marked as requiring a connection, that will be fixed separately.