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

Fixes #52 - Enable .env support with python-dotenv #54

Merged
merged 1 commit into from
May 6, 2019

Conversation

jaryncolbert
Copy link
Collaborator

This commit adds the python-dotenv dependency to manage environment variables, removing the need to specify them in line with the commands to use test data, run the database updates, and start the Flask app.

The README now includes a section on how to copy the provided env.template and modify it to populate the required variables in a new .env file that will not be under version control (and was added to .gitignore.

The Flask environment variables FLASK_APP and FLASK_ENV can be safely added to version control, and are included in the new .flaskenv file. The python-dotenv library will combine the two with .env taking precedence over .flaskenv if there are conflicts.

Copy link
Owner

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! This is an even more comprehensive and automated approach than I imagined. 🤖 I also really appreciate your detailed documentation, including the descriptive error message for missing values. 💜

I have a few small notes on the PostgreSQL connection strings, as well as an optional suggestion for improving some Python readability.

`postgres://localhost/faces`
or
`postgres:///faces`,
to connect over TCP/IP to the database named `faces`.
Copy link
Owner

Choose a reason for hiding this comment

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

I think this got modified in the move. postgres:///faces connects over the Unix domain socket to the database named faces, and I think should probably be the default; postgresql://localhost/ connects over TCP/IP to the default database for your user.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, now that I reread this, I was very unclear about what my concern was here!

Right now, this sentence says:

Alternate database URLs you might try are postgres://localhost/faces or postgres:///faces, to connect over TCP/IP to the database named faces.

The former connects via TCP/IP, but the latter connects via a Unix domain socket, so the explanation at the end is wrong.

Perhaps it could read:

An alternate database URL you might try is: postgres://localhost/faces to connect over TCP/IP to the database named faces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, sorry for the misunderstanding! Corrected with your suggestion 😄

env.template Outdated Show resolved Hide resolved
update-data.py Outdated Show resolved Hide resolved
@jaryncolbert
Copy link
Collaborator Author

Just made all the changes requested and re-pushed env.template, update-data.py, and the README!.

enable the variable `REACT_APP_USE_TEST_DATA=true`,
and then starting the React app is as simple as:
```sh
$ npm run start
Copy link
Owner

Choose a reason for hiding this comment

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

Wait, does this work for NPM?

faces.py Outdated Show resolved Hide resolved
…te README to describe how to populate file from provided env.template
@jaryncolbert jaryncolbert merged commit 4f237e7 into jasonaowen:master May 6, 2019
@jaryncolbert jaryncolbert deleted the 52_dotenv branch May 6, 2019 14:06
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.

2 participants