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

[#35782] Allow loading seeds without ActiveJob (~> 5.2.3) #35896

Merged
merged 1 commit into from Apr 20, 2019

Conversation

Projects
None yet
4 participants
@jlw
Copy link
Contributor

commented Apr 8, 2019

Summary

This fixes #35782.

#34953 introduced a change to process engine's seed files with ActiveJob. However, ActiveJob has always been an optional framework, and this change (released in Rails 5.2.3) breaks rake db:seed (and rake db:setup frequently used in CI/CD) for any apps that are not loading ActiveJob.

This change should allow for engine seeding to continue with ActiveJob when available, and without ActiveJob when not available.

Other Information

This change should be applied to any future Rails 5.2 bug fix releases as well as Rails 6.x, but I'm not sure of the best way to go about that.

@rails-bot rails-bot bot added the railties label Apr 8, 2019

@@ -1511,21 +1537,25 @@ def app
end

# Restrict frameworks to load in order to avoid engine frameworks affect tests.
def restrict_frameworks
def restrict_frameworks(remove: [])

This comment has been minimized.

Copy link
@jlw

jlw Apr 8, 2019

Author Contributor

I started with the assumption that removing ActiveJob during the "boot" process would be the appropriate approach, but it was not enough as I noted on lines 906-908. As such, it's possible this part of the change is no longer necessary.

This comment has been minimized.

Copy link
@gmcgibbon

gmcgibbon Apr 9, 2019

Member

This might be tricky because we boot rails per test. If we can't recreate the NoMethodError we could try mocking it instead.

This comment has been minimized.

Copy link
@jlw

jlw Apr 12, 2019

Author Contributor

@gmcgibbon - I'm not super familiar with minitest anymore, and it looks like stubbing this out correctly is just not possible without bringing in another test double library, which doesn't makes sense. (I don't want to get into an argument about libraries, but in RSpec stubbing this out without affecting the rest of config would be straightforward.)

This comment has been minimized.

Copy link
@gmcgibbon

gmcgibbon Apr 12, 2019

Member

😄 Yes, rspec is much better at stubbing/mocking than minitest, I agree. Thinking further, you can just forgo mocking and do something like this:

test "seed data can be loaded when ActiveJob is not present" do
  @plugin.write "db/seeds.rb", <<-RUBY
    Bukkits::Engine.config.bukkits_seeds_loaded = true
  RUBY

  app_file "db/seeds.rb", <<-RUBY
    Rails.application.config.app_seeds_loaded = true
  RUBY

  boot_rails

  # In a real app, active_job would be undefined
  undefine_config_option(:active_job)

  Rails.application.load_seed
  assert Rails.application.config.app_seeds_loaded
  Bukkits::Engine.load_seed
  assert Bukkits::Engine.config.bukkits_seeds_loaded
end

private

  def undefine_config_option(name)
    Rails.application.config.class.class_variable_get(:@@options).delete(name)
  end

This comment has been minimized.

Copy link
@jlw

jlw Apr 12, 2019

Author Contributor

@gmcgibbon - that's great! I've added both of your suggestions, and will squash the commits and force push my branch after this is approved.

@rafaelfranca rafaelfranca requested a review from gmcgibbon Apr 8, 2019

@jlw jlw force-pushed the jlw:bug/active-jobless-seeds branch from c17b512 to 0e22fbe Apr 9, 2019

@@ -552,7 +552,7 @@ def load_seed
seed_file = paths["db/seeds.rb"].existent.first
return unless seed_file

if config.active_job.queue_adapter == :async
if config.try(:active_job).try(:queue_adapter) == :async

This comment has been minimized.

Copy link
@gmcgibbon

gmcgibbon Apr 9, 2019

Member

If this raises when invoking active_job, try is correct here, but I think &. is better for reading the queue adapter. That should exist when not nil.

This comment has been minimized.

Copy link
@jlw

jlw Apr 12, 2019

Author Contributor

@gmcgibbon - I tried &. first, but it is less forgiving than try and threw exceptions (at least in my imperfect test scenario).

This comment has been minimized.

Copy link
@gmcgibbon

gmcgibbon Apr 12, 2019

Member

Yes, I would expect it to throw an exception on the second option invoke. We still need try for the first call to suppress a potential NoMethodError. eg. config.try(:active_job)&.(:queue_adapter).

@@ -1511,21 +1537,25 @@ def app
end

# Restrict frameworks to load in order to avoid engine frameworks affect tests.
def restrict_frameworks
def restrict_frameworks(remove: [])

This comment has been minimized.

Copy link
@gmcgibbon

gmcgibbon Apr 9, 2019

Member

This might be tricky because we boot rails per test. If we can't recreate the NoMethodError we could try mocking it instead.

@bjeanes

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I think #35905 supersedes this, and is already merged.

@jlw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@bjeanes - #35905 is already merged, but it still expects ActiveJob to be loaded and will still throw an exception otherwise.

@bjeanes

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

@jlw jlw force-pushed the jlw:bug/active-jobless-seeds branch from 56900e7 to 36ee3e8 Apr 17, 2019

@jlw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@gmcgibbon, could you review this again for me?

Is there anything special to be done to help ensure that this fix is included in any future Rails 5.2.x release (instead of only being included in 6.x)?

@gmcgibbon
Copy link
Member

left a comment

👍 Tested this locally and seeding now runs without Active Job. Please squash your commits. I believe since this is a fix for a released bug, we also need a changelog to railties too. We should be able to backport your commit to 5-2-stable and release from there, no need to do anything extra.

@jlw jlw force-pushed the jlw:bug/active-jobless-seeds branch from 36ee3e8 to bfc7363 Apr 19, 2019

@jlw jlw force-pushed the jlw:bug/active-jobless-seeds branch from bfc7363 to b08daf4 Apr 19, 2019

@jlw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@gmcgibbon - I've rebased from the latest (at least for the moment) master and amended my commit with a changelog entry.

@gmcgibbon gmcgibbon merged commit d3dc651 into rails:master Apr 20, 2019

2 checks passed

buildkite/rails Build #60617 passed (8 minutes, 39 seconds)
Details
codeclimate All good!
Details
@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

@gmcgibbon did you backport this PR to 5-2-stable?

gmcgibbon added a commit that referenced this pull request May 7, 2019

Merge pull request #35896 from jlw/bug/active-jobless-seeds
[#35782] Allow loading seeds without ActiveJob (~> 5.2.3)
@gmcgibbon

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Sorry for the delay, backported in a6bf032

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.