Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Dec 28, 2020

Conversation

petergoldstein
Copy link
Collaborator

@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
Copy link
Collaborator Author

That fixes the ActiveJob spec issue.

@petergoldstein
Copy link
Collaborator Author

@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 #160 , so that connection exclusion is picked up.

@shadabahmed
Copy link
Owner

Hi @petergoldstein . Thanks for putting effort on this one. I was also working parallely on this PR - #161. I will try to merge all three - yours, mine and #160

@petergoldstein
Copy link
Collaborator Author

@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
Copy link
Owner

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

@shadabahmed shadabahmed merged commit 3848c28 into shadabahmed:master Dec 28, 2020
@petergoldstein petergoldstein deleted the feature/rails_6_1 branch December 28, 2020 05:33
@petergoldstein
Copy link
Collaborator Author

@shadabahmed Thanks for getting this out there so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants