Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to run dev locally using environment variables #82

Merged
merged 4 commits into from
Dec 25, 2018
Merged

Add the ability to run dev locally using environment variables #82

merged 4 commits into from
Dec 25, 2018

Conversation

Chilace
Copy link
Contributor

@Chilace Chilace commented Dec 10, 2018

in order not to modify the config.json

  • Check if this is a typo @NNTin (DRSS_BOT_CONTROLLER_IDS)?
  • Update the Readme if this PR seems appropriate

in order not to modify the `config.json`
1. `DRSS_BOT_CONTROLLER` is missing in `app.json` manifest, maybe meant `DRSS_BOT_CONTROLLER_IDS` ?
2. `.replace(' ', '')` doesn't make sense because it replaces only the first occurrence
3. Simple array creation without internal spaces
4. Heroku config vars parser should trim leading and trailing spaces by itself, if not, then in this case it is necessary to trim all vars
@Chilace
Copy link
Contributor Author

Chilace commented Dec 13, 2018

@synzen what do you think?

@synzen
Copy link
Owner

synzen commented Dec 20, 2018

sorry for the late reply - I have time to review this now. This seems interesting, I didn't know there was a module like that. But I am wondering what the use case here for dotenv is though? I can't think of a reason other than it being an added complication to something that already works (also yes, the controllers variable was a typo, thanks)

@Chilace
Copy link
Contributor Author

Chilace commented Dec 21, 2018

This is mainly intended for developers who want to contribute. Config varies across devs, code does not
Look at this #77 for an example of how terrible it can be.

This is also useful for running Heroku app locally, since it reads configuration variables from a .env file:
https://devcenter.heroku.com/articles/heroku-local

Glitch also uses a .env file

@synzen
Copy link
Owner

synzen commented Dec 22, 2018

Right, but when I say "added complication to something that already works", the "already works" part I'm referring to is the config json. A couple problems to list that I see this are

  • the variables are not auto transferred over to forked child processes, unless I import dotenv and read the env file (but why not just read the JSON in the first place?)
  • no types
  • because of no types, I have to do more work in type checking whether the config is valid (compare that parsing JSON, easily knowing to throws errors)
  • spreading config across two files now (because, I assume, env files do not store objects and you would have to use a config json anyways)

For heroku in particular, it gives you an interface to input those variables, which is great - they put into the process environment. But if you're modifying the .env file to change variables anyways, why not just change config.json in the first place?

@Chilace
Copy link
Contributor Author

Chilace commented Dec 24, 2018

  • You don't need to import dotenv because it has already preloaded .env into the process environment by dev script of package.json and all env vars are available in child processes.
  • The dotenv parser converts values to strings, besides that, it escapes quotes, trim leading/trailing spaces, etc.
  • No additional work is needed - only those vars that are processed in the config.js are used.
  • JSON is a string and it can be stored in .env:
CONFIG={"log":{"dates":true,"linkErrs":true,"unfiltered":true,"failedFeeds":true},"bot":{"token":"","enableCommands":true,"prefix":"~","status":"online","activityType":"","activityName":"","streamActivityURL":"","controllerIds":[""],"menuColor":5285609,"deleteMenus":false,"exitOnSocketIssues":false,"commandAliases":{}},"database":{"uri":"mongodb://localhost:27017/rss","connection":{},"clean":false,"articlesExpire":14,"guildBackupsExpire":7},"feeds":{"refreshTimeMinutes":15,"checkTitles":false,"timezone":"America/New_York","dateFormat":"ddd, D MMMM YYYY, h:mm A z","dateLanguage":"en","dateLanguageList":["en"],"dateFallback":false,"timeFallback":false,"failLimit":0,"notifyFail":true,"sendOldOnFirstCycle":true,"cycleMaxAge":1,"defaultMessage":":newspaper:  |  **{title}**\n\n{link}\n\n{subscriptions}","imgPreviews":true,"imgLinksExistence":true,"checkDates":true,"formatTables":false,"toggleRoleMentions":false},"advanced":{"shards":2,"batchSize":400,"processorMethod":"concurrent","parallel":2}}
const config = JSON.parse(process.env.CONFIG)

But again, we don't need to store the entire config in .env, only those env vars that are processed later in the config.js.

About Heroku, it prunes dotenv (and all dev dependencies) after building the app by default, so it's about running locally.
For local testing of a Heroku app you need to use .env file anyway - heroku local command uses this file to insert vars into the process environment: https://devcenter.heroku.com/articles/heroku-local#set-up-your-local-environment-variables

If you don't want to use Hiroku CLI you can use dotenv to mimic the action of Heroku config vars.

And I’ll repeat it again, this is necessary:

  • Not for run in production, but only for testing run locally;
  • To prevent accidental commit of a secrets;
  • In order not to modify the config.json at all to keep the project as an exact copy of the upstream (except the files that are being worked on).

@synzen
Copy link
Owner

synzen commented Dec 25, 2018

Ah I see now, thanks 👍

@synzen synzen merged commit f7144a2 into synzen:dev Dec 25, 2018
@Chilace Chilace deleted the run-dev branch December 25, 2018 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants