[Task]: Finish adding Postgres Integration to Analytics Library #72
Conversation
analytics/config.py
Outdated
| ] | ||
| database_url="postgresql://user:password@location:port/dbname" # get vars from the api? | ||
| , |
There was a problem hiding this comment.
If you've got access to env vars here, could build this like we do for the flask API:
This just takes in a config object that is loading env vars, defined in: https://github.com/navapbc/simpler-grants-gov/blob/main/api/src/adapters/db/clients/postgres_config.py
Handles IAM auth as well.
Note, you might want the start of this to be postgresql+psycopg to tell it to use the latest version of psycopg (I think it defaults to psycopg2? They named the 3rd version just psycopg, its weird): https://docs.sqlalchemy.org/en/20/dialects/postgresql.html#module-sqlalchemy.dialects.postgresql.psycopg
| A SQLAlchemy engine object representing the connection to the database. | ||
| """ | ||
|
|
||
| return create_engine(settings.database_url, pool_pre_ping=True) |
There was a problem hiding this comment.
If we have any PII data, we should set hide_parameters=True so that any DB errors don't dump the contents of our queries to the logs.
If we want connection pooling (no idea the run pattern of this tool), can see some of that in the API implementation of this: https://github.com/navapbc/simpler-grants-gov/blob/main/api/src/adapters/db/clients/postgres_client.py#L49
|
Food for thought: The analytics linter is failing in two places; a docstring and a command due to length (both are over 100 char) Splitting the command is going to break it, and I haven't been able to split the docstrings any further than what is present. Is there a way to tweak this check? |
|
analytics/src/analytics/cli.py
Outdated
| connection = engine.connect() | ||
|
|
||
| # Test INSERT INTO action | ||
| # splitting this to appease E501 Line too long (184 > 100 characters will break the command |
There was a problem hiding this comment.
For what its worth, you can split this without breaking the command, you use a multi-line string, eg.
text(
"INSERT INTO audit_log (topic,timestamp, end_timestamp, user_id, details) "
"VALUES('test', ' 2024-06-11 10:41:15+00', '2024-06-11 10:54:15+00', 87654, 'test from command');",
)Two strings next to each other creates a multi-line string.
You can also of course use no-qa on the linter.
| A SQLAlchemy engine object representing the connection to the database. | ||
| """ | ||
| return create_engine( | ||
| f"postgresql+psycopg://{settings.user}:{settings.password}@{settings.host}:{settings.port}", |
There was a problem hiding this comment.
⭐ (very much optional, just nice to have) recommend testing and documenting exactly how to override these settings with environment variables.
analytics/settings.toml
Outdated
| NAME = "app" | ||
| HOST = "0.0.0.0" | ||
| USER = "app" | ||
| PORT = 5432 |
There was a problem hiding this comment.
⭐⭐ (optional but seriously consider) recommend prefixing all of these with POSTGRES_, eg. POSTGRES_USER
https://github.com/navapbc/simpler-grants-gov/blob/main/analytics/pyproject.toml#L53 - it might also be worth adjusting the rules we have enabled. Whoever setup the analytics code enabled everything in ruff and then ignored a few. In the API, we did the opposite (enabled specific rules), partially because the tool we were using before had fewer rules, and partially due to not agreeing with / not wanting to fix 1000+ issues. |
| "INSERT INTO audit_log (topic,timestamp, end_timestamp, user_id, details)" | ||
| "VALUES('test','2024-06-11 10:41:15','2024-06-11 10:54:15',87654,'test from command');", |
There was a problem hiding this comment.
| "INSERT INTO audit_log (topic,timestamp, end_timestamp, user_id, details)" | |
| "VALUES('test','2024-06-11 10:41:15','2024-06-11 10:54:15',87654,'test from command');", | |
| "INSERT INTO audit_log (topic,timestamp, end_timestamp, user_id, details) " | |
| "VALUES('test','2024-06-11 10:41:15','2024-06-11 10:54:15',87654,'test from command');", |
Multiline strings don't turn newlines into spaces by default, so you need to add a trailing space on each line
|
Expected Failures: These require Slack Auth |
Fixes #45 * update `config.py` database url * add function in `cli.py` * updated packages in `poetry.lock` N/A Row created manually in the database alongside a row created via `test_connection` 
Fixes #45 * update `config.py` database url * add function in `cli.py` * updated packages in `poetry.lock` N/A Row created manually in the database alongside a row created via `test_connection` 
…pbc#72) Fixes #45 * update `config.py` database url * add function in `cli.py` * updated packages in `poetry.lock` N/A Row created manually in the database alongside a row created via `test_connection` 
Summary
Fixes #45
Time to review: 3 mins
Changes proposed
config.pydatabase urlcli.pypoetry.lockContext for reviewers
N/A
Additional information
Row created manually in the database alongside a row created via
test_connection