app: Convert health checks to use OKComputer #2
Conversation
awilfox
requested review from
anarchivist,
danschmidt5189,
davezuckerman and
jason-raitz
awilfox
force-pushed
the
awilfox/AP-508-okcomputer
branch
from
1275c46 to
c632f60
Compare
danschmidt5189
left a comment
danschmidt5189
left a comment
There was a problem hiding this comment.
Solid PR, just obviously needs to pass first.
Given that the ActionMailer check is blocked on external review, what do you think about marking the tests as pending (or otherwise reworking it to pass, tentatively)? The checks themselves are valuable when deployed so I'm not sure we want to wait on something we can't control, to provide test-only value that isn't that great IMO.
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
looks great. my comments are nonblocking.
config/initializers/okcomputer.rb
Outdated
| OkComputer.check_in_parallel = true | ||
|
|
||
| class AlmaPatronCheck < OkComputer::Check | ||
| TEST_PATRON_ID = '012158720' |
There was a problem hiding this comment.
I've copy-pasted this from the old health check; I have no idea its provenance. Interestingly enough, it was also present in lost-and-found's health check code until that was migrated to OKComputer as well, but it didn't do anything, it was just there.
There was a problem hiding this comment.
It was implemented in 29db9af by @danschmidt5189 in 2019. Running the API query myself, it seems to be valid.
There was a problem hiding this comment.
general comment that doesn't need to block merging: do we need checks on other external services? the list i'm thinking of here is tind, paypal, worldcat, hathitrust, whois.arin.net, and berkeley servicenow.
There was a problem hiding this comment.
I do agree that more external service checks would be useful, if for nothing more than a way to say "yes, our connection to XYZ is down". I figure these can be added as time permits. Checking ARIN will likely be easier than TIND, which will both be easier than PayPal, for example.
awilfox
force-pushed
the
awilfox/AP-508-okcomputer
branch
2 times, most recently
from
b174013 to
5585ab8
Compare
|
You have no idea how tempted I was to write Latest commit should resolve all the feedback. |
* The Alma patron fetch has been converted to a custom check. * ActionMailer is checked for connectivity. Note that the test is marked pending until emmahsax/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
force-pushed
the
awilfox/AP-508-okcomputer
branch
from
5585ab8 to
1371798
Compare
The Alma patron fetch has been converted to a custom check.
ActionMailer is checked for connectivity.
Note that this won't pass until emmahsax/okcomputer#21 is merged, because the
:testdelivery 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