-
-
Notifications
You must be signed in to change notification settings - Fork 527
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
Improved database defaults #579
Conversation
For security and sanity, must like how Rails does this in ActiveRecord: https://github.com/rails/rails/tree/master/railties/lib/rails/generators/rails/app/templates/config/databases
src/helpers/config-helper.js
Outdated
@@ -69,22 +69,22 @@ const api = { | |||
development: { | |||
username: 'root', | |||
password: null, | |||
database: 'database_development', | |||
database: `${process.env.PWD.split('/').pop()}_development`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this change
src/helpers/config-helper.js
Outdated
host: '127.0.0.1', | ||
dialect: 'mysql' | ||
}, | ||
test: { | ||
username: 'root', | ||
password: null, | ||
database: 'database_test', | ||
database: `${process.env.PWD.split('/').pop()}_test`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this one as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your idea for using environment variables in config by default, but I think we should make that change for production only
3fca901
to
368c91e
Compare
Should have added tests, this config actually doesnt works. Default config is missing "production" keys which use env variable |
Should there be tests for a production environment? I figure that'd be abstracted away by server environment variables per 12factor principles. |
Issue is if those env variable are not defined, |
I am reverting this change, documentation should specify safe defaults, beside having env var inside json is not good either. There is a proper way of doing that as explained in http://docs.sequelizejs.com/manual/tutorial/migrations.html#using-environment-variables |
This reverts commit 006c4ec.
Uses
process.env
variables to control config for production (as documented).Details in commits.