Introduce separate method to fail and avoid monkey patching ActiveRecord::Base

[jobertabma]

We're leveraging this gem to notify developers of missing indexes during our CI runs. The way we've implemented it, is to overwrite the mega_fail! method in ConsistencyFail::Enforcer. This isn't ideal because this might break the implementation when new versions of this gem would be released. I'm thinking of creating a PR that introduces a separate method that fails hard instead of monkey patching the ActiveRecord::Base class, since there's no guarantee that it'll blow up in our test environment. How do others feel about this?

Partial copy of our initializer:

if Rails.env.test?
  require 'consistency_fail/enforcer'

  module ConsistencyFail
    class Enforcer
      class << self
        def mega_fail!
          fail 'You have got missing indexes! Run `bundle exec consistency_fail` to find ' \
            'and fix them.'
        end
      end
    end
  end

  ConsistencyFail::Enforcer.enforce!
end

[trptcolin]

👍 on overall direction for modularity so people can do what they want. I don’t think I ever honestly thought people would use the enforcer stuff! Most folks run it in CI via the script in bin I think, but no reason this thing couldn’t be a public interface. Another method of calling it I would reach for first for CI purposes would probably be a test that asserts the “problems” == [].

It looks like the Enforcer code has some duplication with lib/consistency_fail in collecting/reporting problems. I think introducing a method that takes a Reporter and returns “problems” (if the kind here https://github.com/trptcolin/consistency_fail/blob/master/lib/consistency_fail.rb) would be great as a refactoring to make this change easy. And for the Enforcer’s use case it could pass a Null Object Reporter.

Then some other new method could use that to assert that the problems are empty.

I’d like to keep this idea as distinct from the Enforcer, since it’ll be useful in other contexts besides the “blow everything up at runtime” scenario that the Enforcer code is meant to be all about.

Does all that make sense?

[jobertabma]

Most folks run it in CI via the script in bin I think, but no reason this thing couldn’t be a public interface

That makes sense. What we didn't realize when we set this up was that this only required the database server to be started, not Rails itself. This was an error on our side because some people ran the command locally without a running database, which results in a connection error. We might revisit the way we run it, since we do actually have a running database instance in our CI runs.

Does all that make sense?

Yes, thanks! I'll put up some code for you to review, but since we might also work around it by running a database server, this might take some time. Feel free to close this, too. I think the overall change is still valuable because people might've different needs, but it's definitely less urgent for us at this moment.

[trptcolin]

Yes, agreed, please keep it open even if you don't have time. Thanks!