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

Add .env File #105

Merged
merged 10 commits into from Nov 8, 2019
Merged

Add .env File #105

merged 10 commits into from Nov 8, 2019

Conversation

s-arika
Copy link

@s-arika s-arika commented Nov 8, 2019

This PR references #60

Added:

  • .env.example file. So far it has a few environment variables for PORT, NODE_ENV and login for nodemailer
  • config file. Creates a module with all the variables added and it maps and exports them. Now all files can import the config.js module and use it to access our environment variables using
    const { port } = require('../config/config'); also wouldn't need to write process.env before the variable

.env.example Outdated Show resolved Hide resolved
Copy link
Contributor

@humphd humphd left a 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
Copy link
Contributor

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.

Copy link
Author

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 = {
Copy link
Contributor

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.

Copy link
Author

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 ?

Copy link
Contributor

@humphd humphd left a 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.

@@ -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".
Copy link
Contributor

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

Copy link
Author

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=
Copy link
Contributor

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)

Copy link
Author

Choose a reason for hiding this comment

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

removed NODE_EMAILER_* config

Copy link
Contributor

@humphd humphd left a 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.

Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@s-arika s-arika merged commit af8e02c into Seneca-CDOT:master Nov 8, 2019
@s-arika s-arika deleted the issue60 branch November 8, 2019 22:32
@Reza-Rajabi Reza-Rajabi added this to In progress/Review in Main via automation Nov 9, 2019
@Reza-Rajabi Reza-Rajabi moved this from In progress/Review to Done in Main Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Main
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants