-
Notifications
You must be signed in to change notification settings - Fork 225
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/integrate connections toml #298
base: master
Are you sure you want to change the base?
Feat/integrate connections toml #298
Conversation
@sfc-gh-tmathew and @sfc-gh-jhansen, flagging a few 4.0.0 candidates. I'm especially interested in this one, as it both simplifies the implementation and supports connections.toml configuration. |
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.
Wow, very impressed at the amount of work here. I'm not a schemachange maintainer, but would benefit from this. Left some suggestions.
Also, I haven't spent a lot of time in the code of the Python Connector, but it seems like schemachange is doing a lot of work around the config and connection that should be handled by the Connector. I wonder if there are ways to lean on it more heavily, allowing a lot of this schemachange code to be cut out.
Thanks, hoping this gets merged sooner or later!
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 class and methods are pretty huge; you might consider refactoring to sub-classes for the different types of configuration to split up the logic. That could certainly be a follow-up pull request, to keep this one from getting more complex.
"snowflake_password": connection.get("password"), | ||
"snowflake_private_key_path": connection.get("private-key"), | ||
"snowflake_token_path": connection.get("token_file_path"), | ||
} |
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 has a lot of overlap with DeployConfig's get_session_kwargs()
, so wondering if it makes sense to centralize the logic. If that doesn't make sense, would at least suggest moving it to a standalone function.
change_history_table="some_history_table", | ||
query_tag="some_query_tag", | ||
oauth_config={"some": "values"}, | ||
) |
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 suggest moving these factory()
calls to fixtures to make the tests simpler.
@afeld, that's a great point. I maintained some legacy "check for required arguments" logic, but we could forego this and pass snowflake-related arguments to the Python Connector. |
Support configuration via a connections.toml file.
Previously, credential and required argument logic was distributed across different classes and didn't consider a connections.toml file. This pull request combines configuration and credential logic, supporting an easy-to-follow order of preference for different sources. The
get_merged_config
function pulls from the following sources with descending preference:The
check_for_required_args
logic is authentication-method specific.The following cli arguments have been added:
Limitions / Concerns
config.toml
and the Snowflake Python connector only supports aconnections.toml
. There's a real lack of parity across the tools here.