search

nikhaldi / exception_notification-rake

Sends mail upon failures in Rake tasks
MIT License
39 stars 14 forks source link

Patch Rake::Task#execute instead of Rake::Application#display_error_message #27

Open TylerRick opened 5 years ago

TylerRick commented 5 years ago

This is based on how https://github.com/airbrake/airbrake/blob/master/lib/airbrake/rake.rb does it.

Fixes #26

TylerRick commented 5 years ago

Someone should just fork Airbrake and replace Airbrake-specific things with ExceptionNotifier. That way we benefit from all the work they do upstream (Airbrake) — including all their test coverage!

TylerRick commented 5 years ago

Manually tested using https://github.com/TylerRick/exception_notification-rake_example_with_spring/tree/workaround_using_execute_patch

Would be nice to have integration tests like Airbrake has. I really wonder if forking Airbrake would be the most efficient route.

TylerRick commented 5 years ago

Another unexpected benefit of this PR is that it appears to use my local time zone now. (I'm not sure why this changed ... maybe ExceptionNotifier uses the timestamp key if it is passed in the data hash?)

Before: A RuntimeError occurred in background at 2019-05-21 01:16:26 UTC : Now: A RuntimeError occurred in background at 2019-05-20 18:16:44 -0700 :

nikhaldi commented 5 years ago

Thanks @TylerRick - this is quite a fundamental change. Agreed that having integration tests would be really useful for this kind of thing. Do you have any ideas how to achieve that?

Can you also rebase this on top of master so we get a clearer diff (your other PRs are merged)?

TylerRick commented 5 years ago

All right, it's rebased :)

It's basically just copying how they do it in https://github.com/airbrake/airbrake/blob/master/lib/airbrake/rake.rb, since they no longer do it the way they used to (the way that originally inspired this gem, I believe)...

TylerRick commented 1 year ago

All right, it's rebased again!

TylerRick commented 1 year ago

FYI: This PR (and this gem in general) may be redundant now that https://github.com/smartinez87/exception_notification/pull/536 has been merged