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

build(docker): Allows sharing of network #16937

Merged
merged 1 commit into from Mar 30, 2018

Conversation

Projects
None yet
5 participants
@ojongerius
Copy link
Contributor

commented Mar 23, 2018

See #16936 for details

docker-compose.yml Outdated
@@ -23,6 +24,7 @@ services:
working_dir: /fcc
command:
- npm
- start
- run
- develop

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 23, 2018

Author Contributor

I think this makes sense for development, if other people prefer 'npm start' that's fine too. It should def not do in if you deploy using Docker -I'm pretty sure you are not.

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 23, 2018

Member

It's on the road map to to move completely to docker. But that is some way away. This should be fine.

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Great. Any idea on the timeline? Thinking of it, you probably would not want to use Docker Compose for orchestration of your deployments anyway.

docker-compose.yml Outdated

server:
ports:
- "27017:27017" # Change port if needed

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 23, 2018

Author Contributor

I like being able to connect to mongo, like the change below, this is purely general dx, not related to reaching the goal described in #16936

networks:
- shared

networks: # Used by by other projects like open-api

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 23, 2018

Author Contributor

Creates a network that can be used by other projects wishing to use these resources

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 23, 2018

Member

I like it a lot

docker-compose-shared.yml Outdated
db:
networks:
- shared
freecodecamp:

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 23, 2018

Author Contributor

Less generic than server, other project will refer to the network as freecodecamp_shared which is why I chose a more meaningful name.

docker-compose-shared.yml Outdated
# docker-compose -f docker-compose.yml -f docker-compose-shared.yml up
#
version: "2"
services:

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 23, 2018

Author Contributor

Over rides for services, connects them to the shared network created under networks.

@raisedadead raisedadead changed the title feat: Allows sharing of network, addresses #16936 build(docker): Allows sharing of network Mar 23, 2018

docker-compose-shared.yml Outdated
# Run with:
# docker-compose -f docker-compose.yml -f docker-compose-shared.yml up
#
version: "2"

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 23, 2018

Member

Are we better sticking to version 2? Are there issues with version 3?

I am out of the loop when it comes to docker

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 23, 2018

Author Contributor

Not sure actually, could be worth considering.

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Have bumped it to 3, as was done to staging in per #16825

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Bumped to 3, mostly because docker-compose.yml had been bumped. It seems version 3 is mostly related facilitating Docker Swarm, which I would avoid.

@ojongerius

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

Note that merging #16825 borked this PR. I'll update based on what went in after the weekend.

@ojongerius ojongerius referenced this pull request Mar 23, 2018

Closed

Docker Setup #20

@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2018

@ojongerius updated the pull request.

1 similar comment
@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2018

@ojongerius updated the pull request.

@ojongerius ojongerius force-pushed the ojongerius:feature/share_network branch 2 times, most recently Mar 25, 2018

@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2018

@ojongerius updated the pull request.

@ojongerius

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2018

Merged changes from #16825, good to review when you have time. /cc @Bouncey

docker-compose.yml Outdated
#
version: "3"
services:
db:

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Renamed this back to db. I'd rather keep it generic.

docker-compose.yml Outdated
db:
image: mongo:3.2.6
ports:
- "27017:27017" # Change port if needed

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Still keeping these ports open for development.

docker-compose.yml Outdated
version: "3"
services:
db:
image: mongo:3.2.6

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Reverted this version.

docker-compose.yml Outdated
image: mongo:3.2.6
ports:
- "27017:27017" # Change port if needed
freecodecamp:

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Use a meaningful name, as this will be used as part of the shared network name.

docker-compose.yml Outdated
ports:
- "27017:27017" # Change port if needed
freecodecamp:
image: node:8.9.4

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

I'd still suggest pulling something like freecodecamp/node:stable but that is for another PR.

image: node:8.9.4
depends_on:
- db
- mailhog

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Added mailhog as a dependency.

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 26, 2018

Member

Can you expose the mailhog web port? 8025 off the top of my head, might be wrong.

This comment has been minimized.

Copy link
@raisedadead

raisedadead Mar 26, 2018

Member

Yup, its 8025

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 26, 2018

Author Contributor

Sure!

docker-compose.yml Outdated
environment:
- MONGOHQ_URL=mongodb://db:27017/freecodecamp
volumes:
- .:/app

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

As was changed in the most recent PR. It's a little nicer than fcc, so kept that in.

docker-compose.yml Outdated
- develop
ports:
- "3000:3000" # Change port if needed
mailhog:

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Mailhog! Nice work by @otarza

@@ -1,31 +0,0 @@
# Docker Compose sample file for freeCodeCamp

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

No need to have this as a sample file, devs can customise behaviour with over ride files.

@@ -56,5 +56,3 @@ public/css/main*
webpack-bundle-stats.html
server/rev-manifest.json
google-credentials.json

docker-compose.yml

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Get this file back under source control. If people want override files added to gitignore they can come up with a patter to ignore, but I'm still have not heard a good use case for it.

docker-compose up
```

#### Shared

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 25, 2018

Author Contributor

Hoping this makes sense.

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 26, 2018

Member

So, if I have this straight in my head, this will expose only the mongo connection inside the image, but not the loopback server?

This comment has been minimized.

Copy link
@ojongerius

ojongerius Mar 26, 2018

Author Contributor

Docker containers could already communicate between each other, this just exposes the port to the host running the containers. This allows users to any mongo client to connect to the db from their workstation/laptop.

You are right, it does not expose the loopback server. We could totally expose that too.

CONTRIBUTING.md Outdated

You need to have [docker](https://docs.docker.com/install/) and [docker-compose](https://docs.docker.com/compose/install/) installed before executing commands below.
You'll need to have [docker](https://docs.docker.com/install/) and [docker-compose](https://docs.docker.com/compose/install/) installed before executing commands below.

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 26, 2018

Member

You'll need to have docker and docker-compose installed before executing commands below.

You will need to have docker and docker-compose installed before executing the commands below.

docker-compose up
```

#### Shared

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 26, 2018

Member

So, if I have this straight in my head, this will expose only the mongo connection inside the image, but not the loopback server?

image: node:8.9.4
depends_on:
- db
- mailhog

This comment has been minimized.

Copy link
@Bouncey

Bouncey Mar 26, 2018

Member

Can you expose the mailhog web port? 8025 off the top of my head, might be wrong.

@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

@ojongerius updated the pull request.

1 similar comment
@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2018

@ojongerius updated the pull request.

@ojongerius

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

@Bouncey ready for re-review 😄

@ojongerius

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

AFAICT to the site to be able to reach mailhog twe'll need to add a datasources file for docker, or make changes to datasources.development and use mailhog instead of localhost when running in Docker.

Before I make those changes (far outside of the scope of this original PR), do you have any preference? I'm erring on staying with datasources.development, adding some complexity to it, but I still believe in a bright future where all the things are neatly containerised.

Edit: have implemented this in 4b4b55ba6dd5b12a899d2e5910240dcdad6c86a9

@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2018

@ojongerius updated the pull request.

1 similar comment
@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2018

@ojongerius updated the pull request.

@ojongerius

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2018

@raisedadead why is the blocked for Beta? Because PR work is a constrained resource?

@raisedadead

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

Hi @ojongerius can you take a minute to squash this? I am having trouble pulling the PR. Sorry I just happened to switch machines and dont have my usual dotfiles to rescue me.

Edit: found the problem.

@raisedadead raisedadead force-pushed the ojongerius:feature/share_network branch Mar 29, 2018

@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2018

@raisedadead updated the pull request.

@raisedadead

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

Fixed.

@raisedadead
Copy link
Member

left a comment

Almost there.

server/datasources.development.js Outdated
@@ -23,7 +23,14 @@ if (!process.env.SES_ID) {
}
};
debug(`using MailHog server on port ${ds.mail.transport.port}`);
} else {

This comment has been minimized.

Copy link
@raisedadead

raisedadead Mar 29, 2018

Member

Lets keep it simple:

if (!process.env.SES_ID) {
  ds.mail = {
    connector: 'mail',
    transport: {
      type: 'smtp',
      host: process.env.MAILHOG_HOST || 'localhost',
      secure: false,
      port: 1025,
      tls: {
        rejectUnauthorized: false
      }
    },
    auth: {
      user: 'test',
      pass: 'test'
    }
  };
  debug(`using MailHog server on port ${ds.mail.transport.port}`);
}
@raisedadead

This comment has been minimized.

Copy link
Member

commented Mar 29, 2018

Hi @ojongerius I have rebased / sqashed this PR (don't worry the descriptions are in there).
You should just delete you local branch for this PR and then fetch and checkout.

One minor change and we are good to go!

@ojongerius ojongerius force-pushed the ojongerius:feature/share_network branch Mar 30, 2018

@camperbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2018

@ojongerius updated the pull request.

feat: Allows sharing of network
build(docker): Expose mailhog port.

build(Docker): Improve wording.

build(docker): Mailhog and network changes.

    * Add mailhog container to shared network to enable connectivity
    * Configure project name for docker compose in .env sample file,
        without it the basename of the repo directory is used, which
        makes it unreachable for other services
    * Set mailhog host to mailhog (instead of localhost) if MAILHOG_HOST
      env var is set
    * Expose 1025 to enable local troubleshooting

build(docker): Update README to reflect compose changes.

@ojongerius ojongerius force-pushed the ojongerius:feature/share_network branch to 02e2b8f Mar 30, 2018

@freeCodeCamp freeCodeCamp deleted a comment from camperbot Mar 30, 2018

@ojongerius

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

@raisedadead thanks for squashing and the suggestion to simplify!

@raisedadead raisedadead merged commit ef37c3b into freeCodeCamp:staging Mar 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ojongerius ojongerius deleted the ojongerius:feature/share_network branch Mar 30, 2018

ojongerius added a commit to ojongerius/freeCodeCamp that referenced this pull request Apr 3, 2018

Merge remote-tracking branch 'upstream/staging' into feature/custom_i…
…mage

* upstream/staging: (36 commits)
  docs(documentation): Update readme.md and contributor.md (freeCodeCamp#16990)
  feat: Allows sharing of network (freeCodeCamp#16937)
  fix(seed): Correct typos in es6.json (freeCodeCamp#16972)
  fix(seed): Add test for checking the length of buttons is 2 (freeCodeCamp#16921)
  fix(challenges): Change Symmetric Differences Title (freeCodeCamp#16962)
  fix(challenges): Fix typo in css-grid justify-self challenge (freeCodeCamp#16961)
  chore(package): Update react-freecodecamp-search (freeCodeCamp#16943)
  fix(seed): Fixed issue with approximately always failing (freeCodeCamp#16752)
  fix(lang): Refetch mapUi on language change (freeCodeCamp#16844)
  fix(seed): Make element naming optional (freeCodeCamp#16926)
  fix(seed): Chall seed and test are modified to allow better t (freeCodeCamp#16928)
  fix(projects): Add user stories to projects' description (freeCodeCamp#16924)
  feat(seed): An HTML illustration added (freeCodeCamp#16939)
  fix(gulp): run babel-node with inspect flag in debug (freeCodeCamp#16901)
  fix: remove flash saying JS is disabled
  fix(common): Added expected homeURL that was missing (freeCodeCamp#16922)
  fix(settings): Night mode settings and profile page UI improvements (freeCodeCamp#16806)
  style(settings): Remove extra whitespace
  fix(settings): Report user modal centered to the page
  fix(settings): Fix modal success button hover animation
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.