redcross / smoke-alarm-portal

Red Cross web portal accepting free smoke alarm installation requests
https://getasmokealarm.org/
Other
7 stars 20 forks source link

(IN PROGRESS) Dockerize + Updating a few dependencies #290

Closed YaxelPerez closed 4 years ago

YaxelPerez commented 4 years ago

This pull request adds a few commands that make running the project easier:

In addition, it updates some dependencies.

kfogel commented 4 years ago

One general comment about this change:

The original motivation, as I understood it, was that @YaxelPerez wanted to be able to get this app up and running without tainting his local box with lots of new dependencies. And getting the app up and running is just a step on the way to doing other development on it, of course. So Dockerization was essentially a development strategy (and a fine idea, I think).

But looking at the diff, it seems that a lot of the documentation about how to deploy on a Debian server has gone away. Yet that's how we are deployed in production right now, and Dockerizing for development purposes doesn't change what's going on in production. So that documentation should remain in place. The Dockerization is in addition to, not a replacement for, the existing deployment instructions. (I'm talking about material in INSTALL.md, of course, but also the loss of config.json.tmpl in favor of a Docker-oriented config.json, and maybe other stuff -- I've only had a chance to skim quickly.) This is one reason why I was proposing to have a script that produces config.json from config.json.tmpl -- because that way we preserve config.json.tmpl and avoid duplicating a lot of it in a hardcoded config.json that's meant for Docker.

It's my fault for not offering this feedback earlier, @YaxelPerez -- I'm sorry for that. Now that I have given it, it may result in some backtracking and doing the changes a different way, of course.

YaxelPerez commented 4 years ago

@kfogel

But looking at the diff, it seems that a lot of the documentation about how to deploy on a Debian server has gone away. Yet that's how we are deployed in production right now

This is one reason why I was proposing to have a script that produces config.json from config.json.tmpl -- because that way we preserve config.json.tmpl and avoid duplicating a lot of it in a hardcoded config.json that's meant for Docker.

I put the documentation for how to deploy on a Debian server back.

The new config.js also works without docker. I'm just consolidating all config in .env. Deploying this to production would involve copying values from config.js and config/config.json to .env.

I could roll back changes made to config.js and config/config.json and their respective templates

xmunoz commented 4 years ago

make clean also fails:

$ make clean
rm -f config/recipients.sql
rm -rf node_modules
docker-compose rm -f
Going to remove smoke-alarm-portal_web_1
Removing smoke-alarm-portal_web_1 ... done
docker volume rm -f smokealarm_nodemodules
Error response from daemon: remove smokealarm_nodemodules: volume is in use - [164c06d02f95e87e7481bd6884624289d85836214aebf85582824bee591d30e6]
Makefile:18: recipe for target 'clean' failed
make: *** [clean] Error 1
kfogel commented 4 years ago

Thanks for the review, @xmunoz.

@YaxelPerez, did you run tests (and other make rules) both before and after your changes, to see if your changes caused anything to be different?

YaxelPerez commented 4 years ago

@kfogel @xmunoz There is only one test to run, and it fails because it searches for a string that isn't present in the index page anymore. I'm not sure if I should add the fix since the scope of this PR is already overblown. However, I did manually test getting the app running from a blank slate (make clean).

@xmunoz I also ran into that issue but assumed it was being caused by something specific to my machine. Newest change to the makefile should have fixed it.

A new problem I've found is that you need to run make migrate twice to get it to work. This is because docker-compose doesn't wait for postgres to be 'ready' before it starts trying to run the migrations, so it fails without removing the db container. The second time you run make migrate, it uses the hanging db container.

YaxelPerez commented 4 years ago

solution for the "having to run make migrate twice" issue was just to add a 5 second delay. I looked into wrapping the command with a script that would wait for postgres to be ready (https://docs.docker.com/compose/startup-order/) but this is simpler.

YaxelPerez commented 4 years ago

Rolled back changes so that project doesn't depend on dotenv anymore. Also, some minor adjustments to the makefile.

YaxelPerez commented 4 years ago

Split out into two new PRs:

https://github.com/redcross/smoke-alarm-portal/pull/291 https://github.com/redcross/smoke-alarm-portal/pull/292