-
Notifications
You must be signed in to change notification settings - Fork 0
[Task] Update analytics db to use local.env and Remove Dynaconf from Analytics #136
Conversation
analytics/config.py
Outdated
slack_bot_token: str = Field(alias="SLACK_BOT_TOKEN") | ||
reporting_channel_id: str = Field(alias="REPORTING_CHANNEL_ID") |
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.
⭐⭐⭐ (must change before approval) These two need to retain their ANALYTICS_
prefix, or you need to change the terraform env var: https://github.com/navapbc/simpler-grants-gov/blob/main/infra/analytics/app-config/env-config/environment-variables.tf. Whichever you prefer.
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.
Good catch! I'll keep the ANALYTICS
prefix
analytics/src/analytics/cli.py
Outdated
slackbot = slack.SlackBot(client=WebClient(token=DBSettings.slack_bot_token)) | ||
metric.post_results_to_slack( | ||
slackbot=slackbot, | ||
channel_id=settings.reporting_channel_id, | ||
channel_id=DBSettings.reporting_channel_id, | ||
output_dir=Path(output_dir), |
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.
Does this work? I believe you need to instantiate the object in order to use the variables otherwise you're just using the class definition itself.
Something like
settings = DBSettings()
slackbot = slack.SlackBot(client=WebClient(token=settings.slack_bot_token))
@@ -22,8 +21,10 @@ def get_db() -> Engine: | |||
sqlalchemy.engine.Engine | |||
A SQLAlchemy engine object representing the connection to the database. | |||
""" | |||
db = DBSettings |
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.
You'd want to use get_db_settings
here as well.
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.
@chouinar do you mean something like
def get_db_settings() -> DBSettings:
return DBSettings()
def get_db() -> Engine:
# rest of code
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.
In a few other files you do:
from config import get_db_settings
settings = get_db_settings()
You need to do that here as well.
Just doing DBSettings.user
isn't valid code - a quick test shows that would raise an AttributeError. DBSettings is the class, not an actual settings object.
--
Is this bit of code actually running in the tests? It should fail if run as written right now
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've been testing locally by running the actual make commands which (bizarrely) work. I'm in the process of double checking the tests.
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.
@aplybeah any update on the suggested change?
…Analytics (#136) Fixes #107 Fixes #115 * removed the `*.toml` files related to dynaconf * removed references to Dynaconf (e.g. in Docstrings, gitignore) * use Pydantic for loading > After getting feedback for #107, the consensus was to reevaluate the way the database loader works for more uniformity. Changes will need to be made primarily in db.py and cli.py > > With the PR #84 , the env settings for the db are stored in settings.toml. The config settings should be updated to use the existing local.env file > Screenshots, GIF demos, code examples or output to help show the changes working as expected.
…Analytics (#136) Fixes #107 Fixes #115 * removed the `*.toml` files related to dynaconf * removed references to Dynaconf (e.g. in Docstrings, gitignore) * use Pydantic for loading > After getting feedback for #107, the consensus was to reevaluate the way the database loader works for more uniformity. Changes will need to be made primarily in db.py and cli.py > > With the PR #84 , the env settings for the db are stored in settings.toml. The config settings should be updated to use the existing local.env file > Screenshots, GIF demos, code examples or output to help show the changes working as expected.
…Analytics (navapbc#136) Fixes #107 Fixes #115 * removed the `*.toml` files related to dynaconf * removed references to Dynaconf (e.g. in Docstrings, gitignore) * use Pydantic for loading > After getting feedback for navapbc#107, the consensus was to reevaluate the way the database loader works for more uniformity. Changes will need to be made primarily in db.py and cli.py > > With the PR navapbc#84 , the env settings for the db are stored in settings.toml. The config settings should be updated to use the existing local.env file > Screenshots, GIF demos, code examples or output to help show the changes working as expected.
Summary
Fixes #107
Fixes #115
Time to review: 5 mins
Changes proposed
*.toml
files related to dynaconfContext for reviewers
Additional information