Clean up database generators implementation #51765
Conversation
andrewn617
force-pushed
the
refactor-devcontainer-implementation
branch
4 times, most recently
from
8d3febe
to
297c0a6
Compare
| def devcontainer_db_service_yaml(**options) | ||
| return unless service = devcontainer_database.service | ||
|
|
||
| { devcontainer_database.name => service }.to_yaml(**options)[4..-1] |
There was a problem hiding this comment.
What does 4 and 1 means here?
There was a problem hiding this comment.
It's the leading --- that separate the body of the doc from directives. I don't think to_yaml takes an option to remove it.
| if db_service | ||
| compose_config["services"].merge!(db_service) | ||
| compose_config["volumes"] = { db_volume_name_for_devcontainer => nil }.merge(compose_config["volumes"] || {}) | ||
| if devcontainer_database.service |
There was a problem hiding this comment.
Can we go a step further and also refactor all the other places that have different behavior depending on the databsae in the generator? There is no reason why this object should be devcontaienr specific. We need to find the gem for the database, the docker image for the database, etc.
There was a problem hiding this comment.
Yeah I can do that. The only slightly annoying thing is including all the other db that aren't available for devcontainer. I will work on it when I find some time this week.
There was a problem hiding this comment.
We can drop those databases. We already decided we would do, but never had a reason to. Now we have that reason.
andrewn617
force-pushed
the
refactor-devcontainer-implementation
branch
from
66e9023
to
148304a
Compare
| case database | ||
| when "mysql" then "default-libmysqlclient-dev" | ||
| when "postgresql" then "libpq-dev" | ||
| class Database |
There was a problem hiding this comment.
Can we move this entire class to railties/lib/rails/generators/database.rb? There is a lot of duplication still on that file that we can remove now that we have a abstraction to the database on generators.
There was a problem hiding this comment.
Yes this is still WIP - sorry got distracted by something else in the middle of working on it this morn. I'll ping you when it is ready for another look
There was a problem hiding this comment.
@rafaelfranca Alright ready for another review
andrewn617
force-pushed
the
refactor-devcontainer-implementation
branch
from
148304a
to
b6ad9b9
Compare
andrewn617
changed the title
andrewn617
force-pushed
the
refactor-devcontainer-implementation
branch
from
b6ad9b9
to
1d9c431
Compare
…ters from the new and db:system:change commands. The supported options are sqlite3, mysql, postgresql and trilogy.
Extract all the DB information (gems, dockerfile packages, devcontainer etc.) into an object. This let's us remove the growing number of case statements in this code.
andrewn617
force-pushed
the
refactor-devcontainer-implementation
branch
from
1d9c431
to
c1bc7ee
Compare
…tation Clean up database generators implementation
Motivation / Background
A lot of complexity has been added to the app generator and change generator for devcontainers, specifically around database setup. My goal here is to simplify this.
Detail
First, I remove support for al the java and proprietary databases from these generators. Going forward, these generators will just support our core databases:
sqlite3,mysql,postgresqlandtrilogy.Then, I extracted all the database info into an object - gems, docker dependencies, and devcontainer setup. Then we just have to do
Database.build(options[:database])to get all the needed database info for a generator. This reduces the huge amount of case statements littering the code.A good side effect, is now the
ChangeGeneratorcan just create its own database object. Previously, theChangeGeneratorwas using theDevcontainermixin to update the devcontainer database config. But that is sketchy because the change generator does not know everything about the app, such as if the app uses redis. So it could be used to generate incorrect devcontainer config if someone started using those methods. Now that logic is separate from the database logic and the change generator only knows how to modify the DB setup.This also allowed me to get rid of the hardcoded list of all the db features and service names which we need to iterate over in the change generator to delete the old db config.
Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]