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

Fix shared config feature in 3-tier config #45309

Merged

Conversation

eileencodes
Copy link
Member

Rails allows you to use a shared entry in the database yaml which gets
merged into other entries. This was broken when using a 3-tier config.

The change here ensures we're looping through all the configs. We know
we have a 3-tier config if the config is a hash and all the values
inside that config are a hash. If so we need to keep looping, otherwise
we have a standard 2-tier config.

Fixes #45299

Rails allows you to use a `shared` entry in the database yaml which gets
merged into other entries. This was broken when using a 3-tier config.

The change here ensures we're looping through all the configs. We know
we have a 3-tier config if the config is a hash and all the values
inside that config are a hash. If so we need to keep looping, otherwise
we have a standard 2-tier config.

Fixes #
@rails-bot rails-bot bot added the railties label Jun 9, 2022
@eileencodes eileencodes merged commit 21e529b into rails:main Jun 9, 2022
@eileencodes eileencodes deleted the fix-shared-feature-in-3-tier-config branch June 9, 2022 12:52
eileencodes added a commit that referenced this pull request Jun 9, 2022
…er-config

Fix shared config feature in 3-tier config
@jasonkarns
Copy link
Contributor

jasonkarns commented Jan 12, 2023

Is there an expectation that the shared entry would be able to support per-database options?

The original issue #45299 had a sample config where shared had distinct options for each database:

shared:
   foo:
     migrations_paths:
       - "db/foo"

   bar:
     migrations_paths:
       - "db/bar"

I'm running Rails 7.0.4 (which seems to have this patch included), and the current behavior merges shared into each database, rather than merged with each environment.

ie:

shared:
  pool: 15 # all dbs get same pool option

I'm not sure which one is preferred; there are pros and cons to each. The current implementation makes it easy to set defaults that would apply to all databases across all environments. However, it's not possible to set defaults unique per database across all environments.

@eileencodes
Copy link
Member Author

eileencodes commented Jan 12, 2023

and the current behavior merges shared into each database, rather than merged with each environment.

I'm not following the expected vs actual behavior you're asking about. If you think there is a bug please open a new issue with a reproduction.

However, I think the current implementation is correct. Since Rails creates database configuration objects from the info in the database.yml, there's no environment to access once they are created. We don't have a concept of a higher level of shared information that doesn't get merged directly into the database config object.

@jasonkarns
Copy link
Contributor

Opened #47367 as related. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrations ignores shared section in database.yml
2 participants