Server startup fails if Google Spreadsheet creds are not present

[kmcculloch]

@danielballan

With code from https://github.com/b5/pipeline/pull/74 in place, running "meteor" to start the server throws errors if process.env can't find GOOGLE_SERVICE_CLIENT_EMAIL or GOOGLE_SERVICE_PRIVATE_KEY

[kmcculloch]

I'm not sure how the rest of your code works if, for example, the environment variables are set but authentication fails, or if authentication succeeds but the Google doc ID hard-coded into your code doesn't exist in the account. Assuming these conditions throw errors, it would be good to fail gracefully so that the project can be cloned to developer workstations or testing environments without needing to touch a production Google account.

One thing I like to do when I .gitignore a credentials file (which is what I assume ".env" is) is commit in example file, like ".env.example" maybe, which has dummy values for whatever creds the code needs. Then a new dev can just make a copy and remove the ".example" extension, and so long as the code fails gracefully on the dummy values everything remains pretty portable.

[danielballan]

That sounds like a good approach to me. In this case, the Meteor.call will just never return. It won't be clear from the browser that it failed but at least it will be clear from the server logs that the auth failed.

[b5]

Agreed. Latest commits avoid this issue by early-returning when env vars aren't present

[kmcculloch]

Yep, I can confirm that the app loads okay now off the master branch, although I get a persistent "Counting uningested rows from Google spreadsheet...." message when I log in as root on my localhost copy that suggests that something behind the scenes is hung. This is a low priority for sure, but in general it's good to check a condition like this and fail gracefully, even if the app is able to limp along regardless.

[kmcculloch]

Latest fix is to wrap the spreadsheet import in a conditional, but that needs refactoring. See @b5's comments on https://github.com/b5/pipeline/pull/86#discussion_r105415631

[kmcculloch]

Here is the comment from the pipeline repo:

Just for the record, conditional imports can lead to problems (caused by transpilers). In the future, we should instead consider wrapping the entire imports/api/spreadsheet_ingest file in a process.env.NODE_ENV === 'production' conditional.

@b5 @danielballan I was actually surprised that the linter didn't object to the conditional import when I tried it. I think it might be because Meteor 1.4 includes a library explicitly meant to handle this:

https://guide.meteor.com/1.4-migration.html#nested-imports

Do you guys still think this is something we should avoid as a general JS bad practice, or should we embrace the fact that Meteor allows it?

[b5]

I've vote to avoid. In a separate but related issue, this conditional import was the source of our app crashing in production last night thanks to a whole file going unchecked until it hit production. Generally we should keep the amount of code affected by "mode checking" as small as possible to make catching env discrepancies easier.

Moving the check out of file inclusion, (even into wrapping all code in the file in the same check) would be a step in the right direction. If that code is imported elsewhere... yeah you guys get the point.