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

Added a shared section to config/database.yml that will be loaded for all envs #28896

Merged
merged 1 commit into from Apr 27, 2017

Conversation

Projects
None yet
6 participants
@pschambacher
Copy link
Contributor

pschambacher commented Apr 27, 2017

Summary

I've discovered this commit from @dhh to have a shared section for secrets.yml today at @claudiob 's talk at RailsConf.
I really like the idea and since all the database.yml that I'm working with have some & <<: * shenanigans, I thought it would be nice to make it available there as well.

activesupport/lib/active_support/core_ext/yaml.rb Outdated
@@ -0,0 +1,17 @@
require "yaml"

module YAML

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 27, 2017

Member

We should not add a monkey patch to implement this feature. We don't want to implement a new version of the YML specification. Could you move it back to the previous implementation and only add support to the database configuration?

This comment has been minimized.

@pschambacher

pschambacher Apr 27, 2017

Author Contributor

I sure can but fear that we might want to expand this to more yml files over time. Would it be OK if I make it a module of its own rather than a monkey patch? ActiveSupport::YamlWithSharedSection.load?

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 27, 2017

Member

If that is the case we can do it, but right now I prefer duplication. This is not something that I personally want to spread to more yaml files. I also don't want to give users access to this feature so as harder it is to do, better for us.

This comment has been minimized.

@pschambacher

pschambacher Apr 27, 2017

Author Contributor

👍 I'll make the change

@pschambacher pschambacher force-pushed the pschambacher:load_with_shared branch Apr 27, 2017

@pschambacher pschambacher force-pushed the pschambacher:load_with_shared branch to dfc361d Apr 27, 2017

@pschambacher

This comment has been minimized.

Copy link
Contributor Author

pschambacher commented Apr 27, 2017

Changes applied.

@rafaelfranca rafaelfranca merged commit 28cd12c into rails:master Apr 27, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Apr 27, 2017

Merge pull request #28896 from pschambacher/load_with_shared
Added a shared section to config/database.yml that will be loaded for all envs
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 27, 2017

Backported in 92278a5

@pschambacher pschambacher deleted the pschambacher:load_with_shared branch Apr 27, 2017

@pschambacher

This comment has been minimized.

Copy link
Contributor Author

pschambacher commented Apr 27, 2017

Much appreciated 🙇 🙏

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented Apr 27, 2017

This seems to conflict conceptually with #28095

@pschambacher

This comment has been minimized.

Copy link
Contributor Author

pschambacher commented Apr 27, 2017

Hmm yes my change would not affect those nested keys inside of environments.
Maybe letting the shared key alone rather than use delete so it can be recovered when loading the nested configuration?

@claudiob

This comment has been minimized.

Copy link
Member

claudiob commented Apr 27, 2017

@matthewd I wonder if, given that conflict, we might be better off not backporting this to 5.1 and instead letting it ship with 5.2 with full support for #28095

@rafaelfranca what do you think? Seems like a new feature that we are adding to a RC.

rafaelfranca added a commit that referenced this pull request Apr 27, 2017

Revert "Merge pull request #28896 from pschambacher/load_with_shared"
This reverts commit 92278a5.

This seems to conflict conceptually with #28095 and it is a new feature
after the RC.
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 27, 2017

@claudiob totally agree. I'll revert the backport.

@matthewd I'm not strong about this feature and I'm leaning to the side of reverting it entirely. I personally don't like that we are creating a new yaml dialect. WDYT?

@dhh

This comment has been minimized.

Copy link
Member

dhh commented Apr 27, 2017

@pschambacher

This comment has been minimized.

Copy link
Contributor Author

pschambacher commented Apr 27, 2017

Regarding 5.1-5.2, I intended this for 5.2 so all cool 😊
@dhh do you mean moving from loading the YAML to where ActiveRecord reads the configuration with the environment?

@matthewd

This comment has been minimized.

Copy link
Member

matthewd commented May 8, 2017

Yeah, my concern is that "shared" feels a rather blunt instrument in a three-level world: do we mean a) these values are shared by all connections in all environments, or b) these are entire connection definitions which are shared across environments? Or do we actually want to c) specify values that are shared by all connections within an environment?

To me, it starts to get involved enough that the explicitness of YAML's built-in syntax, where the data gets explicitly labelled and then referenced, seems more predictable.

If we can decide on an overwhelmingly-more-useful interpretation between the three above, though, there are obviously advantages of supporting the same mechanism we do in secrets. It does feel like it belongs a bit further up, where AR uses database_configuration, though... inside resolve_all, perhaps.

@pschambacher

This comment has been minimized.

Copy link
Contributor Author

pschambacher commented May 8, 2017

I actually tend to think that the shared concept would bring a lot to the other PR. The sample yaml given in the PR would simply become:

shared:
  adapter: sqlite3
  pool: <%= ENV.fetch("RAILS_MAX_THREADS") { 5 } %>
  timeout: 5000

development:
  primary:
    database: db/development.sqlite3
  readonly:
    database: db/readonly.sqlite3

It looks concise and clear to me.
Based on your feedback and dhh's one, I'll make a follow-up PR to move the logic where AR is using database_configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.