-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add .env File #105
Add .env File #105
Conversation
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.
We also need some info in the README or CONTRIBUTING that explains the need to copy env.example
to .env
.
Lastly, please rename .env.example
to env.example
so it isn't a hidden file. We want people to see it, since it serves as our template.
.env.example
Outdated
@@ -0,0 +1,4 @@ | |||
NODE_ENV=development |
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.
Above each line, please add a comment
# NODE_ENV should be one of "development" or "production"
NODE_ENV=development
# PORT is the port used by the web server
PORT=8080
...
This example file can serve as both a template, and documentation.
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.
- renamed .env.example to env.example
- added instructions in contributing.md
- added comments in env.example
config/config.js
Outdated
|
||
dotenv.config(); | ||
|
||
module.exports = { |
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 wonder about this vs. just letting people use process.env
everywhere. If we go with this approach, then every file has to require
this file. If we leave it all on process.env.*
we don't.
I'd suggest we remove this and export nothing from config.js
, and just make sure we require('./config')
early in the 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.
ok, do you want me to add
const dotenv = require('dotenv');
dotenv.config();
at the very beginning of index.js for 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.
Let's leave your config.js
file as is, but remove the exports. Then if we have multiple processes, they can all read from the same config using that code.
docs/CONTRIBUTING.md
Outdated
@@ -28,6 +28,9 @@ An easier solution would be to use Docker. | |||
|
|||
**Setup** | |||
1. Navigate to the root directory of telescope. | |||
1. Create a new file and name it ".env". | |||
1. Copy the contents of "env.example" into ".env". |
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.
Combine these first 2 lines into one: 1. Copy env.example to .env to create a new environment configuration
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.
Combined first 2 lines into one
env.example
Outdated
NODEMAILER_USERNAME= | ||
|
||
# NODEMAILER_PASSWORD is sender's password credential | ||
NODEMAILER_PASSWORD= |
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.
Let's remove the NODE_EMAILER_* config until we need it (e.g., future PR can add it if necessary)
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.
removed NODE_EMAILER_* config
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.
Looks good to me, thank you.
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.
LGTM 👍
This PR references #60
Added:
const { port } = require('../config/config'); also wouldn't need to write process.env before the variable