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

Introduce MaintenanceTasks::Task.rescue_from #1150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

andrewn617
Copy link
Contributor

@andrewn617 andrewn617 commented Jan 21, 2025

By default any errors encountered while processing an iteration will be raised and the task with error. In our app we noticed a lot developers writing very similar code to rescue some exceptions, report them and moving on to the next iteration. So, let's introduce a helper to make this easier.

We introduced a report_on method to MaintenancneTask::Tasks. Exceptions passed to this method will be rescued during iteration and reported the rails error reporter, then iteration will continue.

Copy link
Member

@kmcphillips kmcphillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this feature.

app/models/maintenance_tasks/task.rb Outdated Show resolved Hide resolved
@andrewn617
Copy link
Contributor Author

I'd like to do this just be including ActiveSupport::Rescuable now that I think about it, but then I can't have the default behaviour since it doesn't support that.

@andrewn617 andrewn617 force-pushed the rescue-from branch 5 times, most recently from 665666b to 8cdc2db Compare January 21, 2025 17:53
@andrewn617
Copy link
Contributor Author

@kmcphillips I added support for 6.1 (and tested it locally) but ci just seems broken for 6.1 🤔 So maybe we should just remove it.

README.md Outdated Show resolved Hide resolved
README.md Outdated
the task with error. Errors can be rescued using the `rescue_from` handler. Errors
rescued this way will be reported by the handler and iteration will continue.

The default handler just calls `Rails.error.report` with the raised error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The default handler just calls `Rails.error.report` with the raised error.
The default handler calls `Rails.error.report` with the raised error:

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/jobs/maintenance_tasks/task_job_test.rb Outdated Show resolved Hide resolved
test/jobs/maintenance_tasks/task_job_test.rb Outdated Show resolved Hide resolved
MaintenanceTasks::Task.report_on takes a list of exceptions. When the task is iterating
those exceptions will be rescued and reported to the error reporter, then iteration will
continue.
@andrewn617
Copy link
Contributor Author

After some discussion, I am going to use ActiveSupport::Rescuable and a method on top of rescue_from called report_on that reports to Rails.error.report.

For consistency's sake, I deprecated the MaintenanceTasks.error_handler in favour of Rails.error.report in #1152.

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.

4 participants