AP-534: Add additional health checks for external services #13
Conversation
|
@davezuckerman |
davezuckerman
left a comment
davezuckerman
left a comment
There was a problem hiding this comment.
Looks good. Just some minor changes as noted. There was a branched pushed to main yesterday dealing with okcomputer so some of these changes you put in may overlap a bit with that when you rebase.
config/initializers/okcomputer.rb
Outdated
| OkComputer::Registry.register 'database-migrations', OkComputer::ActiveRecordMigrationsCheck.new | ||
|
|
||
| # Ensure connectivity to the mail system. | ||
| OkComputer::Registry.register 'action-mailer', OkComputer::ActionMailerCheck.new |
There was a problem hiding this comment.
This is no longer needed because a custom mail-check was just checked in which covers this
config/initializers/okcomputer.rb
Outdated
|
|
||
| # Ensure TIND API is working. | ||
| tind_health_check_url = "#{Rails.application.config.tind_base_uri}api/v1/search?In=en" | ||
| OkComputer::Registry.register 'thind-api', BerkeleyLibrary::Util::HeadCheck.new(tind_health_check_url) |
There was a problem hiding this comment.
This should be "tind-api" right?
spec/request/okcomputer_spec.rb
Outdated
| database | ||
| alma-patron-lookup | ||
| database-migrations | ||
| thind-api |
There was a problem hiding this comment.
Should this be "'tind-api"?
spec/request/okcomputer_spec.rb
Outdated
| paypal-payflow | ||
| hathitrust-api | ||
| berkeley-service-now | ||
| action-mailer |
There was a problem hiding this comment.
per above comment. No longer needed.
spec/request/okcomputer_spec.rb
Outdated
| database | ||
| alma-patron-lookup | ||
| database-migrations | ||
| thind-api |
spec/request/okcomputer_spec.rb
Outdated
| hathitrust-api | ||
| berkeley-service-now | ||
| mail-connectivity | ||
| action-mailer |
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
Suggestion on the config, otherwise this looks good after @davezuckerman's comments are addressed.
config/application.rb
Outdated
| config.hathiTrust_health_check_url = 'https://catalog.hathitrust.org/api/volumes/full/oclc/424023.json' | ||
| config.whois_health_check_url = 'https://whois.arin.net/rest/poc/1AD-ARIN' | ||
| config.berkeley_service_now_health_check_url = 'https://berkeley.service-now.com/kb_view.do?sysparm_article=KB0011960' |
There was a problem hiding this comment.
Would it be better to define a health_check_url clade to keep them logically together? Something like:
| config.hathiTrust_health_check_url = 'https://catalog.hathitrust.org/api/volumes/full/oclc/424023.json' | |
| config.whois_health_check_url = 'https://whois.arin.net/rest/poc/1AD-ARIN' | |
| config.berkeley_service_now_health_check_url = 'https://berkeley.service-now.com/kb_view.do?sysparm_article=KB0011960' | |
| config.x.health_check_urls.hathiTrust = 'https://catalog.hathitrust.org/api/volumes/full/oclc/424023.json' | |
| config.x.health_check_urls.whois = 'https://whois.arin.net/rest/poc/1AD-ARIN' | |
| config.x.health_check_urls.berkeley_service_now = 'https://berkeley.service-now.com/kb_view.do?sysparm_article=KB0011960' |
Note that since we would be defining a sub level, config.x is required here.
|
I had missed your comment about rebasing. It looks like the rebase conflicts were properly resolved. Other than the "thind-api" vs. 'tind-api' naming and not needing the action-mailer (along with Anna's comments). This looks good |
3 participants
Add additional health check for below services: