Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow loading configuration from .env files. #229

Closed
wants to merge 4 commits into from

Conversation

shtlrs
Copy link
Contributor

@shtlrs shtlrs commented Jan 9, 2024

Closes #227

The changes in this PR will now allow configuring pinnwand through a .env file.

The configuration in the .env file will not take precedence over its equivalent in the system's environment.

@shtlrs shtlrs force-pushed the load-dotenv branch 3 times, most recently from c4c0fc2 to ff73dc5 Compare January 9, 2024 18:38
@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 9, 2024

@supakeen I see the test_use_shorter_uri is failing because of the clean_bd fixture.

It's only used in one place, which seems a bit off since test_use_longer_uri is almost identical and isn't calling it.

Plus, why are you we clearing the tables if the database is destroyed later on along with its tempdirectory ? Is the point of it to ensure that the tables are empty before we start testing ?

I was thinking about completely deleting this, but wanted to double check first.

EDIT:
I wanted to test this even further, so I wanted to check the behavior on the master branch and I noticed the following:

image

For the same test instance, the database_uri in the database fixture is somehow different then the one in the clear_db fixture and the one that the main command sees before creating the tables.

This could be the reason why we have never seen this exception before, I'll have a deeper look later on to see if I can pinpoint why this is happening.

Second Edit:
After even more investigation, it seems this is a problem with the way the config is loaded, when I inspect configuration.database_uri and database._engine.url, the first has the path/database.db value and the engine is writing to the in-memory database. This seems because the import happens before we load the env variables.
I'll see how to fix this.

@supakeen
Copy link
Owner

Let's track these configuration issues in #232 for now. I'll take a look at these issues as well.

This will do all the heavy lifting of loading config from .env files
This is invoked upon an cli command execution.
This also overrides any value in the actual environment with its equivalent in the .env file.
@shtlrs
Copy link
Contributor Author

shtlrs commented Jan 14, 2024

If #239 gets merged, I'll drop this one & make a new PR since the way we handle config will change.

@supakeen
Copy link
Owner

supakeen commented Mar 2, 2024

@shtlrs I've merged #239.

@supakeen supakeen closed this Mar 10, 2024
@shtlrs
Copy link
Contributor Author

shtlrs commented Mar 10, 2024

@supakeen Great! I'll make a new PR to deal with the related issue.

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.

Load environment variables from .env file
2 participants