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

use node-convict for configuration; reorg directory structure to bin/,lib/; use const/let as appropriate #35

Merged
merged 3 commits into from Aug 7, 2018

Conversation

jrgm
Copy link
Contributor

@jrgm jrgm commented Aug 6, 2018

  • put libs in lib/, and executables in bin/
  • no-var
  • use node-convict to be consistent with other repos

The config changes are one step to moving this someday to standard dockerflow.

r? - @jbuck or @vladikoff

@jrgm jrgm requested review from vladikoff and jbuck August 6, 2018 14:24

// Domain whitelist. RCPT TO addresses must match one of the domains on this
// list. (Strict matching, no subdomains).
rcptToDomainWhitelist: {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

webPort: {
'default': 8080,
doc: 'Port number for web server to listen on.',
env: 'WEB_PORT',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I like the EMAIL_PORT, but i am not sure if we have to specify the WEB_ here, can we just use PORT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. But symmetry? I find it odd that one gets to be PORT and the other not.

Copy link
Contributor

Choose a reason for hiding this comment

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

symmetry++

@vladikoff vladikoff merged commit cabdddf into master Aug 7, 2018
@vladikoff vladikoff deleted the node-convict branch August 7, 2018 14:41
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.

None yet

2 participants