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.

Sign up for GitHub

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

Jump to bottom

Brian/#229-env #237

Merged
merged 38 commits into from
Jul 18, 2018
Merged

Brian/#229-env #237

merged 38 commits into from
Jul 18, 2018

Conversation

brian-nguyen
Copy link
Collaborator

@brian-nguyen brian-nguyen commented Jul 1, 2018 •
edited

🎟️ Ticket(s): Closes #229

👷 Changes

Dev and Prod environments should only differ by a .env file now, which currently contains the following variables:

NODE_ENV
PORT
SERVER_URL
DATABASE_URL

Adding a new environment variable requires modifying the two docker-compose files hmmm. More work can be done here. The make docker-start has been removed due to some issues with Webpack (the production build specifically) not being able to pick up the environment variables from the build container. Due to this bug, in the production build, the .env is passed to the client container, while the server container receives the contents of the .env file through Docker-Compose as expected.

🔦 Testing Instructions

Create a .env file with the variables mentioned above and place it in the root directory.
Then, make client and make server

@brian-nguyen brian-nguyen added the ready for review This issue or pull request is ready to be reviewed label Jul 1, 2018
@coveralls
Copy link

coveralls commented Jul 1, 2018 •
edited

Coverage Status

Coverage increased (+0.07%) to 66.061% when pulling 07a41d1 on brian/#229-env into d6b1277 on master.

@brian-nguyen brian-nguyen added work in progress This issue or pull request is a work in progress and removed ready for review This issue or pull request is ready to be reviewed labels Jul 2, 2018
@brian-nguyen brian-nguyen added ready for review This issue or pull request is ready to be reviewed and removed work in progress This issue or pull request is a work in progress labels Jul 2, 2018
Grafne23
Grafne23 previously approved these changes Jul 3, 2018
View reviewed changes
Copy link
Collaborator

@Grafne23 Grafne23 left a comment

Choose a reason for hiding this comment

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

Looks fine, I didn't run it though. Will we need to change our run process?

@brian-nguyen
Copy link
Collaborator Author

brian-nguyen commented Jul 4, 2018 •
edited

@Grafne23

I didn't run it though

Why not?

Will we need to change our run process?

Yes. See the last paragraph of the description.

@Grafne23
Copy link
Collaborator

Grafne23 commented Jul 4, 2018

Why not?
Not a good use of time on a PR like this - theres nothing to test on it except if it runs or not but I trust that it ran for you otherwise you wouldn't PR it.

Yes. See the last paragraph of the description.
I run it with make server and make client, does it change those too?

@brian-nguyen brian-nguyen added work in progress This issue or pull request is a work in progress and removed ready for review This issue or pull request is ready to be reviewed labels Jul 6, 2018
@brian-nguyen
Broader ignore pattern to help with testing different environments
5e8d933
@brian-nguyen
Add .env file to container for webpack build
e0ca78d
@brian-nguyen
Add log for database connection
dd78c9b
@brian-nguyen
Use PWD env to get relative path
edced62
@brian-nguyen
Switch config using env variable. Unnecessary config should be stripp…
8bf408f 
…ed out during webpack build
@brian-nguyen
Fix spelling mistake
7def60c
@brian-nguyen
Move production and dev builds to single file
79083f8
@brian-nguyen
Update command for development
90b1252
@brian-nguyen
Revert "Update command for development"
5c63ada 
This reverts commit 90b1252.
Reason: custom Docker-Compose command is incompatible with current
version of Inertia.
@brian-nguyen
Revert "Move production and dev builds to single file"
2812c43 
This reverts commit 79083f8.
Current version of Inertia does not support custom commands.
@brian-nguyen
Merge branch 'brian/#229-env' of https://github.com/ubclaunchpad/bumper…
f364fb9 
… into brian/#229-env
@brian-nguyen
Add more env vars and logs to report them
58fabc3
@brian-nguyen
Ignore VS IDE artifact
15f5e53
@brian-nguyen
Load environment variables from .env file
a227955 
New library was added to allow for locally
running instances of the server. Tries the
parent and current directory for redundancy.
@brian-nguyen
Removing docker dev command
baa7d9a 
Bug exists with webpack builds and env loading.
The env passed to the build container is not
being picked up by the npm run build command.
Further investigation is needed.
@brian-nguyen
Fix path to .env file from Docker and Webpack
aa870f3 
This path allows for both locally running instances
and the full production build to succeed.

Fix typo reported by gometalinter
@brian-nguyen brian-nguyen requested review from Grafne23 and SeeratSek and removed request for SeeratSek July 17, 2018 06:08
@brian-nguyen brian-nguyen added ready for review This issue or pull request is ready to be reviewed and removed work in progress This issue or pull request is a work in progress labels Jul 17, 2018
@Grafne23
Copy link
Collaborator

Can you post what we should set those variables to? Both build for me but the client isn't reaching the server so I must have one of them wrong.
Also don't forget to run make deps as there's a new go dependancy.

@Grafne23
Copy link
Collaborator

Works for me with the env variables:
NODE_ENV=development
SERVER_URL=localhost:9090
DATABASE_URL=https://bumperdevdb.firebaseio.com
PORT=9090

Grafne23
Grafne23 requested changes Jul 17, 2018
View reviewed changes
Copy link
Collaborator

@Grafne23 Grafne23 left a comment

Choose a reason for hiding this comment

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

Should update the readme so it's easy for people to run. Maybe now it should say that you need the .env file and an example of what goes in it?

@brian-nguyen brian-nguyen merged commit a520b09 into master Jul 18, 2018
@brian-nguyen brian-nguyen deleted the brian/#229-env branch July 18, 2018 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Grafne23 Grafne23 Grafne23 requested changes

@SeeratSek SeeratSek Awaiting requested review from SeeratSek

Assignees
No one assigned
Labels
ready for review This issue or pull request is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@brian-nguyen @coveralls @Grafne23