-
-
Notifications
You must be signed in to change notification settings - Fork 359
Conversation
The PR looks good to me |
Gonna wait to rebase with #146 before continuing |
@timmyichen I think you forgot about this PR 😛, pinging you so that we can merge once you update this PR 😃 |
@vaibhavsingh97 still waiting on #146 🙃 |
In summary: this is blocked by NodeMailer. Once we include NodeMailer we will be able to return to QA'ing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥 , just a minor concern
@@ -19,6 +19,10 @@ services: | |||
- DB_PASSWORD=password | |||
- DB_NAME=chapter | |||
- DB_URL=db | |||
- [email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Env
variables will grow in the future, shouldn't we maintain separate .env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the most part, there's no functional difference between having a long list of env variables in the docker-compose file and a long list of env variables in .env
. With a separate .env file, you are requiring people to copy/rename a file before running it whereas this will be set up when you run docker-compose up
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of simplicity I think that for now we should stick with env vars in docker-compose, but maybe later when we have a lot of them we could extract them.
@timmyichen can you please resolve merge conflicts |
sorry about the delay - done! |
Update README.md
).master
branch of Chapter.Adds serverside testing:
App
class with all the setup/takedown for testsexampleRouter.ts
package.json
into externaljest.config.js
since it was getting quite longI'm not sure what best practices are for this so let me know how this pattern looks and any feedback you might have. Thanks!
TODOS: