Models not created even though event parsed successfully

[danielmcquillen]

Hi Joshua thanks for creating this really useful project.

I'm experimenting with it using a test GitHub app. I've configured the GITHUB_... env variables with the GitHub app information, and I'm able to receive events from GitHub with no problems.

However, the Installation and Repository models aren't created even though I can see the "installation " event has been parsed and stored as an Event Log instance (with event type "installation" and action "created"). Any thoughts on why the Installation and Repository models aren't being created?

[joshuadavidthomas]

Hmm, first thing that comes to mind would be if you're using the sync side of things but haven't set it in the GITHUB_APP dictionary in your settings. From the README:

Make sure your GITHUB_APP["WEBHOOK_TYPE"] setting matches your view choice:

Use "async" with AsyncWebhookView
Use "sync" with SyncWebhookView

As long as you have the app in your INSTALLED_APPS, which it sounds like you do given the event logs are showing up, the internal event handlers should automatically be registered provided you have the correct setting given whether you're using the sync or async flavor.

[joshuadavidthomas]

Relevant lines from the apps.py of the library:

https://github.com/joshuadavidthomas/django-github-app/blob/main/src/django_github_app/apps.py#L18-L21

[danielmcquillen]

Thanks for the quick reply, Joshua.

I do have "sync" set in my base.py:

GITHUB_APP = {
    ...
    "WEBHOOK_TYPE": "sync",  # Use "async" for ASGI projects or "sync" for WSGI projects
}

...so I'm not sure why an EventLog instance would be created but the Installation create_from_event method isn't eventually called.

Perhaps I'll fork and add some debug log messages to try to triage...

[joshuadavidthomas]

Yeah, a minimal reproduction would be great. I've got unit tests for the behavior that are passing, but unfortunately haven't gotten around to writing a real world integration test because of the complexity of it.

[danielmcquillen]

@joshuadavidthomas One thing I noticed is that there doesn't seem to be logic in SyncWebhookView's post() method that would create an Installation if it doesn't exist.

So when a preliminary "action":"added", "installation": {} event comes through, there doesn't seem to be logic branching from that initial post() method that would eventually lead to create_from_event on the Installation model's manager being called...

The get_from_event() is called, but that just returns None as the Installation doesn't yet exist...

[danielmcquillen]

...and to answer my own question, I understand now that one must use the management command in order to add an existing installation! The app won't create an Installation from a installation_repositories event from a pre-eixsting Installation.

[joshuadavidthomas]

Ah, yep if the app has already been installed in your account/org the necessary events to set up the models never fire. The docs could stand to be a bit clearer and more explicit around that. Glad you got it figured out!

[joshuadavidthomas]

So when a preliminary "action":"added", "installation": {} event comes through...

There isn't a "added" action for installations (https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation) or repositories (https://docs.github.com/en/webhooks/webhook-events-and-payloads#repository), I assume this is the "installation_repositories" event you're referencing here? (https://docs.github.com/en/webhooks/webhook-events-and-payloads#installation_repositories)

Edit: That's what I get for replying on my phone.. You said it was the installation_repositories event 🤦

Just thinking out loud, but I wonder if instead of splitting create_from_event / get_from_event across the different installation event handlers, what if there was a get_or_create_from_event method?

[joshuadavidthomas]

Added a bit more clarification in the README about new vs existing apps and what's needed in #99

[joshuadavidthomas]

Added a new (a)get_or_create_from_event method to the Installation manager that should make this easier in the future. Part of v0.9.0.