Skip to content

Conversation

@jomey
Copy link
Member

@jomey jomey commented Nov 19, 2025

From the commit message:

Moving away from hardcoding a path to find the DB credentials file and use environment
variables instead. This now support two ways - one is setting the path to the file and the
other is setting the database URL directly. This should make it easier to deploy the
library and not relying on the file being in the correct location.

This also adds a credentials file to the test database that matches the user and password from the CI. Should make it easier to test snowex_db and still gives the user the ability to overwrite to his own.

Closes #203

@codecov
Copy link

codecov bot commented Nov 19, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.88%. Comparing base (4d4e7e3) to head (1dd2d1e).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
snowexsql/db.py 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
+ Coverage   75.73%   75.88%   +0.14%     
==========================================
  Files          21       21              
  Lines         647      651       +4     
==========================================
+ Hits          490      494       +4     
  Misses        157      157              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Moving away from hardcoding a path to find the DB credentials file and use environment
variables instead. This now support two ways - one is setting the path to the file and the
other is setting the database URL directly. This should make it easier to deploy the
library and not relying on the file being in the correct location.
Copy link
Contributor

@aaarendt aaarendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jomey we will still need to add env: entries in the workflow files so test work in CI?

@jomey
Copy link
Member Author

jomey commented Nov 19, 2025

@jomey we will still need to add env: entries in the workflow files so test work in CI?

Only for the snowex_db suite once this is merged. This one uses the one set via conftest.

@aaarendt
Copy link
Contributor

@jomey we will still need to add env: entries in the workflow files so test work in CI?

Only for the snowex_db suite once this is merged. This one uses the one set via conftest.

OK, so the logic is if you don't have env variables set, and don't supply your own file, then this must be a test suite? I agree those can be hard coded since they are arbitrary and don't need to be secretized.

@jomey
Copy link
Member Author

jomey commented Nov 19, 2025

@jomey we will still need to add env: entries in the workflow files so test work in CI?

Only for the snowex_db suite once this is merged. This one uses the one set via conftest.

OK, so the logic is if you don't have env variables set, and don't supply your own file, then this must be a test suite? I agree those can be hard coded since they are arbitrary and don't need to be secretized.

That was my thinking. Less wrangling of credentials if its a test run. Open to changes if you see any other easier way or logic.

Copy link
Contributor

@Ibrahim-Ola Ibrahim-Ola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love env variables!

Use a different variable when a user wants to use different credentials than the default.
This only lets the user change the db host and database. The credentials remain the same
to reduce the possibility to run the test against a production DB that would wipe out all
data.
@aaarendt aaarendt merged commit 910143f into master Nov 20, 2025
8 checks passed
@aaarendt aaarendt deleted the credentials branch November 20, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support reading credentials from environment variables

4 participants