Skip to content

Conversation

@davezuckerman
Copy link
Contributor

Adds healtcheck to verify Mail password is set

added test for new mail password healthcheck

removed stubbing for gpg key
Copy link
Member

@awilfox awilfox left a comment

Choose a reason for hiding this comment

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

r+, this addresses the ticket's ask.

I still think it would be useful to eventually grow a check that tries to connect to SMTP. It doesn't need to send a message (as we don't want to waste email traffic), but it could AUTH, wait for a 2.7.0, then NOOP to ensure the connection works. That can be later.

Copy link
Member

@danschmidt5189 danschmidt5189 left a comment

Choose a reason for hiding this comment

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

The ticket asks to test that the credentials are valid ("Credential invalidity (e.g. on expiration) is an important case for this check, so it should be covered.") so this actually doesn't meet the requirement.

You can get the app's SMTP configuration via ActionMailer::Base.smtp_settings, then use that to configure a test connection via the underlying Net::SMTP library. (I don't think you can get that object from ActionMailer, so you do have to create it yourself; I'd check the ActionMailer internals to make sure your method matches what it does internally.)

@davezuckerman
Copy link
Contributor Author

I changed it so it's making a real smtp call. I think custom_mailer that was added makes the action_mailer in okcomputer call unnecessary since it's only doing a ping. Should I take that out?

Copy link
Member

@awilfox awilfox left a comment

Choose a reason for hiding this comment

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

Question on logging exceptions, otherwise looks great.

get '/health'
expect(response.parsed_body.keys).to match_array %w[
action-mailer
custom-mailer
Copy link
Member

Choose a reason for hiding this comment

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

One or the other of these will need to be removed once you consolidate the mailer checks.

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