search

flashmob / go-guerrilla

Mini SMTP server written in golang
MIT License
2.78k stars 366 forks source link

Allow SQL backend to support alternative drivers #95

Closed danielwhite closed 6 years ago

danielwhite commented 6 years ago

This relates to https://github.com/flashmob/go-guerrilla/issues/94 which was raised in regards to the dependency footprint. I thought it might be more useful to bring something to the table.

By replacing the mysql backend with an sql backend:

  1. It avoids a direct dependency for clients that don't need this plugin.
  2. Enables alternative SQL drivers to be used instead.

The latter case is still untested, and the test case perhaps needs some further consideration.

Questions:

  1. The test only checks if a record was inserted, but doesn't do much sanity checking of the content. Would it be worth making this a bit more complete?
  2. It's quite a breaking change. I think this is okay given the other changes on master since the last release, but happy to rethink this. Thoughts?
  3. Is it worth include an additional test case for an alternative backend (perhaps PostgreSQL) for completeness? The main downside is that it would increase the test imports.
  4. Should the MySQL test be enabled in Travis?
  5. Am I barking up the wrong tree?
flashmob commented 6 years ago

Sorry for the late reply. The change is really neat, thanks!

To answer your questions:

Testing the individual DBs would not be necessary (and not worth additional dependencies as you say). Perhaps it would be worth to test with a mock driver instead? Eg. https://github.com/DATA-DOG/go-sqlmock - it could be something that's imported for tests

Yes, it changes some config options and may break stuff, but should be ok as no release has been made yet. As mentioned by the other guys, the redis import is also in the way for them, so planning to move this to a separate repo in the future.