DatabaseCleaner / database_cleaner

Strategies for cleaning databases in Ruby. Can be used to ensure a clean state for testing.
https://www.rubydoc.info/github/DatabaseCleaner/database_cleaner
MIT License
2.93k stars 484 forks source link

Adding new adapters #628

Open TheSmartnik opened 4 years ago

TheSmartnik commented 4 years ago

Hi! Since database_cleaner is moving away from putting all adapters in adapters folder. I was wondering how to contribute new adapters? I guess would be a good idea to add a README entry for that.

etagwerker commented 4 years ago

@TheSmartnik Definitely a good idea. Do you want to take a stab at it?

The good news is that there are many examples out there now. For example: https://github.com/databaseCleaner/database_cleaner-active_record

I think the most important code will be in this folder: https://github.com/DatabaseCleaner/database_cleaner-active_record/tree/master/lib/database_cleaner/active_record

The section in the README should say a little bit about:

Anything else, @botandrose?

TheSmartnik commented 4 years ago

@etagwerker thank you for a quick reply! Sure, would love to do that. One thing is unclear, though. When adding a new adapter:

  1. Do I create a folder adapters in database_cleaner repository and you create a separate gem and rept after the pr is merged.
  2. Do I have to create my own repo and transfer it to you
  3. Something else?
botandrose commented 4 years ago

@TheSmartnik Yes, thank you for pointing this out! Now that adapters can be made and maintained without involving changes to database_cleaner core, we should absolutely document how this can be done in the README. Your perspective as an outsider trying to do this will be extremely helpful in creating this documentation. Thank you! I think @etagwerker's suggestion above is a really good place to start. Any bumps you hit or questions you have while doing this will be really valuable.

To answer your question above: in theory, there would be no actual need to involve database_cleaner core at all, in the form of PRs, etc. You can create your adapter repo, and maintain it separately, just as the split-off adapter gems are.

I suppose there's the question of having it be "blessed" or "adopted" into the DatabaseCleaner Github organization, but I think that question can be deferred until after the adapter is up and running and in use.

etagwerker commented 4 years ago

@TheSmartnik What @botandrose said is correct:

  1. Do I create a folder adapters in database_cleaner repository and you create a separate gem and rept after the pr is merged.

You don't need to submit a PR to database_cleaner with the adapter code, you can create your own repository. Once it's up and running, you can submit a PR to database_cleaner so that we can list it here: https://github.com/databaseCleaner/database_cleaner/#list-of-adapters

  1. Do I have to create my own repo and transfer it to you

You don't need to transfer it to the DatabaseCleaner organization. In the future we can consider adding it to our organization.

I suppose there's the question of having it be "blessed" or "adopted" into the DatabaseCleaner Github organization, but I think that question can be deferred until after the adapter is up and running and in use.

Definitely. We can discuss this in the future.

botandrose commented 4 years ago

Thinking about this some more, perhaps database_cleaner-core can package some kind of test suite that adapters can include that verifies that they're adhering to the contract that we will define in the README. Something like:

require "database_cleaner/rspec"

describe DatabaseCleaner::ActiveRecord do
  it_behaves_like "a database_cleaner adapter"
end

This could verify (at the very least) that the described constant supplies a list of strategies, a default strategy, and that each strategy has all the right methods defined. Thoughts?

TheSmartnik commented 4 years ago

@botandrose I really like the idea. Do you want to do it? I can implement that in a separate PR before implementing the README update

botandrose commented 4 years ago

Awesome, yes, please go ahead!

TheSmartnik commented 4 years ago

I took a look at the current adapters and it seems that maybe it would be a good idea to add a cli command for generating adapter. Something like

database_cleaner generate adapter elasticsearch

What do say?

botandrose commented 4 years ago

@TheSmartnik I think that's a really interesting idea, and worth considering!

It would add some complexity to the project, though, so let's wait and see where things are at after we fully document the process for building a new adapter manually. Looking at that, there may be opportunities for us to simplify the process to the point where its so simple to do so that introducing a generator would be overkill. I think this would be the best outcome, if its possible, but let's wait and see.

At least, those are my thoughts on the matter. @etagwerker what do you think?

TheSmartnik commented 4 years ago

@botandrose I've started working on an README, but have found out that since v2 things changed a bit. For once, database_cleaner-core isn't yet released, but all future adapters should depend on it and not on database_cleaner. So I thought maybe it's a good idea to wait with it until v2 is released. What do you think?

botandrose commented 4 years ago

@TheSmartnik Personally, I think having a good README story for adapter writers is something that I want to have when v2 is released. Also, this is our last chance to break things until v3 (which hopefully won't be for a long time), so if there are small things we can break now to make adapter writing simpler and easier, I'd love to do it before v2.

In short, I consider your efforts here vital for v2! Consider me at your disposal for this. :)

botandrose commented 4 years ago

@TheSmartnik Hiya! I'm getting ready to move onto documenting (and hopefully improving) the story for adapter writers, ahead of the upcoming v2 release. If you have anything to share, even if its just a Work in Progress, I'd love to see it!