Fix #702: heartbeat now returns 200 on warnings

[leplatrem]

Fix #702

What's changed:

• leverage dockerflow check system (body of heartbeat response has changed)
• return 200 when the status is warning only
• return warning from checks when only a portion of workflows are affected
• let dockerflow log messages and details

Questions:

• A lot of testing code in test_router.py asserts the content of the heartbeat response body. Shall we only leave the bare minimum (eg. all success, one warning, one error) instead of testing all combinations?
• This pattern to do injection in checks is not great:

def check_jira_all_project_issue_types_exist(service=None, actions=None):
    actions = actions or get_actions()
    service = service or get_service()
    ...

What would you recommend instead?
(Note: configuration.get_actions() is not cached)

Todo Next:

• Wait for Add integration for FastAPI mozilla-services/python-dockerflow#81 to leverage FastAPI heartbeat view

[grahamalama]

This pattern to do injection in checks is not great:

def check_jira_all_project_issue_types_exist(service=None, actions=None):
    actions = actions or get_actions()
    service = service or get_service()

What would you recommend instead?

I don't know how much better or worse this is, but here are some ideas:

• for providing actions, we can use the global constant we define on app initialization
• for providing services, maybe we could use register_partial?

So that'd be:

def check_jira_all_project_issue_types_exist(service, actions=ACTIONS):
    ...

checks.register_partial(check_jira_all_project_issue_types_exist, get_service())

but also, maybe these functions actually deserve to be service methods if we continually need to provide service as the first argument (though I don't know how compatible that is with the check registration system)

@checks.register(name="jira.all_project_issue_types_exist")
def check_jira_all_project_issue_types_exist(self, actions=ACTIONS):
    ...

[leplatrem]

The problem with register_partial is that the parameter evaluated at registration time, and our mock_jira and mock_bugzilla come too late to override them (in test_router.py for example).

An alternative would be to pass a callable:

@lru_cache(maxsize=1)
def get_service():
    return JiraClient()


def check_jira_connection(_get_service):
    service = _get_service()
    return []

checks.register_partial(check_jira_connection, get_service, name="jira.up")

but I'm not 100% convinced, so I did it in a separate commit in case we would want to revert it.

[grahamalama]

The problem with register_partial is that the parameter evaluated at registration time, and our mock_jira and mock_bugzilla come too late to override them (in test_router.py for example)

Ahh okay 👍🏻

but I'm not 100% convinced

Yeah, I'm not either. Nothing feels like the "right" solution here. Let's merge this with what feels "most right", then after #874, we can consider how to make further improvements.

I started to mess around with this branch, which I pushed up here if you're interested (and comparing the two).

So far, it ended up being more steps to the same problem of

parameter evaluated at registration time, and our mock_jira and mock_bugzilla come too late to override them

I think for that branch to work, we'd need to make a fixture that actually generates a new app for each test (by calling FastAPI(...))

So make that'd mean we make our app with a factory pattern. Like in jbi/app.py:

def make_app(*args, **kwargs):
    ...
    return FastAPI(...)


app = make_app(*args, **kwargs)

and in conftest:

def app:
    return make_app()

that way we could ensure the the client is patched before we register the checks.

But again, let's get this and #874 merged before we go down this road. We might even look to Python Dockerflow to figure out how to best handle "provide dependencies to checks that need them".