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

[pschambacher]

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.

[rafaelfranca]

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?

[pschambacher]

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?

[rafaelfranca]

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.

[pschambacher]

👍 I'll make the change

[pschambacher]

Changes applied.

[rafaelfranca]

Backported in 92278a5

[pschambacher]

Much appreciated 🙇 🙏

[matthewd]

This seems to conflict conceptually with #28095

[pschambacher]

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]

@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]

@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]

Not a fan of doing this stuff in YAML as a patch. I like shared as a concept, but it should be done when these are being read. Like we do with secrets.

…

On Thu, Apr 27, 2017 at 10:25 AM, Rafael França ***@***.***> wrote: @claudiob <https://github.com/claudiob> totally agree. I'll revert the backport. @matthewd <https://github.com/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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28896 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtRtzg9VkYh3sXMbSysa-Csl2hjjFks5r0M95gaJpZM4NJoqn> .

[pschambacher]

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]

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]

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.