(IN PROGRESS) Dockerize + Updating a few dependencies

[YaxelPerez]

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

• make setup
• make migrate
• make build
• make run
• make test

In addition, it updates some dependencies.

[kfogel]

Was there a reason to replace the prominent all-caps fill-these-in placeholders with the smaller "user" and "pass"?

[kfogel]

Oh, is that maybe a standard in Docker configuration -- something looks for those specific strings to replace?

[YaxelPerez]

I'm using "user" and "pass" as the default user and pw for postgres in docker-compose.yml

[kfogel]

Gotcha. And that's okay because this is all inside Docker and has no security implications for anyone else (right? just checking).

However, config/config.json.tmpl is also a file people are supposed to edit (after copying it to config/config.json) when they're setting up a production service. This change replaces the long, all-caps placeholders with short, lower-case strings, and furthermore the new strings aren't just placeholders -- they're actually used (within Docker).

So I think a better change would be to script the copying of config/config.json.tmpl to config/config.json and the replacement of the two placeholder parameters with the desired actual parameters, as part of Docker setup. In other words, instead of changing the template config file, script the process of converting the template config file to the live file, so that the distinction between template and live file remains clear.

Does this make sense?

[YaxelPerez]

Yeah, I see what you mean. I think it might be better to avoid editing JSON in a makefile and instead rely on a .env file for configuration? That way, docker-compose can read the parameters from there. I checked and config.json is only used in a couple of places so it shouldn't be hard to replace.

[YaxelPerez]

Yep, all done. Config is consolidated in a .env file now

[xmunoz]

This a first-pass review with some ergonomic suggestions for streamlining the setup. Suggestions aside, I cloned your fork and tried to get everything setup, and the db migration failed.

$ make migrate
docker-compose -f docker-compose.builder.yml run --rm migrate
Starting smoke-alarm-portal_db_1 ... done

Sequelize [Node: 8.17.0, CLI: 2.8.0, ORM: 3.35.1]

Cannot find "/usr/src/app/config/config.js". Have you run "sequelize init"?
Makefile:16: recipe for target 'migrate' failed

[xmunoz]

Should rm -f .env be added here too?

[YaxelPerez]

decided makefile shouldn't mess with .env since it's deploy-specific

[xmunoz]

Why not let the docker do the default postgres volume management mentioned here?

Let Docker manage the storage of your database data by writing the database files to disk on the host system using its own internal volume management. This is the default and is easy and fairly transparent to the user.

[YaxelPerez]

By default postgres doesn't create a volume for itself, I think. I'm using the volume that Makefile creates with make build

[xmunoz]

make run now fails with this:

web_1  | > smoke-alarm-portal@0.0.1 dev /usr/src/app
web_1  | > nodemon ./bin/www
web_1  | 
web_1  | [nodemon] 1.19.4
web_1  | [nodemon] to restart at any time, enter `rs`
web_1  | [nodemon] watching dir(s): *.*
web_1  | [nodemon] watching extensions: js,mjs,json
web_1  | [nodemon] starting `node ./bin/www`
web_1  | 
web_1  | /usr/src/app/node_modules/twilio/lib/Client.js:20
web_1  |             throw 'Client requires an Account SID and Auth Token set explicitly ' +
web_1  |             ^
web_1  | Client requires an Account SID and Auth Token set explicitly or via the TWILIO_ACCOUNT_SID and TWILIO_AUTH_TOKEN environment variables
web_1  | [nodemon] app crashed - waiting for file changes before starting...

[kfogel]

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]

@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]

make run now successfully brings up the app. make test fails, however. Here is some of that output:

Sequelize [Node: 8.17.0, CLI: 2.8.0, ORM: 3.35.1]

Loaded configuration file "config/config.js".
Using environment "development".
No migrations were executed, database schema was already up to date.

> smoke-alarm-portal@0.0.1 test /usr/src/app
> NODE_ENV=test mocha tests



  GET
GET / 200 477.718 ms - -
    1) responds with an index page in HTML


  0 passing (964ms)
  1 failing

  1) GET responds with an index page in HTML:
     Uncaught AssertionError: expected '<!DOCTYPE html>\n<html lang="en">\n  <head>\n    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">\n    <meta charset="utf-8">\n    <meta http-equiv="X-UA-Compatible" content="IE=10; IE=9; IE=8; IE=EDGE">\n    <title>Get a Smoke Alarm</title>\n    <meta name="description" content="Get a Smoke Alarm from the Red Cross.">\n    <meta id="id_og_type" property="og:type" content="website">\n    <meta id="id_og_title" property="og:title" content="Get a Smoke Alarm">\n    <meta id="id_og_description" property="og:description" content="Get a Smoke Alarm from the Red Cross.">\n    <meta id="id_fb_app_id" property="fb:app_id" content="64368537493">\n    <meta id="id_og_site_name" property="og:site_name" content="American Red Cross">\n    <meta id="id_og_image" property="og:image" content="http://www.redcross.org/images/MEDIA_CustomProductCatalog/m48040100_ButtonLogo200.jpg">\n    <meta id="id_og_url" property="og:url" content="https://getasmokealarm.org">\n    <meta name="author" content=" ">\n    <meta name="keywords" content="smoke alarm, smoke detector, red cross ">\n    <meta name="robots" content="">\n    <meta name="viewport" content="width=device-width, initial-scale=1.0">\n    <link rel="shortcut icon" href="/favicon.ico">\n    <!-- Bootstrap-->\n    <link rel="stylesheet" type="text/css" href="/third-party/bootstrap.min.css">\n    <script type="text/javascript" src="/third-party/jquery.min.js"></script>\n    <script type="text/javascript" src="/third-party/bootstrap.min.js"></script>\n    <!-- Drywall config-->\n    <script src="/layouts/core.min.js?br34k-01"></script>\n    <!-- jQuery validate-->\n    <script type="text/javascript" src="/third-party/jquery.validationEngine-en.js"></script>\n    <script type="text/javascript">\n      $.validationEngineLanguage.allRules["phone-10-digit-US"] = {\n        "regex": /^ *(\\+?1-?)?(\\([2-9]([02-9]\\d|1[02-9])\\)|[2-9]([02-9]\\d|1[02-9]))(-| +)?[2-9]([02-9]\\d|1[02-9])(-| +)?\\d{4} *$/,\n        "alertText": "* Invalid phone number" // AFAICT this err msg will never be seen; yet this key must have a value for stuff to work.\n      }\n    </script>\n    <script type="text/javascript" src="/third-party/jquery.validationEngine.js"></script>\n    <link rel="stylesheet" type="text/css" href="/third-party/validationEngine.jquery.css">\n    <!-- Local stylesheets and js-->\n    <script type="text/javascript" src="/js/main.js"></script>\n    <!-- The DCSOps stylesheet messes up how bootstrap columns behave when collaped-->\n    <!-- link(rel=\'stylesheet\', type=\'text/css\', href=\'./smokeAlarmPortal_files/DCSOps.css\')-->\n    <link rel="stylesheet" type="text/css" href="./smokeAlarmPortal_files/base.css">\n    <noscript><div id="javaScriptError" style="text-align:center; padding-top:130px;"> <font color="FF0000"> <img src="/images/icon/warning.png" width="20" height="23" alt="" /> <strong> Javascript is disabled in your browser. In order to use the site correctly, please enable javascript.</strong> </font> </div></noscript>\n  </head>\n  <body>\n    <div class="bg"></div>\n    <div class="bg-screen"></div>\n    <div id="container" class="container-fluid">\n      <div id="top">\n        <div id="logo" class="text-center"><img src="./smokeAlarmPortal_files/redcross-logo-white-letters.png" alt="American Red Cross" class="logo">\n        </div>\n        <div id="safety-message" role="complementary" class="row text-center">\n          <p class="col-xs-12 bigger-centered">Smoke alarms save lives.  Installing a smoke alarm is the first step to keeping your family safe.</p>\n        </div>\n      </div>\n      <div id="language">\n        <div class="row">\n          <p class="col-xs-12 bigger-centered"><a data-lang="en" class="language-toggle">English</a>&nbsp;|&nbsp;<a data-lang="es" class="language-toggle">Español</a></p>\n        </div>\n      </div>\n      <div id="main" role="main" class="row">\n        <div id="left" class="col-md-3">\n          <p>The Red Cross and its partners will install a limited number of free smoke alarms for those who cannot afford to purchase smoke alarms or for those who are physically unable to install a smoke alarm.  The Red Cross installs a limited number of specialized bedside alarms for individuals who are deaf or hard-of-hearing.  To request a free installation, please complete all fields in the form below and click “Submit.”</p>\n        </div>\n        <div id="center" class="col-md-6">\n          <div role="form" class="container-fluid">\n            <form action="/" method="POST" class="form-horizontal">\n              <fieldset>\n                <div class="row">\n                  <!-- Text input-->\n                  <div class="form-group">\n                    <div class="col-xs-12">\n                      <label for="name" class="screenreader">Your Name</label>\n                      <input id="name" name="name" type="text" placeholder="Your Name" data-prompt-position="topLeft:0,5" class="form-control input-md validate[required]">\n                    </div>\n                  </div>\n                </div>\n                <div class="row">\n                  <!-- Text input-->\n                  <div class="form-group">\n                    <div class="col-xs-12">\n                      <label for="street_address" class="screenreader">Street Address &amp; Unit (e.g., 123 Main St., Unit Q)</label>\n                      <input id="street_address" name="street_address" type="text" placeholder="Address &amp; Unit" data-prompt-position="topLeft:50,5" class="form-control input-md validate[required]">\n                    </div>\n                  </div>\n                </div>\n                <div class="row">\n                  <!-- Text input-->\n                  <div class="form-group">\n                    <div class="col-xs-5">\n                      <label for="city" class="screenreader">City</label>\n                      <input id="city" name="city" type="text" placeholder="City" data-prompt-position="topLeft:0,5" class="form-control input-md validate[required]">\n                    </div>\n                    <div class="col-xs-3">\n                      <label for="state" class="screenreader">State</label>\n                      <select id="state" name="state" data-prompt-position="topRight:-40,5" class="form-control validate[required]">\n                        <option value="" disabled selected>State</option>\n                        <option value="Alabama">AL</option>\n                        <option value="Alaska">AK</option>\n                        <option value="Arizona">AZ</option>\n                        <option value="Arkansas">AR</option>\n                        <option value="California">CA</option>\n                        <option value="Colorado">CO</option>\n                        <option value="Connecticut">CT</option>\n                        <option value="Delaware">DE</option>\n                        <option value="District of Columbia">DC</option>\n                        <option value="Florida">FL</option>\n                        <option value="Georgia">GA</option>\n                        <option value="Hawaii">HI</option>\n                        <option value="Idaho">ID</option>\n                        <option value="Illinois">IL</option>\n                        <option value="Indiana">IN</option>\n                        <option value="Iowa">IA</option>\n                        <option value="Kansas">KS</option>\n                        <option value="Kentucky">KY</option>\n                        <option value="Louisiana">LA</option>\n                        <option value="Maine">ME</option>\n                        <option value="Maryland">MD</option>\n                        <option value="Massachusetts">MA</option>\n                        <option value="Michigan">MI</option>\n                        <option value="Minnesota">MN</option>\n                        <option value="Mississippi">MS</option>\n                        <option value="Missouri">MO</option>\n                        <option value="Montana">MT</option>\n                        <option value="Nebraska">NE</option>\n                        <option value="Nevada">NV</option>\n                        <option value="New Hampshire">NH</option>\n                        <option value="New Jersey">NJ</option>\n                        <option value="New Mexico">NM</option>\n                        <option value="New York">NY</option>\n                        <option value="North Carolina">NC</option>\n                        <option value="North Dakota">ND</option>\n                        <option value="Ohio">OH</option>\n                        <option value="Oklahoma">OK</option>\n                        <option value="Oregon">OR</option>\n                        <option value="Pennsylvania">PA</option>\n                        <option value="Rhode Island">RI</option>\n                        <option value="South Carolina">SC</option>\n                        <option value="South Dakota">SD</option>\n                        <option value="Tennessee">TN</option>\n                        <option value="Texas">TX</option>\n                        <option value="Utah">UT</option>\n                        <option value="Vermont">VT</option>\n                        <option value="Virginia">VA</option>\n                        <option value="Washington">WA</option>\n                        <option value="West Virginia">WV</option>\n                        <option value="Wisconsin">WI</option>\n                        <option value="Wyoming">WY</option>\n                      </select>\n                    </div>\n                    <div class="col-xs-4">\n                      <label for="zip" class="screenreader">Zip Code</label>\n                      <input id="zip" name="zip" type="text" placeholder="Zip Code" data-prompt-position="topRight:-40,5" class="form-control input-md validate[required, custom[zip]]">\n                    </div>\n                  </div>\n                </div>\n                <div class="row">\n                  <div class="form-group">\n                    <div class="col-xs-5">\n                      <label for="phone" class="screenreader">Phone Number</label>\n                      <input id="phone" name="phone" type="text" placeholder="Phone #" data-prompt-position="topRight:0,5" class="form-control input-md validate[groupRequired[contactPreference], custom[phone-10-digit-US]]">\n                    </div>\n                    <div class="col-xs-7">\n                      <label for="email" class="screenreader">Email Address</label>\n                      <input id="email" name="email" type="text" placeholder="Email" data-prompt-position="topRight:0,5" class="form-control input-md validate[groupRequired[contactPreference], custom[email]]">\n                    </div>\n                  </div>\n                </div>\n                <div class="row">\n                  <!-- http://www.sitepoint.com/understanding-bootstrap-grid-system/ helped-->\n                  <div class="form-group">\n                    <div class="col-xs-12 text-center">\n                      <input type="hidden" name="_csrf" value="cgwY3DHr-i8hOTnS65-zBeD1HeHzA6KQp_Ek">\n                      <button id="singlebutton" name="singlebutton" class="btn btn-primary">Submit<img src="/images/loader.gif" style="display:none;" class="loader"></button>\n                    </div>\n                  </div>\n                </div>\n              </fieldset>\n            </form>\n          </div>\n        </div>\n        <div id="right" class="col-md-3">\n          <p>The American Red Cross and its partners have launched the Home Fire Campaign to reduce deaths and injuries caused by home fires by 25 percent over five years.  Depending on where you live and other factors, either an American Red Cross representative or one of our partner organizations will contact you to schedule an installation appointment.</p>\n        </div>\n      </div>\n      <div id="bottom" class="row">\n        <p class="availability col-xs-12">This form can accept requests from the following states and counties: AR (except Crittenden and Miller Counties), IA, ID, IL, KS, MN, MO, MT, ND, NE, SD, WA (except Clark, Cowlitz, Klickitat, Pacific, Skamania and Wahkiakum Counties), WI and Malheur County, OR.<span class="notabene">  If your location is not listed, please call 800-RED-CROSS to be connected to </span><span class="notaoptime">your</span><span class="notabene"> local Red Cross Chapter.</span></p>\n        <p class="availability col-xs-12">If you are able to purchase and install your own smoke alarm, but would like information on home fire safety and smoke alarm installation, please visit <a href="https://www.redcross.org/sound-the-alarm" target="_blank" class="contrast-link">redcross.org/sound-the-alarm</a>.</p>\n        <p class="availability col-xs-12">\n          This web site is open source \n          &mdash;\n           volunteers welcome <a href="https://github.com/redcross/smoke-alarm-portal/" target="_blank" class="contrast-link"> at GitHub</a>.\n        </p>\n      </div>\n      <script type="text/javascript">\n        // data for jquery error tootips, translated by i18n\n        var validationRules = {\n            \'#name\': {\n              \'required\': {\n                \'message\': "* " + "This field is required"\n              }\n            },\n            \'#street_address\': {\n              \'required\': {\n                \'message\': "* " + "This field is required"\n              }\n            },\n            \'#city\': {\n              \'required\': {\n                \'message\': "* " + "This field is required"\n              }\n            },\n            \'#state\': {\n              \'required\': {\n                \'message\': "* " + "This field is required"\n              }\n            },\n            \'#phone\': {\n              \'groupRequired\': {\n                \'message\': "* " + "Please provide either a 10-digit phone number or an email address"\n              },\n              \'custom[phone-10-digit-US]\': {\n                \'message\': "* " + "The phone number is invalid"\n              }\n            },\n            \'#zip\': {\n              \'required\': {\n                \'message\': "* " + "A valid US zip code is required"\n              },\n              \'custom[zip]\': {\n                \'message\': "* " + "A valid US zip code is required"\n              }\n            },\n            \'#email\': {\n              \'groupRequired\': {\n                \'message\': "* " + "Please provide either a 10-digit phone number or an email address"\n              },\n              \'custom[email]\': {\n                \'message\': "* " + "The email address is invalid"\n              }\n            }\n        };\n        // Change validation rules for contact info on 311\n        if (window.location.pathname.substr(0, 4) === \'/311\') {\n          // Delete contact rules from object, change custom validation classes\n          delete validationRules[\'#email\'][\'custom[email\'];\n          delete validationRules[\'#phone\'][\'groupRequired\'];\n          $(\'#phone, #email\').attr(\'class\', \'form-control input-md\');\n          $(\'#email\').addClass(\'validate[custom[email]]\');\n          $(\'#phone\').addClass(\'validate[required, custom[phone-10-digit-US]]\');\n          $(\'#phone\').attr(\'required\', true);\n        \n          validationRules[\'#phone\'][\'required\'] = {\n            \'message\': \'* \' + "A 10-digit phone number is required"\n          };\n        }\n        $(".form-horizontal").validationEngine({\n          \'scroll\': false,\n          \'autoPositionUpdate\': true,\n          \'maxErrorsPerField\': 1,\n          \'onValidationComplete\': function() {\n            $("#singlebutton").prop("disabled", true);\n            $("img.loader").show();\n            return true;\n          },\n          \'custom_error_messages\': validationRules\n        });\n        // On Firefox and Safari back-forward caching causes the submit button state to be retained,\n        // even after clicking the back button. This checks if the state has been cached, and\n        // if so re-enables the submit button. https://stackoverflow.com/a/13123626\n        $(window).bind("pageshow", function(event) {\n          if (event.originalEvent.persisted) {\n            $("#singlebutton").prop("disabled", false);\n            $("img.loader").hide();\n          }\n        });\n        // Fix Safari font-weight. The bizzare if-clause is feature-detection for Safari.\n        if (Object.prototype.toString.call(window.HTMLElement).indexOf(\'Constructor\') > 0) { $("body").css("font-weight", "400"); }\n      </script>\n    </div>\n  </body>\n</html>' to include 'Thank you for your interest'
      at Test.<anonymous> (tests/index-spec.js:44:37)
      at Test.assert (node_modules/supertest/lib/test.js:156:6)
      at Server.assert (node_modules/supertest/lib/test.js:127:12)
      at emitCloseNT (net.js:1664:8)
      at _combinedTickCallback (internal/process/next_tick.js:136:11)
      at process._tickCallback (internal/process/next_tick.js:181:9)



npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! smoke-alarm-portal@0.0.1 test: `NODE_ENV=test mocha tests`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the smoke-alarm-portal@0.0.1 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2020-02-27T20_59_30_448Z-debug.log
Makefile:5: recipe for target 'test' failed
make: *** [test] Error 1

[xmunoz]

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]

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]

@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]

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]

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

[YaxelPerez]

Split out into two new PRs:

#291
#292