-
Notifications
You must be signed in to change notification settings - Fork 107
feat(datasets): support TableDataset credentials
#1218
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(datasets): support TableDataset credentials
#1218
Conversation
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
6f97810 to
74cbed1
Compare
Signed-off-by: gitgud5000 <[email protected]>
…ypy and surface explicit errors for invalid credential inputs. Signed-off-by: gitgud5000 <[email protected]>
05d047d to
13e8b3b
Compare
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
|
@deepyaman Opened a new PR with the credentials feature for Didn’t add connection string support yet... not sure if that should go in a separate PR. |
Yeah, separate PR probably better for that. |
Signed-off-by: gitgud5000 <[email protected]>
…de argument (PR kedro-org#1093) and credentials support in the upcoming changes section. Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
TableDataset credentials
…erging; update docs and tests Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
Signed-off-by: gitgud5000 <[email protected]>
deepyaman
left a comment
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 think this largely looks good, just one comment on fixing a test. Thanks!
…erride case Signed-off-by: gitgud5000 <[email protected]>
| else: | ||
| backend = "duckdb" | ||
| def test_connection_config_with_credentials(self, mocker, table_dataset, key): | ||
| backend = table_dataset._connection_config["backend"] |
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.
Sure, much simpler. 🤦♂️
DimedS
left a comment
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.
thanks, @gitgud5000 and @deepyaman , LGTM!
From PR #1093
credentialsparameter to accept connection info (dictor string URI).Raises a warning if both.credentialsand deprecatedconnectionare providedAdds backend extraction helper and adjusts._describe()to use itImproves documentation to reflect the preferred usage.Description
credentialsParameter:credentialsas the preferred method for specifying Ibis backend connection configurations.A string connection URI.(with optional.constring)Supersedes the olderconnectionparameter.]Warns if both are provided.Updates both._connect()and_describe()to support new functionalityDevelopment notes
ibis.connect()Credentials Support
credentialsparam to init signature and docstring.constring.Checklist
jsonschema/kedro-catalog-X.XX.jsonif necessaryRELEASE.mdfile