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

Dockerize #69

Closed
wants to merge 7 commits into from
Closed

Dockerize #69

wants to merge 7 commits into from

Conversation

leonm1
Copy link
Member

@leonm1 leonm1 commented Apr 24, 2019

Breaking changes:

  1. You must now use docker-compose up --build to build and run the container (and docker-compose up on subsequent runs). The advantage of this setup is that it only requires one command and will always produce the same result on every platform
  2. .env now stores three variables, as docker-compose is hardcoded to use this file and we cannot change that:
SERVER_PORT=8080
CLIENT_PORT=8081
MONGO_PORT=27017
  1. client.env now stores all the env vars for the application, and docker-compose injects them into the containers

@bencooper222
Copy link
Member

Strongly urge that we seek feedback on this from the Slack

@leonm1 leonm1 removed the request for review from slowjazz April 24, 2019 04:15
@leonm1
Copy link
Member Author

leonm1 commented Apr 24, 2019

Not yet mentioned features:

  • it makes deploy a million times easier. We can set up preview deploy with one click now.
  • Testing in containers is much more reliable because there are no environment difficulties

@bencooper222
Copy link
Member

bencooper222 commented Apr 24, 2019

You should

  • pull in the circleci config and rewrite it for docker. They haven't launched their node 12 containers yet but I'm sure it's coming. not 100% sure how it interacts with this.
  • adapt the contributing.md to docker

@cktang88
Copy link
Contributor

cktang88 commented Apr 24, 2019

Yeah I think we should discuss this more.

It's also maybe hard to get docker to work on Windows Home and even WSL?
Might also run into networking problems with port forwarding, firewalls, etc.?

@cktang88 cktang88 added the DO NOT MERGE What does it sound like? label Apr 24, 2019
@bencooper222
Copy link
Member

Yep, I'll try to get this working in WSL. If that works, I don't think we should let Windows Home's flaws hold us back (that's not to say there's other reasons we might not want to do this). The number of developers who lie at the intersection of "only have Windows Home" and "have a stubborn attachment to Windows Home and the Windows terminal" is very small.

@cktang88 cktang88 removed the DO NOT MERGE What does it sound like? label Apr 25, 2019
@github-actions github-actions bot added the approved Automatically added, do not manually add label Apr 25, 2019
@cktang88
Copy link
Contributor

@leonm1 resolve conflicts?

@bencooper222
Copy link
Member

Just realized - should .env be removed from .gitignore if it no longer has sensitive data?

@cktang88
Copy link
Contributor

@leonm1 resolve conflicts so we can merge?

@leonm1
Copy link
Member Author

leonm1 commented May 10, 2019 via email

@cktang88 cktang88 mentioned this pull request May 17, 2019
@bencooper222 bencooper222 self-requested a review May 23, 2019 07:41
@bencooper222
Copy link
Member

IMPORTANT:

  • integrate with circle CI and generally make sure it doesn't fuck all that shit up

I'm actually kinda worried about this - I think there's a lot of intersecting functionality that we need to watch out for.

@stale
Copy link

stale bot commented Jun 22, 2019

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jun 22, 2019
@cktang88
Copy link
Contributor

cktang88 commented Jul 9, 2019

Needs rebase

@bencooper222
Copy link
Member

I don't think rebasing this is worth the time and honestly, I'd rather work on this from square one. I'll consult what you've done but I don't think I'll directly use the code you've written.

@bencooper222 bencooper222 deleted the dockerize branch July 9, 2019 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Automatically added, do not manually add
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants