Refactor for_each(databases) code #38658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While working on something else I noticed this code was duplicated. It was returning `name` from `for_each` and then we'd have to lookup the `db_config` again from the `name` and `env`. We can instead just return `db_config` from `for_each` and clean up these tasks a little bit.
eileencodes
added a commit
to eileencodes/rails
that referenced
this pull request
Mar 5, 2020
…each-databases-code" This reverts commit 9a5bc24, reversing changes made to 227ff44. I realized afterwards that this was written this way for a reason and it wasn't accidental. The databases loaded in `for_each` are not complete database configs because these are generated from the database yaml before the Rails env is loaded. So we actually do need to refetch the correct database configuration objects after we've loaded the Rails env.
|
I had to revert this 😢 because I realized I wrote it this way on purpose. See for explanation #38661. I don't have time right now to add a test but will do that tomorrow so I don't forget again in 2 years why I wrote it this way. |
eileencodes
added a commit
that referenced
this pull request
Mar 5, 2020
Revert "Merge pull request #38658 from eileencodes/refactor-for-each-…
eileencodes
added a commit
that referenced
this pull request
Mar 6, 2020
If I had had these tests previously I would have not created PR #38658 and then promptly realize I needed to revert it. We need to load and parse the configurations twice. Once before the environment is loaded to create the named tasks and once after the environment is loaded to have the real configurations. The configs loaded before the env have the ERB stripped out and aren't valid configs.
|
Added tests in 67b102a |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on something else I noticed this code was duplicated. It
was returning
namefromfor_eachand then we'd have to lookup thedb_configagain from thenameandenv. We can instead just returndb_configfromfor_eachand clean up these tasks a little bit.