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

Add task db:prepare:with_data #274

Closed
wants to merge 5 commits into from

Conversation

Floppy
Copy link
Contributor

@Floppy Floppy commented Jul 1, 2023

This is my attempt at #215. At the moment there's lots of code copied out of rails, it needs DRYing up a bit, and it needs tests adding. I'll work on it some more before making this ready for review, but it seems to work...

This is basically a direct copy of ActiveRecord::Tasks::DatabaseTasks#prepare_all, but instead of calling migrate, it calls migrate_with_data (refactored out of databases.rake), and adds data-appropriate calles into schema loading and dumping code as well.

Original code is at https://github.com/rails/rails/blob/5ed37b35d666b833aeccb14a4cacd2926251232d/activerecord/lib/active_record/tasks/database_tasks.rb#L176

The code is on a feature branch off the 9.1.0 tag at the moment, as I know there's a lot of work going on with v10, and I didn't want to start stepping on toes there; also, that's the version I'm using in production so that's where I needed it :)

@Floppy
Copy link
Contributor Author

Floppy commented Nov 25, 2023

This is definitely working reliably for me, so I'm making the PR ready to review, even though it could still do with some tidying up.

@Floppy Floppy marked this pull request as ready for review November 25, 2023 17:34
@ngan ngan requested a review from wildmaples November 25, 2023 19:05
@santiagodoldan
Copy link

It would be awesome to release this feature 💯

@Floppy
Copy link
Contributor Author

Floppy commented Feb 8, 2024

I should be able to get back to this shortly! I've been using it in production for ages 👍

@jgmontoya
Copy link
Contributor

@Floppy @wildmaples any updates on this? Any chance of getting it released soon?

Floppy and others added 5 commits June 20, 2024 14:27
so that we can call it from other tasks directly without invoking rake
Ideally we should reference these directly, I will probably rewrite this commit when I get a moment to work out how.
…o prepare_all in core Rails.

This is basically a direct copy of ActiveRecord::Tasks::DatabaseTasks#prepare_all, but instead of calling migrate, it calls migrate_with_data, and adds data into schema loading and dumping code as well. Original code is at https://github.com/rails/rails/blob/5ed37b35d666b833aeccb14a4cacd2926251232d/activerecord/lib/active_record/tasks/database_tasks.rb#L176
@Floppy Floppy force-pushed the db-prepare-withdata branch from b2cbb40 to 9f3e76a Compare June 20, 2024 13:28
@Floppy
Copy link
Contributor Author

Floppy commented Jun 20, 2024

I've just rebased this against the latest main, so it should be up to date and work with Rails 7.1. I'll test it for a while and report back.

@Floppy
Copy link
Contributor Author

Floppy commented Jun 27, 2024

Seems to be working fine...

@ignaciogrondona
Copy link

Thank you @Floppy for this! Any chance it gets merged anytime soon?

@Floppy
Copy link
Contributor Author

Floppy commented Aug 14, 2024

It's working happily in production on my site, so it all seems fine still. Happy to make any changes based on feedback from @ilyakatz or other maintainers! I guess it depends on v10 work perhaps?

@jgmontoya
Copy link
Contributor

I noticed #319 got merged, not sure what the differences are with this one

@ilyakatz
Copy link
Owner

ilyakatz commented Aug 14, 2024

Oh, i thought it would close this this PR. Basically, cherry picked so that I could update versions. So it's in, thanks for your contribution @Floppy !

@ilyakatz
Copy link
Owner

Fixed by #319

@ilyakatz ilyakatz closed this Aug 14, 2024
@Floppy
Copy link
Contributor Author

Floppy commented Aug 14, 2024

Oh, superb, thanks @ilyakatz!

@jgmontoya
Copy link
Contributor

jgmontoya commented Aug 14, 2024

@ilyakatz @Floppy quick question, just because I'm curious: This PR had some stuff that didn't quite make it to the merged version

i.e.:

# lib/data_migrate/database_tasks.rb
def with_temporary_pool(db_config)
  original_db_config = migration_class.connection_db_config
  pool = migration_class.establish_connection(db_config)
  yield pool
ensure
  migration_class.establish_connection(original_db_config)
end

def migration_class # :nodoc:
  ActiveRecord::Base
end

def migration_connection # :nodoc:
  migration_class.connection
end
def self.prepare_all_with_data
...

What was this about? 😅

EDIT: I see now that with_temporary_pool, migration_class, migration_connection are already defined

@ilyakatz
Copy link
Owner

oh, oops, did i mess something up? I checked out the branch from @Floppy and created a new branch. But looks like I messed something up. Do you mind creating a new PR, if you update the versions numbers, I can just merge as is and not mock around branches

@jgmontoya
Copy link
Contributor

jgmontoya commented Aug 14, 2024

@ilyakatz I added the missing task definition here: #321

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

Successfully merging this pull request may close these issues.

6 participants