Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented HealthCheck Endpoint and improved Mailman resilience #3

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

knikolla
Copy link
Contributor

@knikolla knikolla commented Nov 3, 2020

The /v1/moc/HealthCheck endpoint will perform checks for various
external components that the service needs access to. Ex. SSH
connection to the mailing list server, which is the primary
source of flakiness at the moment.

Additionally, refactored the Mailman code into a separate class, to
facilitate testing and health checking.

Finally, made failing to subscribe a user to a mailing list not
be a blocking error.

The /v1/moc/HealthCheck endpoint will perform checks for various
external components that the service needs access to. Ex. SSH
connection to the mailing list server, which is the primary
source of flakiness at the moment.

Additionally, refactored the Mailman code into a separate class, to
facilitate testing and health checking.

Finally, made failing to subscribe a user to a mailing list not
be a blocking error.
Copy link

@rob-baron rob-baron left a comment

Choose a reason for hiding this comment

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

It reads well and looks ok.

On a broad topic, we need to decide if the "# TODO:" comments should also reference an issue.

if errors:
raise ConnectionError(errors)

# Note(knikolla): Not entirely sure if closing before reading is fine.

Choose a reason for hiding this comment

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

is this comment old, or is it referring to something that exec_command is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment is old. I also realized that I didn't delete the old unused methods in the action, which I was keeping as a reference while splitting out the functionality to its separate class. Will delete both the comment and those now.

@knikolla
Copy link
Contributor Author

With regards to your comment about TODOs, it makes sense to me moving forward now that we have an established issue/task/sprint tracker.

Copy link

@naved001 naved001 left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, pointed out some nit-picky things. pylint picked up a few more (indentation, docstrings) but don't really care about those.

]
)

def get(self, request, format=None):

Choose a reason for hiding this comment

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

request and format are unused.

format is a built-in so you might wanna call it something else if you do want to use that argument.

from confspirator import groups as config_groups
from confspirator import fields as config_fields
from rest_framework.response import Response
from adjutant.api import utils

Choose a reason for hiding this comment

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

unused import.


return Response({'errors': errors})

def _get_action_defaults(self, action):

Choose a reason for hiding this comment

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

this can be static.

self.client.close()

def _execute(self, command):
stdin, stdout, stderr = self.client.exec_command(command)

Choose a reason for hiding this comment

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

nit: stdin is unused. _ can be used for throaway values

@knikolla knikolla merged commit 9383514 into master Jan 7, 2021
@knikolla knikolla deleted the ssh_resilience branch January 10, 2021 21:36
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.

5 participants