Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

[Task] Update analytics db to use local.env and Remove Dynaconf from Analytics #136

Merged
merged 10 commits into from
Jul 26, 2024

Conversation

aplybeah
Copy link

@aplybeah aplybeah commented Jul 8, 2024

Summary

Fixes #107
Fixes #115

Time to review: 5 mins

Changes proposed

  • removed the *.toml files related to dynaconf
  • removed references to Dynaconf (e.g. in Docstrings, gitignore)
  • use Pydantic for loading

Context for reviewers

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

Additional information

Screenshots, GIF demos, code examples or output to help show the changes working as expected.

@aplybeah aplybeah marked this pull request as draft July 8, 2024 19:55
Comment on lines 18 to 19
slack_bot_token: str = Field(alias="SLACK_BOT_TOKEN")
reporting_channel_id: str = Field(alias="REPORTING_CHANNEL_ID")
Copy link
Collaborator

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.

Copy link
Author

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

Comment on lines 252 to 256
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),
Copy link
Collaborator

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))

@aplybeah aplybeah marked this pull request as ready for review July 10, 2024 15:17
@@ -22,8 +21,10 @@ def get_db() -> Engine:
sqlalchemy.engine.Engine
A SQLAlchemy engine object representing the connection to the database.
"""
db = DBSettings
Copy link
Collaborator

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.

Copy link
Author

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

Copy link
Collaborator

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

Copy link
Author

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.

Copy link
Member

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?

@aplybeah aplybeah merged commit 79aa2cd into main Jul 26, 2024
7 checks passed
@aplybeah aplybeah deleted the alsia/update-db-conn branch July 26, 2024 20:05
acouch pushed a commit that referenced this pull request Sep 18, 2024
…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.
acouch pushed a commit that referenced this pull request Sep 18, 2024
…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.
acouch pushed a commit to HHS/simpler-grants-gov that referenced this pull request Sep 18, 2024
…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.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Remove Dynaconf from Analytics [Task]: Update analytics db to use local.env
4 participants