ucsdlib / hifive

An application supporting an Employee Recognition program workflow
MIT License
0 stars 0 forks source link

Feature/env variables #309

Closed VivianChu closed 4 years ago

VivianChu commented 4 years ago

Fixes #298

Local Checklist

What does this PR do?

Extract credentials to environment variables

Deployment Instructions

@ucsdlib/developers - please review

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 99.66% when pulling 47b70f56314a8b2808422f4497889bd2303a7293 on feature/env-variables into df79a4955f7e7aef28b2efa3e4f715dd536070c0 on master.

mcritchlow commented 4 years ago

This all looks great. I think the one question I have is what we should do when required environment variables aren't present.

So currently pretty much throughout we have ENV.fetch('APPS_H5_SENDER') { 'default' }

I'm wondering if we should consider the pros/cons of defaults. Some thoughts:

  1. We could just never have a fallback, which would certainly expose missing environment variables more immediately as we'd get a KeyError
  2. We could have a global symbol we pass in as a default, instead of 'default'. Something like :missing_env_var or something
  3. Leave it as-is with the default string being used. This might actually be allowing some tests to pass? Not sure since we have the .env file now.

In Starlight, we went with Option 1, which would turn the call above into ENV.fetch('APPS_H5_SENDER')

Thoughts?

VivianChu commented 4 years ago

@mcritchlow - I use 'default' value because damspas and dmr are using that way.
However, I like option 1 more. We're also using option 1 in Lark. I'll update the code to use option 1. Thanks