-
Notifications
You must be signed in to change notification settings - Fork 135
Create a Sqlite connector utility #1244
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
Conversation
b9811b5 to
1f97353
Compare
|
@austinweisgrau just to clarify, are this and #1243 meant to be tested/reviewed/merged after #1242 is merged? Is that why the tests are failing? Or is that unrelated? |
|
Yes, the tests in this PR use the refactored code in #1242 so this should only be merged after #1242 is approved and merged. The tests are failing because I haven't figured out how to get sqlite properly installed and accessible in the github actions test environment, so that's a work in progress. (Tests should be passing on a local setup, it's a github actions specific issue. I think.) |
|
Although it might be a windows issue specifically since that's the tests that are failing, so that's another avenue to look into |
1a60700 to
68f7175
Compare
|
So this is to connect to a local SQL instance and SQL is not installed within the GitHub Actions environment? I'd suggest an actions step of installing SQL via apt on Linux and winget on Windows. Not. Sure about macOS. |
|
That being said, you might not actually need SQL installed to test the connector properly. There might be a Pytest SQL plugin or something which acts like an SQL instance. You might want to look at the MySQL connector to see how they're doing it there. |
|
The test actually is appropriately failing, I realized. Even though sqlite3 comes with python as part of the stdlib, on windows machines and probably other misc architectures, the sqlite3 cli utility isn't available by default. One of the methods in the connector attempts to use the shell utility through a subprocess, but that won't work if the shell utility isn't installed and on path. I'm going to modify that behavior to check if the shell utility is available first, and use a fallback behavior if it is not. |
896564d to
e148f05
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
yay |
|
cool security check @bmos , very helpful |
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 looks good to me although it might make sense to have someone who uses the database connectors more often (like @IanRFerguson or @sharinetmc or @KasiaHinkson) take a quick look to.
The only thing missing from my perspective is docs. The database utilities/connectors are documented in this file: https://github.com/move-coop/parsons/blob/main/docs/databases.rst
You can see what this looks like from the reader's perspective here:
https://move-coop.github.io/parsons/html/stable/databases.html
responding to a bandit security check which flagged shell=True as vulnerable to injection attacks
Done! |
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.
LGTM!
Should follow #1242
Works with query(), copy(), and the DB sync utilities