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

feat: Put mongo on the streams network #4805

Merged
merged 2 commits into from
Nov 20, 2018

Conversation

focusaurus
Copy link
Contributor

Impact: minor
Type: feature

Issue

We are building some data integrations for a separate project and need connectivity to mongo, so putting it on the streams network for local development. This doesn't affect production deployments. The related PR into reaction-platform for the streams network is PR 28. It would be best to merge the PR into reaction-platform first then merge this PR.

Solution

Put mongo onto the streams network, adjust as needed in the config and code. I also removed an additional hard coding in waitForMongo.js so more code is properly using the MONGO_URL environment variable.

Breaking changes

This will require the docker network streams.reaction.localhost to be created, which developers can do by pulling down the latest reaction-platform and running make (or make network-create if they want to be surgical about it).

Testing

  • Unit tests still pass
  • I manually ran eslint on waitForMongo.js as it's normally excluded
  • I ran reaction locally and confirmed I could still read and write to mongo via the reaction web app
  • I manually tested waitForMongo.js

@focusaurus focusaurus added enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug impact-minor labels Nov 14, 2018
@focusaurus focusaurus self-assigned this Nov 14, 2018
Copy link
Member

@ticean ticean left a comment

Choose a reason for hiding this comment

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

Hi @focusaurus. Made some change requests that would reduce the scope of change. mongo service can be added to the streams network while leaving the internal connections as they were on the default network.

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@focusaurus focusaurus force-pushed the feat-pete-reaction-search-2 branch 2 times, most recently from c708c56 to 80a0010 Compare November 14, 2018 23:41
@aldeed
Copy link
Contributor

aldeed commented Nov 15, 2018

LGTM if @ticean approves of the changes

@aldeed
Copy link
Contributor

aldeed commented Nov 15, 2018

May also need to look into why the feature branch deploy step failed on CI. I don't know if deploy will fail when merged in the same way. @griggheo

@focusaurus
Copy link
Contributor Author

@griggheo Looks like errors running propel

download: s3://reaction-staging-propel-artifacts/propel-linux-amd64 to ./propel
Error: unknown flag: --suffix
Usage:
  propel param copy [flags]

Copy link
Member

@ticean ticean 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!

@griggheo
Copy link
Contributor

@focusaurus I think you can go ahead and merge if you want, and then I can fix the propel part via another PR.

@griggheo Looks like errors running propel

download: s3://reaction-staging-propel-artifacts/propel-linux-amd64 to ./propel
Error: unknown flag: --suffix
Usage:
  propel param copy [flags]

@focusaurus
Copy link
Contributor Author

@spencern Do you have admin to merge this despite the CI error?

@aldeed aldeed changed the base branch from master to release-2.0.0-rc.7 November 20, 2018 23:31
@aldeed aldeed merged commit 2767447 into release-2.0.0-rc.7 Nov 20, 2018
@aldeed aldeed deleted the feat-pete-reaction-search-2 branch November 20, 2018 23:31
@spencern spencern mentioned this pull request Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants