search

shadabahmed / logstasher

Awesome rails logs
MIT License
821 stars 136 forks source link

Initial Rails 6.1 changes in a way that is compatible with Rails 5.2 and 6.0 #161

Closed petergoldstein closed 3 years ago

petergoldstein commented 3 years ago

@shadabahmed This is an attempt to incorporate the Rails 6.1 changes so that the code can run in 5.2 and 6.0.

The major shift is in how the LogStasher::ActiveJob::LogSubscriber inheritance is managed. We use a variable for the parent class, which depends on the version of ActiveJob that is loaded.

This runs green with existing specs on Rails 5.2.x and 6.0.x, and has 6 failures on Rails 6.1.x. Roughly speaking these break down as:

  1. ActiveJob - a test helper issue, where perform_enqueued_jobs is asserting no errors, rather than raising the error as in previous versions (https://api.rubyonrails.org/classes/ActiveJob/TestHelper.html#method-i-perform_enqueued_jobs)
  2. MailerLogSubscriber - this likely needs an update for Rails 6.1 changes (ActiveMailbox) which are warned about on Rails 6.0.x
  3. Integration spec failures on custom fields - Honestly I don't really understand what this spec is supposed to be testing, so I'm at a bit of a loss.
petergoldstein commented 3 years ago

That fixes the ActiveJob spec issue.

petergoldstein commented 3 years ago

@shadabahmed The integration spec fix turned out to be easy once I looked at it - needed to allow the logger to receive debug. With that, I think this should work on all of Rails 5.2, 6.0, and 6.1.

I'd still suggest merging https://github.com/shadabahmed/logstasher/pull/160 , so that connection exclusion is picked up.

shadabahmed commented 3 years ago

Hi @petergoldstein . Thanks for putting effort on this one. I was also working parallely on this PR - https://github.com/shadabahmed/logstasher/pull/161. I will try to merge all three - yours, mine and #160

petergoldstein commented 3 years ago

@shadabahmed It may be tricky to merge this with #162 given they take such different approaches (#162 is essentially a Rails 6.1 only PR). I've rebased and believe that this should now contain a superset of the functionality in #162.

shadabahmed commented 3 years ago

Sounds good. Will merge this PR. Will also try to get my changes in if I rebase on top of yours

petergoldstein commented 3 years ago

@shadabahmed Thanks for getting this out there so quickly.