ActionMailerCheck: Support :sendmail and :test - #21
Conversation
11d61cf to
9888f9a
Compare
* The Alma patron fetch has been converted to a custom check. * ActionMailer is checked for connectivity. Note that this won't pass until okcomputer-ruby/okcomputer#21 is merged, because the `:test` delivery method isn't recognised for the ActionMailer check. Once it is merged, we can update the Gemfile to use the new version and then the tests will pass. Ref: AP-508
* The Alma patron fetch has been converted to a custom check. * ActionMailer is checked for connectivity. Note that this won't pass until okcomputer-ruby/okcomputer#21 is merged, because the `:test` delivery method isn't recognised for the ActionMailer check. Once it is merged, we can update the Gemfile to use the new version and then the tests will pass. Ref: AP-508
* The Alma patron fetch has been converted to a custom check. * ActionMailer is checked for connectivity. Note that this won't pass until okcomputer-ruby/okcomputer#21 is merged, because the `:test` delivery method isn't recognised for the ActionMailer check. Once it is merged, we can update the Gemfile to use the new version and then the tests will pass. Ref: AP-508
* The Alma patron fetch has been converted to a custom check. * ActionMailer is checked for connectivity. Note that this won't pass until okcomputer-ruby/okcomputer#21 is merged, because the `:test` delivery method isn't recognised for the ActionMailer check. Once it is merged, we can update the Gemfile to use the new version and then the tests will pass. Ref: AP-508
* The Alma patron fetch has been converted to a custom check. * ActionMailer is checked for connectivity. Note that the test is marked pending until okcomputer-ruby/okcomputer#21 is merged, because the `:test` delivery method isn't recognised for the ActionMailer check. Once it is merged, we can update the Gemfile to use the new version and then the tests will pass. Ref: AP-508
|
@awilfox I am having trouble getting this PR to trigger the tests. Can you try making another commit to the PR and maybe I'll be able to trigger them. EDIT: I managed to trigger them. |
|
@awilfox I have addressed the tests in my latest PR that I merged. Can you sync your fork with |
9888f9a to
71b3361
Compare
|
Sorry, I didn't get emailed/notified about these pings. I've rebased now. Thank you! |
| mark_failure | ||
| end | ||
| when :test | ||
| mark_message "#{klass} is in test mode" |
There was a problem hiding this comment.
The case klass.delivery_method has no else/default branch. Any delivery method other than :smtp, :sendmail, or :test (e.g. :file, or third-party adapters like Postmark/SES/Resend) falls through and silently reports success? == true with message == nil — verified this live. For a health-check gem, silently reporting "healthy" without actually checking anything seems like a real problem. Worth adding an else branch that fails (or explicitly documents unsupported delivery methods as unsupported) rather than defaulting to success.
There was a problem hiding this comment.
Agreed and changed in latest revision.
| rescue => e | ||
| mark_message "#{klass} at #{host}:#{port} is not accepting connections: '#{e}'" | ||
| mark_failure | ||
| end |
There was a problem hiding this comment.
The :sendmail branch has no exception handling, unlike :smtp which keeps the original begin/rescue. If sendmail_settings[:location] is nil (a realistic misconfiguration — e.g. an app sets sendmail_settings = { arguments: [...] } without :location), File.executable?(nil) raises a TypeError. I reproduced this live, and it propagates unrescued through Check#run and CheckCollection (both check_in_sequence and check_in_parallel, since Thread#join re-raises), crashing the health-check endpoint instead of reporting a graceful failure. Worth wrapping this in a begin/rescue like the :smtp branch.
There was a problem hiding this comment.
Exceptions are handled in the latest revision.
This adds support for checking that the `sendmail` binary chosen is executable when ActionMailer is using the `:sendmail` delivery_method, and stubs out the check when using the `:test` delivery_method. Additionally, this change ensures that other delivery_methods fail explicitly instead of falling through and being silently ignored.
71b3361 to
adf30f9
Compare
|
v2:
Thank you for the detailed review! I have addressed all the feedback. |
2 participants
This adds support for checking that the
sendmailbinary chosen is executable when ActionMailer is using the:sendmaildelivery_method, and stubs out the check when using the:testdelivery_method.My original goal with this change was to make sure OKComputer was successful during RSpec tests for both RapidRetail and BerkeleyLibrary/framework, of which I am adding OKComputer checks to both and seeing RSpec failures when using the ActionMailer check. I realised
:sendmailwould be pretty simple to check too, so I added that as well.This does not add support for
:file, but I can add it if desired.