refactor Monalisten client #368
Description
Currently, the Monalisten client is created - nested in the github_integration webhook logic:
discord-bot/app/components/github_integration/webhooks/core.py
Lines 37 to 42 in 67c130d
This client should not be created and passed around globally. It should instead be initialized and setup elsewhere. There are a few issues with the current setup.
Note
PR should wait until monalisten 0.3.x comes out and is integrated in this repo.
Objectives
- Import should only go down the hierarchy, not back up
- client is defined in
./webhook/core.pybut is imported into../commits.py
- client is defined in
- Try to figure out a way to unload hooks when reloading an extension that uses the client. Related Loading and unloading extension logic #352
- All valid extensions should be tied to a single, valid cog (remove
github_integration/__init__.py:setup)
Potential solution(s)
Pass the client into the initialization of the bot, as an argument → setup hooks - from a helper function - during on_ready → setup bot reference to the listen background task → consume client only from reference to bot
To do this, the client could be setup inside config.py or it can be done inside __main__.py. I feel like we should remove global setup where possible. gh gets away from it because it was a trade-off during the large refactor.
The question then becomes what to do about internal event handler hooks.
Potential setup function example
def setup(client: Monalisten) -> None:
@client.on_internal("error")
async def _(
event_data: dict[str, Any],
message: str,
pydantic_errors: list[ErrorDetails] | None,
) -> None:
msg = f"{message}\n{event_data}\n{pydantic_errors or []}"
raise RuntimeError(msg)
@client.on_internal("auth_issue")
async def _(issue: AuthIssue, event_data: dict[str, Any]) -> None:
guid = event_data.get("x-github-delivery", "<missing-guid>")
logger.warning("token {} in event {}: {}", issue.value, guid, event_data)
@client.on_internal("ready")
async def _() -> None:
logger.info("monalisten client ready")With this, who should call this setup function? The __main__.py file that passes in a constructed client? Or should this be done inside the bot logic itself?
I don't think there is a perfect clean solution. Maybe something would be easier to do if you could create a class that inherits the Monalisten class and overrides the internal event handlers. However, these are not functions but instead a typed dict of events → list of callable.
Another route you can go, is to construct many clients. One for each user (cog). That way, when the cog is unloaded, you don't have to do anything extra for ensuring that hooks are not pointing to removed functions. The setup logic would still need to be done for handling internal events (errors). But this can be abstracted to some other function.
Example setup function
def setup() -> Monalisten:
monalisten_client = Monalisten(
config.github_webhook_url.get_secret_value(),
token=config.github_webhook_secret.get_secret_value()
if config.github_webhook_secret
else None,
)
@monalisten_client.on_internal("error")
async def _(
event_data: dict[str, Any],
message: str,
pydantic_errors: list[ErrorDetails] | None,
) -> None:
msg = f"{message}\n{event_data}\n{pydantic_errors or []}"
raise RuntimeError(msg)
@monalisten_client.on_internal("auth_issue")
async def _(issue: AuthIssue, event_data: dict[str, Any]) -> None:
guid = event_data.get("x-github-delivery", "<missing-guid>")
logger.warning("token {} in event {}: {}", issue.value, guid, event_data)
@monalisten_client.on_internal("ready")
async def _() -> None:
logger.info("monalisten client ready")
return monalisten_clientOn cog load, you add the client's listener to the background task list (would need to be a dict now) and on unload, you remove it. I really like this solution.
The main drawback to this solution is that you will be creating multiple consumers of the same source. I don't know what type of strain that puts on the webhook. I know it is possible since multiple bots have been running at the same time, responding to webhook events (I had mine running while trag1c was testing and both got the full event with no issue).