Add postgres support with tests #44

spencerldixon · 2017-02-14T16:07:05Z

Building on top of Zach's PR and attempting to get this gem working with tests so we can merge postgresql support. Not entirely sure what I'm doing here so I would appreciate any input!

dblock · 2017-02-14T17:01:52Z

Start by rebasing this code to fix the conflicts.

dblock

I made a bunch of comments.

dblock · 2017-02-14T17:02:28Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 * [#28](https://github.com/slack-ruby/slack-ruby-bot-server/pull/28): Use slack-ruby-danger gem - [@dblock](https://github.com/dblock).
 * [#31](https://github.com/slack-ruby/slack-ruby-bot-server/pull/31): Adds MONGODB_URI as environment variable for MongoDB - [@corprew](https://github.com/corprew).
+* [#38](https://github.com/slack-ruby/slack-ruby-bot-server/pull/38): Add Postgres support, reference #7. - [@zachfeldman](https://github.com/zachfeldman).


There's an extra period in here, and you can remove "reference #7." altogether.

dblock · 2017-02-14T17:03:10Z

README.md

@@ -31,6 +31,8 @@ You can use the [sample application](sample_app) to bootstrap your project and s

 Install [MongoDB](https://www.mongodb.org/downloads), required to store teams. We would like your help with [supporting other databases](https://github.com/slack-ruby/slack-ruby-bot-server/issues/12).

+You can also use PostgreSQL. See the PostgreSQL example - ensure you've filled out postgresql.yml and have setup your database correctly (user with the right name and privileges according to the file).


Lets make this two sections, maybe ### MongoDB and ### PostgreSQL with instructions on how to use each. You can remove "We would like your help with supporting other databases."

dblock · 2017-02-14T17:03:57Z

database.yml

@@ -0,0 +1,18 @@
+default: &default


This file seems misplaced. I'd expect it to be in something like config folder.

dblock · 2017-02-14T17:04:30Z

database.yml

+development:
+  <<: *default
+  database: slack_ruby_bot_server_development
+  username: slack_ruby_bot_server


I don't think this is how PostgreSQL default config works, we want this code to be able to run PostgreSQL tests without any changes.

dblock · 2017-02-14T17:05:53Z

lib/slack-ruby-bot-server.rb


 require 'slack-ruby-bot-server/ext'
 require 'slack-ruby-bot-server/version'
 require 'slack-ruby-bot-server/info'
-require 'slack-ruby-bot-server/models'
+
+if SlackRubyBotServer::Config.mongo?


Create two files, maybe config/databases/mongo.rb and config/databases/postgresql.rb and move all this logic there. The require should be something like require 'config/databases/SlackRubyBotServer::Config.database_type.

dblock · 2017-02-14T17:10:14Z

sample_apps/sample_app_mongo/Gemfile

@@ -1,8 +1,12 @@
 source 'https://rubygems.org'


mongo => mongoid

dblock · 2017-02-14T17:10:40Z

spec/models.rb

@@ -0,0 +1,2 @@
+class Team < ActiveRecord::Base


This shouldn't be needed.

dblock · 2017-02-14T17:10:58Z

spec/schema.rb

@@ -0,0 +1,15 @@
+ActiveRecord::Schema.define do


This should be in a pg-related subfolder.

dblock · 2017-02-14T17:11:33Z

spec/spec_helper.rb

+files       = Dir[File.join(__dir__, 'support', '**/*.rb')]
+mongo_files = ["#{File.dirname(__FILE__)}/support/mongoid.rb"]
+
+if defined? Mongo


Spearate into subfolders and load, eg. "spec/support/databases/mongoid.rb" and "pg.rb"

dblock · 2017-02-14T17:11:51Z

spec/support/database_cleaner.rb

@@ -7,7 +7,7 @@
  end

  config.after :suite do
-    Mongoid.purge!
+    Mongoid.purge! if defined?(Mongo)


Move into the support folder's per-database file.

dblock · 2017-02-14T17:13:03Z

Most importantly we need a build here that keys of DATABASE_ADAPTER=mongoid or DATABASE_ADAPTER=pgsql or similar in .travis.yml so that we can make sure all tests pass against one and the other.

dblock · 2017-02-14T17:13:30Z

Also please see comments in #38, a lot of the changes I'd like are still relevant here.

spencerldixon · 2017-02-16T00:42:03Z

Thanks for the feedback @dblock, I'll get on it. Still not entirely sure how everything works but it's really interesting figuring it out, so please bare with me. One more thing, I'd like to use this in a Rails app I'm working on, would that be possible? Perhaps we could include some better instructions for doing this too

dblock · 2017-02-17T02:25:59Z

Lets table the whole Rails thing for later, it's a whole other ballgame. Generally I would just use slack-ruby-bot in a Rails app.

spencerldixon · 2017-02-17T13:24:10Z

Brilliant, I'm keen to work on rails integration too, I'm trying to create a bot app with rails that will need to support a personal assistant bot for individual members across multiple teams. Perhaps we can open an issue to start discussing this too, I know scale will also be an issue. Anyway, I've made a few changes, let me know your thoughts so far and I'll continue working on this

dblock

Good! Your build with PostgreSQL is passing, so it's encouraging. Now you just need to re-add mongodb in the build matrix and get the whole thing green.

I made some naming suggestions, I think it's important to get names right early, but none of them are blockers to get this merged, the build however is.

dblock · 2017-02-17T14:32:59Z

.travis.yml

@@ -21,3 +21,6 @@ matrix:
 before_install:
  - "export DISPLAY=:99.0"
  - "sh -e /etc/init.d/xvfb start"
+
+env:
+  - DB=postgres


Since there's a matrix above, you should modify the matrix to include 1 of each, something like

matrix: include: - rvm: 2.3.1 script: - bundle exec danger - rvm: 2.3.1 env: - DATABASE_ADAPTER=pg - rvm: 2.3.1 env: - DATABASE_ADAPTER=mongoid

For now feel free to just remove ruby-head and jruby-head.

dblock · 2017-02-17T14:33:42Z

README.md

-Install [MongoDB](https://www.mongodb.org/downloads), required to store teams. We would like your help with [supporting other databases](https://github.com/slack-ruby/slack-ruby-bot-server/issues/12).
+### MongoDB
+
+Install [MongoDB](https://www.mongodb.org/downloads), required to store teams.


Add a note to use mongoid.

dblock · 2017-02-17T14:34:03Z

README.md

+
+### PostgreSQL
+
+You can also use PostgreSQL. See the PostgreSQL example - ensure you've filled out postgresql.yml and have setup your database correctly (user with the right name and privileges according to the file).


I would just remove "You can also use PostgreSQL" and make it similar to the mongoid instructions in every way.

dblock · 2017-02-17T14:34:52Z

config/databases/mongo.rb

@@ -0,0 +1,3 @@
+require 'slack-ruby-bot-server/models/team/mongo.rb'


Just for consistency sake it's easier if the ENV variable keys the names, if DATABASE_ADAPTER=mongoid then this file should be called database_adapters/mongoid.rb.

…ods to reflect the adapters, from postgres to pg, and mongo to mongoid

spencerldixon · 2017-02-20T20:51:38Z

Ok getting closer, I've moved everything to database_adapters folders and renamed the main config methods from postgresql to pg and mongo to mongoid to better reflect this. Let me know if I've missed anything though. Not sure how to go about hooking up the DATABASE_ADAPTER env in .travis.yml though to get this to run against the tests?

dblock · 2017-02-21T13:58:19Z

lib/slack-ruby-bot-server/config.rb

@@ -11,20 +11,20 @@ def reset!

    def database_adapter
      database_adapter = if defined?(::Mongoid)


This should be @database_adapter ||= ....

dblock · 2017-02-21T13:59:51Z

spec/spec_helper.rb

-elsif SlackRubyBotServer::Config.mongo?
-  require 'spec/support/databases/mongo'
+if SlackRubyBotServer::Config.pg?
+  require 'spec/support/database_adapters/pg/*.rb'


I didn't know you could do a require with *.rb, learning every day :)

This isn't a big deal, but it makes the order of requires unpredictable inside that folder. I would require .../pg.rb and inside pg.rb require anything that it may need.

dblock · 2017-02-21T14:00:56Z

Getting really close! For rubocop, run rubocop -a, then rubocop --auto-gen-config.

dblock · 2017-02-21T14:03:52Z

lib/slack-ruby-bot-server/app.rb

@@ -2,7 +2,6 @@ module SlackRubyBotServer
  class App


Should break this up and out into database-specific classes. For example you could have an Adapter class with a common interface, and SlackRubyBotServeR::Config.database_adapter would no longer be an enum, but an actual instance of that thing that you can call methods like check! and prepare!.

dblock · 2017-02-21T14:12:36Z

lib/slack-ruby-bot-server/config.rb


    def reset!
      self.server_class = SlackRubyBotServer::Server
    end

+    def database_adapter


You could move this into an initializer. We call reset!, so maybe just set it there?

dblock · 2017-03-06T19:59:40Z

Made some progress from here in #48, closing this one. Still some test failures remaining.

dblock · 2017-03-12T15:09:40Z

Released AR support in 0.6.0.

zachfeldman and others added 5 commits November 26, 2016 13:41

Add Postgres support, reference slack-ruby#7.

916c23c

Add CHANGELOG.md entry, minor cleanup.

fd50c06

Updates in response to @dBlocks comments.

fdf0a15

Minor edits.

26348b2

First attempt at dynamically loading which database to test against

0541b4e

dblock requested changes Feb 14, 2017

View reviewed changes

zachfeldman and others added 6 commits February 15, 2017 23:22

Add Postgres support, reference slack-ruby#7.

faacce1

Add CHANGELOG.md entry, minor cleanup.

1b79f5b

Updates in response to @dBlocks comments.

c2910fe

Minor edits.

e1c7b67

First attempt at dynamically loading which database to test against

011bfeb

Rebased to avoid conflicts

7f171df

Working changes to split database into folders for mongo and postgres

46446e1

dblock requested changes Feb 17, 2017

View reviewed changes

Moved everything to db specific folders, also renamed the config meth…

176b829

…ods to reflect the adapters, from postgres to pg, and mongo to mongoid

Fixed travis indentation

a607a36

dblock reviewed Feb 21, 2017

View reviewed changes

spencerldixon added 2 commits February 21, 2017 14:17

Fixed rubocop errors

e6a67be

Fixed indentation typo

08023e0

dblock closed this Mar 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add postgres support with tests #44

Add postgres support with tests #44

spencerldixon commented Feb 14, 2017

dblock commented Feb 14, 2017

dblock left a comment

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock Feb 14, 2017

dblock commented Feb 14, 2017

dblock commented Feb 14, 2017

spencerldixon commented Feb 16, 2017

dblock commented Feb 17, 2017

spencerldixon commented Feb 17, 2017

dblock left a comment

dblock Feb 17, 2017

dblock Feb 17, 2017

dblock Feb 17, 2017

dblock Feb 17, 2017

spencerldixon commented Feb 20, 2017

dblock Feb 21, 2017

dblock Feb 21, 2017

dblock commented Feb 21, 2017

dblock Feb 21, 2017

dblock Feb 21, 2017

dblock commented Mar 6, 2017

dblock commented Mar 12, 2017

		@@ -31,6 +31,8 @@ You can use the [sample application](sample_app) to bootstrap your project and s

		Install [MongoDB](https://www.mongodb.org/downloads), required to store teams. We would like your help with [supporting other databases](https://github.com/slack-ruby/slack-ruby-bot-server/issues/12).

		You can also use PostgreSQL. See the PostgreSQL example - ensure you've filled out postgresql.yml and have setup your database correctly (user with the right name and privileges according to the file).


		### PostgreSQL

		You can also use PostgreSQL. See the PostgreSQL example - ensure you've filled out postgresql.yml and have setup your database correctly (user with the right name and privileges according to the file).

		@@ -0,0 +1,3 @@
		require 'slack-ruby-bot-server/models/team/mongo.rb'

		@@ -11,20 +11,20 @@ def reset!

		def database_adapter
		database_adapter = if defined?(::Mongoid)

Add postgres support with tests #44

Add postgres support with tests #44

Conversation

spencerldixon commented Feb 14, 2017

dblock commented Feb 14, 2017

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Feb 14, 2017

dblock commented Feb 14, 2017

spencerldixon commented Feb 16, 2017

dblock commented Feb 17, 2017

spencerldixon commented Feb 17, 2017

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerldixon commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Mar 6, 2017

dblock commented Mar 12, 2017