mozilla / jira-bugzilla-integration

Jira Bugzilla Integration (JBI) - system to sync bugs and issues
Mozilla Public License 2.0
8 stars 22 forks source link

Fix #702: heartbeat now returns 200 on warnings #862

Closed leplatrem closed 6 months ago

leplatrem commented 6 months ago

Fix #702

What's changed:

Questions:

Todo Next:

leplatrem commented 6 months ago

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 commented 6 months ago

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 https://github.com/mozilla/jira-bugzilla-integration/pull/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".