Provide better 'bad key' value error message by rlxdev · Pull Request #1558 · 18F/identity-idp

rlxdev · 2017-07-19T12:32:10Z

Why:

Provide clearer message when environment variables have
yes/no values instead of true/false - prior message pointed
to application.yml instead of environment, wasting debug time

How:

Remove incorrect 'config/application.yml' reference, as values
come from environment instead
Indicate source is 'environment' in error message if ENV is used
Specifically check for 'yes' or 'no' in environment variable value
Remove unnecessary '\' - makes clearer that only '\n' is being
removed

monfresh · 2017-07-19T16:09:01Z

lib/config_validator.rb

env will always be ENV since that is the default, and we don't set it to anything else. So, instead of this conditional, what do you think about updating the message in the warning method to say something like "You have invalid values (yes/no) for [bad keys] in config/application.yml or your environment. Please change them to true or false."?

monfresh · 2017-07-19T16:13:34Z

Thanks for this PR, and congrats on your first GitHub pull request!

Out of curiosity, can you please explain how you are configuring the environment? Are you not using config/application.yml at all? Or are you using it locally, and then once you're ready to push the values to production, you are setting the ENV variables directly on your server, or perhaps you are pushing them to Heroku using Figaro's helper commands?

rlxdev · 2017-07-19T18:50:41Z

Thanks, I'll take your suggestion. My edits are partly out of ignorance about how things are working, but I hit this issue and spent time trying to figure it out so I thought I'd save someone else by changing the message.

I configured this on a CentOS 7 VM using your instructions in README. I just wanted to try it locally to see if I could get it up and running. I hit a couple of things that may be environment-related, but I got it going.

BTW, the environment variable I hit this on is IMSETTINGS_INTEGRATE_DESKTOP (was set to 'yes').

I think this is a great idea for common authentication for government agencies and I hope it is well adopted. It would be a win-win for the agencies that use it and also the public users.

monfresh · 2017-07-19T19:26:54Z

Ah, I see. Thanks for the details. IMSETTINGS_INTEGRATE_DESKTOP is not something this app uses. It was set in your VM, and the bug is that we are checking environment variables that aren't related to the app. So, instead of iterating through all the keys that don't start with Figaro::Application::FIGARO_ENV_PREFIX, we should do the opposite and only select those keys, and then strip the prefix from them.

Do you want to take a stab at fixing that? If not, I'd be happy to work on it.

rlxdev · 2017-07-19T19:29:39Z

Exactly, and sure, I'll take a stab at it. Thanks!

rlxdev · 2017-07-21T15:23:38Z

Here's my stab at it - hopefully, I got it right. I notice that with the configuration settings, Figaro creates 2 environment variables, one with the setting name as the key and one with the key prefixed with "FIGARO_". If we're only interested in the configuration settings and not the other environment variables, then we can collect up the keys having both definitions (prefixed and not prefixed). These are the changes I've made.

Prior to commit, I've tested this out by printing the key/value combinations I've found and I'm satisfied that we're getting only the configuration values for the application.

I hope this is what you're looking for.

monfresh · 2017-07-21T16:14:24Z

lib/config_validator.rb

Was there a particular reason for switching to regex? It's almost twice as slow as checking the array. Can we do this instead?

%w[yes no].include?(env[key].strip.downcase)

monfresh · 2017-07-21T16:15:29Z

lib/config_validator.rb

Can we rename the keys argument to env, since that is what is passed in?

monfresh · 2017-07-21T16:21:10Z

spec/lib/config_validator_spec.rb

Single space after period?

monfresh · 2017-07-21T16:25:38Z

spec/lib/config_validator_spec.rb

In order to test the stripping and downcasing, can you please add some more candidate keys, such as " YES" and "NO "?

rlxdev · 2017-07-21T23:21:41Z

thanks for your patience and suggestions:

Was there a particular reason for switching to regex? It's almost twice as slow as checking the array. Can we do this instead?

You're right.

Can we rename the keys argument to env, since that is what is passed in?

Yes

Single space after period?

Yup, I'm a student of when you put 2 spaces after sentences - hard habit to break.

In order to test the stripping and downcasing, can you please add some more candidate keys, such as " YES" and "NO "?

Added 2 relevant tests.

Thanks!

monfresh

Looks great. Thanks! Please squash your commits into one.

**Why**: - Current implementation looks at all environment variables, both those associated with application configuration and those that don't apply (e.g., IMSETTINGS_INTEGRATE_DESKTOP). Non-configuration settings with bad values will cause warnings. This change aims to target validation to only the configuration values. **How**: - Select keys (environment variable names) that have a 2nd definition prefixed with "FIGARO_" - these are the key/value pairs associated with application configuration. - Check values case insensitive, ignoring white space. - Add test cases.

rlxdev · 2017-07-24T20:43:45Z

Thanks! Squash is done.

monfresh

LGTM

monfresh reviewed Jul 19, 2017

View reviewed changes

monfresh reviewed Jul 21, 2017

View reviewed changes

lib/config_validator.rb Outdated

Copy link

Contributor

monfresh Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename the keys argument to env, since that is what is passed in?

monfresh reviewed Jul 21, 2017

View reviewed changes

spec/lib/config_validator_spec.rb Outdated

Copy link

Contributor

monfresh Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single space after period?

monfresh reviewed Jul 21, 2017

View reviewed changes

monfresh approved these changes Jul 21, 2017

View reviewed changes

rlxdev force-pushed the rlxdev-cfg_validator branch from 14cb296 to dad14d9 Compare July 24, 2017 20:41

monfresh approved these changes Jul 28, 2017

View reviewed changes

monfresh merged commit 80a0598 into 18F:master Jul 28, 2017

Conversation

rlxdev commented Jul 19, 2017

Uh oh!

monfresh Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jul 19, 2017

Uh oh!

rlxdev commented Jul 19, 2017

Uh oh!

monfresh commented Jul 19, 2017

Uh oh!

rlxdev commented Jul 19, 2017

Uh oh!

rlxdev commented Jul 21, 2017

Uh oh!

monfresh Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

monfresh Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

monfresh Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

monfresh Jul 21, 2017

Choose a reason for hiding this comment

Uh oh!

rlxdev commented Jul 21, 2017

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

rlxdev commented Jul 24, 2017

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

monfresh Jul 19, 2017 •

edited

Loading