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

GoodJob::Bulk.enqueue not handling missing migrations #1010

Closed
dan-manges opened this issue Jul 17, 2023 · 2 comments · Fixed by #1011
Closed

GoodJob::Bulk.enqueue not handling missing migrations #1010

dan-manges opened this issue Jul 17, 2023 · 2 comments · Fixed by #1011

Comments

@dan-manges
Copy link

Hello! We just hit an error in production due to this error

ActiveModel::UnknownAttributeError: unknown attribute 'error_event' for GoodJob::Execution.

It looks like GoodJob tries to gracefully degrade when migrations are missing, but that logic isn't compatible with GoodJob::Bulk.enqueue.

A while ago we added a test in our test suite to catch missing migrations. We'd rather have our test suite fail than go to production with migrations missing, especially if there are edge cases where the graceful degradation isn't working properly. Unfortunately, because the check for this migration is happening under error_event_migrated? and not migrated?, our test didn't catch this before it went to production.

RSpec.describe "GoodJob" do
  it "is fully migrated" do
    GoodJob.constants.each { |c| GoodJob.const_get(c) }
    expect(GoodJob::BaseRecord.subclasses).to all(be_migrated)
  end

Is there a more proper way that we can add something to our test suite to make sure all of the migrations have been run?

@bensheldon
Copy link
Owner

@dan-manges thanks for reporting this! Migrations are intended to be "optional/non-breaking" within minor releases, so I'm considering this a bug. Sorry! I'll fix that immediately.

I do like your idea of exposing a method publicly to check. What do you think of something like GoodJob.pending_migrations?

@dan-manges
Copy link
Author

Thanks for the quick fix for this @bensheldon! We updated right away; sorry I didn't reply sooner. We also added a test to assert GoodJob.migrated? in our test suite – thanks for adding that!

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