ActionNetwork SQL Mirror querying support + general SSH util to query any db through ssh #1102
Conversation
…s-actionnetwork-changes
…s-actionnetwork-changes
…dvocacy Campaigns, Attendances, Campaigns, Custom Fields, Donations, Embeds, Event Campaigns, Events, Forms, Fundraising Pages, Items, Lists, Messages, Metadata, Outreaches, People, Petitions, Queries, Signatures, Submissions, Tags, Taggings, Wrappers)
…s-actionnetwork-changes
…s-actionnetwork-changes
…h function in module)
…s-actionnetwork-changes
…s-actionnetwork-changes
…ng with using the redshift conector there instead of just connecting with psycopg and SSHTunnelForwader
…ng with using the redshift conector there instead of just connecting with psycopg and SSHTunnelForwader2
NirTatcher
changed the title
requirements-dev.txt
Outdated
| @@ -8,3 +8,5 @@ pytest-mock==3.12.0 | |||
| pytest==8.1.1 | |||
| requests-mock==1.11.0 | |||
| testfixtures==8.1.0 | |||
| sshtunnel==0.4.0 | |||
There was a problem hiding this comment.
Remove this, it shouldn't be added to requirements-dev.txt
| except Exception as e: | ||
| logging.error(f"Error during query execution: {e}") |
There was a problem hiding this comment.
Remove this except block - this will prevent exceptions from being raised. It's good to log the exceptions, but generally if an exception occurs here, it would be appropriate to raise it.
There was a problem hiding this comment.
Done, we are now raising the exception after logging it. Hope this is ok and please let me know if you prefer the code without the except block all together.
| finally: | ||
| if con: | ||
| con.close() | ||
| logging.info("Database connection closed.") | ||
| if server: | ||
| server.stop() | ||
| logging.info("SSH tunnel closed.") |
There was a problem hiding this comment.
The finally block is good though
There was a problem hiding this comment.
Technically, querying the ActionNetwork SQL mirror is a new connector. It has different dependencies, it queries data from a different source. I get the impulse to have it as a method on the existing ActionNetwork API connector, but I think that isn't the right move for a few reasons.
- It adds two dependencies to the ActionNetwork connector that API users don't need: the postgres extra and the sshtunnel package
- The method on the ActionNetwork connector to query the SSH mirror doesn't do anything - it wraps the query_through_ssh method without doing anything specific.
I think the query_through_ssh should be added as its own utility as you do here, but the ActionNetwork connector should be left unchanged. Anyone who wants to query the ActionNetwork SQL mirror can use the query_through_ssh method, and nothing needs to be done with the ActionNetwork connector.
There was a problem hiding this comment.
Makes a lot of sense. Just to be sure I've added to the ActionNetwork docs an example of how you would call query_through_ssh to query your SQL Mirror.
| except Exception as e: | ||
| print(e) | ||
| return None |
There was a problem hiding this comment.
Again just in general, avoid this pattern - it results in Exceptions that should be raised not being raised.
There was a problem hiding this comment.
Like you said I deleted this function from the ActionNetwork connector, thank you so much for your time reviewing and giving me feedback I will for sure notice and avoid this pattern of not raising exceptions!
There was a problem hiding this comment.
Since the new method on the ActionNetwork connector doesn't do anything specific, it just calls the query_through_ssh method and passes all arguments to it, there's no need for an additional test. This test can be removed.
There was a problem hiding this comment.
One last thought is that since the SSHTunnel utility adds a new dependency, you'll need to include it as an extra in the setup.py file. I think it would be appropriate to add a new extra called ssh where the dependencies can be included. e.g. the new extra will be:
"ssh": ["sshtunnel", "psycopg2-binary>=2.9.9", "sqlalchemy >= 1.4.22, != 1.4.33, < 2.0.0"]
because you'll need to include the postgres connector's dependencies as well
Done. |
…ActionNetwork docs in our project so people would have all the resources they need
@austinweisgrau, @shaunagm I hope this won't break anything like my other PR, I recreated the utility and made it from a class to just a function we can use to query a DB through a SSH tunnel connection.
Please let me know if there is any feedback or any change request :)
Also, thank you again @austinweisgrau and @shaunagm for everything!