Fix local dev; improve configuration system

[nwalters512]

This PR fixes the local dev story by installing up-to-date versions of node-sass and sqlite3 and improving how configuration is handled.

Previously, config was handled by vanilla dotenv and a single .env file. While conceptually very simple, this introduced an (undocumented) need to devs to create a .env file after cloning the repo, since it's best practice to not version-control the .env file.

This PR switches to using dotenv-flow, which supports environment-specific config, as well as .env.*.local files that aren't tracked by VCS. This allows us to check in a .env.development file that's used during local dev, while still allowing end users to specify secret values in .env.production.local files.

This PR also drops the idiosyncrasy of including _{ENV} in the database config environment variable names. What was previously DB_DIALECT_DEVELOPMENT and DB_DIALECT_PRODUCTION is now simply DB_DIALECT; support for multiple environments is handles by having multiple suffixed .env files.

The above two changes mean that this will require changes to config files in production deployments. The existing .env file should be changed to .env.production.local to ensure the correct precedence, and the suffixed DB_* values will need to have the environment-specific suffix removed.

Resolves #323

[vercel[bot]]

Deployment failed with the following error:

should NOT have additional property `scale`

[echuber2]

Thanks again! We'll try this out. Do we need to revoke the permissions that Vercel used to have through this configuration portal? https://github.com/apps/vercel/

[nwalters512]

We can to be safe, though I think the permissions are read-only, and it's a public repository, so there's no urgency there. I'm unable to do that, as I'm not an owner of the illinois organization.

Note that we'll want to keep the integration enabled for illinois/queue-info, as it's still working and useful there.

[nwalters512]

Throw an approval on this once you're able to confirm that it works and I'll merge it in :shipit:

[astrosticks]

Thanks Nathan :) :+1:

[echuber2]

@nwalters512 I edited the run dev script starter in package.json to set NODE_ENV=production (which was a crucial detail I think my team had been missing before). I also added some documentation to .env.development so others don't make the same mistake in the future, if package.json gets damaged or something and the env var is unset again.

I also added a few things to the gitignore so we don't make a blunder with these new .env options. Should production switch to using .env.production.local instead of .env, since .env has lower priority than .env.development with dotenv-flow? I copied the relevant documentation about this priority in a comment in .env.development.

In the other thread #330 you mentioned you would remove now.json, but it's still here. It doesn't seem to be doing any harm; should we remove it or leave it?

Also, this was working in node 9.11.2 with an old npm v5.x and now in node 14.15.5 with npm 6.something. It also works with node 14.15.5 and npm 7.5.4, but this migrates the format of package-lock.json and I'm not sure if that's okay or not.

Lastly, I noted that npm update breaks things, but I suppose that was to be expected.

[nwalters512]

I edited the run dev script starter in package.json to set NODE_ENV=production

Was starting to question this and then I looked at the commit and saw it was NODE_ENV=development. Good catch! I'm going to change this to set the default_node_env option for dotenv-flow instead, which should be more resilient in the future.

I also added a few things to the gitignore so we don't make a blunder with these new .env options.

I appreciate this!

Should production switch to using .env.production.local instead of .env, since .env has lower priority than .env.development with dotenv-flow?

Yep, I'll call this out in the PR message.

I copied the relevant documentation about this priority in a comment in .env.development.

I'd bias towards keeping docs like this under the relevant section in the readme. I'd also prefer not to duplicate dotenv-flow's docs here, instead just linking out to their repo.

you mentioned you would remove now.json, but it's still here. It doesn't seem to be doing any harm; should we remove it or leave it?

I was going to do this in a separate PR, but I can do that here.

Lastly, I noted that npm update breaks things, but I suppose that was to be expected.

Heh, yeah, I considered doing that here but opted to just make the minimal number of changes possible to get master in a working state. We can come back in the future and do other package bumps.

[echuber2]

Thanks for catching my typo there, oof. Thankfully I didn't put the wrong one in the real files.

It seems a little fragile on node 9.11.2 (I seemed to get a corrupted database locally trying to experiment with it, but can't reproduce that reliably). With node 14.15.5, it seems okay now. It also seems okay with npm upgraded to 7.5.4, but this pushes the version of package-lock.json to lockfileVersion: 2. What combination of node/npm versions should we be using here? I feel inclined to try to run with node 14.15.5 and npm 7.x if we can, as long as we're ripping off bandages. (Node 14 because it's LTS for a good while yet.)

[nwalters512]

I'm not too concerned about how things work on Node 9, it's not an LTS version. I was testing locally with Node 14, and things worked fine there.

I've had a variety of issues with npm v7 both at work and over in PrairieLearn, so I'm inclined to stick with v6 for now, or to switch to using Yarn instead. I'm going to consider that outside the scope of this PR though. Let's get master in a known good state, then we can make and test all the other smaller, isolated changes we thing might improve things.

[echuber2]

I think it's okay from the look of it (tried again it with latest npm 6). However before we merge this I'd like to try a test run locally in a centos docker, replicating the production server's current setup and then doing the git pull, nvm/npm installs, and auto migration, just to see if everything works seamlessly.

My concern was that as soon as we merge this to master, somebody would go and update production too.

[nwalters512]

Sounds good, let me know once you've had a chance to test.

[echuber2]

I got to try this in a Docker replication of the live server (to the extent that I could reproduce it). It seems like the update/migration can work okay. We may have to mess with the pm2 configuration a little bit on the server though; maybe schedule some downtime.

~I did run into a migration error for one record in the real database (one student/netid in particular). Not sure how to deal with that, or if anything's necessary. The errors were like this, redacting the ID:~

  code: 'ER_DUP_ENTRY',
    errno: 1062,
    sqlState: '23000',
    sqlMessage: "Duplicate entry 'z*****0@illinois.edu' for key 'users_netid_unique'",
    sql: "UPDATE users SET uid='z*****0@illinois.edu' WHERE uid='z*****0'"

(Update: I don't think this is related.)

[echuber2]

@nwalters512 We're going to be merging this in the near future (I know it's been a long time). Sorry for leaving it standing so long!