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

Initial project #1

Merged
merged 20 commits into from
Sep 4, 2017
Merged

Initial project #1

merged 20 commits into from
Sep 4, 2017

Conversation

austinkelleher
Copy link
Contributor

This is not ready for a very formal review. There are no tests and things aren't completely working. I'm opening this PR as more of a way to share my current directory structure. I propose that we follow something similar to this.

- host: '127.0.0.1'
port: 7000
- host: '127.0.0.1'
port: 7000
Copy link

Choose a reason for hiding this comment

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

7001

package.json Outdated
"cluster:start": "browser-refresh src/cluster.js"
},
"author": "",
"license": "ISC",
Copy link

Choose a reason for hiding this comment

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

No MIT?

POSTGRES_PASSWORD: 'postgres'
POSTGRES_DB: 'windbreaker'
ports:
- '5432:5432'
Copy link

Choose a reason for hiding this comment

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

You may encounter errors when running CI with exposed ports like this.

package.json Outdated
"test:integration": "docker-compose run test npm run test:docker:integration",
"test:docker:ci": "npm run lint && nyc ava test/unit test/integration --verbose",
"test:docker:integration": "nyc ava test/unit test/integration --verbose",
"start": "browser-refresh src/server.js",
Copy link

Choose a reason for hiding this comment

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

Due to a limitation of the docker image we use for rediscluster, you won't be able to actually execute commands on the cluster from outside of docker.

Copy link
Contributor

@Druotic Druotic Aug 6, 2017

Choose a reason for hiding this comment

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

It's actually a limitation of redis cluster in NAT'd environments. Redis cluster will advertise IPs/ports which are routable from inside the docker network, but the host machine has no idea how to route to those addresses. The initial connect will work, but any requests which try to look up a key that is on a different master will result in the client getting redirected to an address which will be unroutable from the perspective of the host. Related: redis/redis#2527

package.json Outdated
@@ -0,0 +1,46 @@
{
"name": "windbreaker-server",
Copy link

Choose a reason for hiding this comment

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

windbreaker-webhooks


startProducer()
// .then(() => {
// TODO: Handle all messages in async queue here
Copy link

Choose a reason for hiding this comment

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

Don't forget to reattach an error listener to the new producer that gets created.

src/server.js Outdated
})

process.on('unhandledException', function (err) {
// TODO: Log this to a database
Copy link

Choose a reason for hiding this comment

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

For situations like this, there's not real guarantee that we will have access to a our database at this point. Using a monitor or logging system like newrelic or scaylr would be slightly more reliable.

package.json Outdated
"author": "",
"license": "ISC",
"dependencies": {
"custom-logger": "^0.3.1",
Copy link

Choose a reason for hiding this comment

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

remove

Copy link
Contributor

@Druotic Druotic left a comment

Choose a reason for hiding this comment

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

👍

@austinkelleher austinkelleher merged commit 8a5ea68 into master Sep 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants