[CLI] Moves DB settings to config/environments #402
Conversation
eliasjpr
requested review from
drujensen,
elorest,
marksiemers and
faustinoaq
| @@ -56,9 +56,9 @@ describe Amber do | |||
| expected_session = {:key => "amber.session", :store => :signed_cookie, :expires => 0} | |||
| settings.session.should eq expected_session | |||
| settings.port.should_not eq 3000 | |||
| settings.database.should eq "mysql://root@localhost:3306/amber_test_app_test" | |||
There was a problem hiding this comment.
I liked this being in secrets more because as a framework we're not opinionated on which ORM to use or even if you have to use one at all.
Someone might choose to use mongo instead of a relational database or choose to have 2 or 3 databases per environment. As such I feel like it makes more sense to leave this in secrets.
There was a problem hiding this comment.
One way or another, the database url for non-development environments needs to be protected, as it has a username and password in it.
Either encrypting it, or reading from an environment variable.
Is there a certain one that amber has been preferring to date?
|
@elorest I think the database is embedded in Amber and feel like the Does that make sense? |
| @@ -10,15 +10,16 @@ describe Amber::Server do | |||
| @log = ::Logger.new(STDOUT).tap{|l| l.level = ::Logger::INFO} | |||
| @color = true | |||
| @redis_url = "\#{ENV[%(REDIS_URL)]? || %(redis://localhost:6379)}" | |||
| @database = "mysql://root@localhost:3306/amber_test_app_test" | |||
There was a problem hiding this comment.
should be called database_url
| it "does create the database when `db migrate`" do | ||
| MainCommand.run ["generate", "model", "Post"] | ||
| MainCommand.run ["db", "migrate"] | ||
| p db_filename |
|
I have not made any changes to this PR since @drujensen and I will pair to merge the 2 branches of work #403 |
|
@drujensen and @amberframework/core-team please review and approve |
Currently database settings reside in the config/database.yml this is redundant since Amber has settings per environment. This PR removes the `config/database.yml` and uses the environment yamls files to load the database settings. There is a couple of benefits for doing this: 1. Database settings are loaded per environment. 2. It is more intuitive to know where to put DB settings. [ENV LOADER] Add database settings to environment_loader - Adds database property to Amber::Settings - Adds database property to spec test.yml - Load database settings from environment yml - Updates settings specs to check for database settings - Updates environment loader specs to check for database settings [CLI] Updates init spec file - Updates amber spec file to check for database key - Tidy init_spec connection string validation - Add `expected_db_url(db_key, env)` method to cleanup specs
eliasjpr
force-pushed
the
ep/move-db-settings-to-environments
branch
from
f3f15bd to
2eb329d
Compare
|
We will use |
|
closing this in favor of #406 which changes the name from |
[CLI] Moves DB settings to config/environments …
Currently database settings reside in the config/database.yml this is
redundant since Amber has settings per environment.
This PR removes the
config/database.ymland uses the environment yamlsfiles to load the database settings.
There is a couple of benefits for doing this:
[ENV LOADER] Add database settings to environment_loader
[CLI] Updates init spec file
expected_db_url(db_key, env)method to cleanup specsIssue #293