Remove database access from GitHub webhook handler #4327
Assignees
Comments
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 12, 2024
…s to typed protobuf messages This will help us get rid of database and/or property calls in the webhook handler. Related: stacklok#4327
10 tasks
This was referenced Sep 17, 2024
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 17, 2024
When processing a webhook payload, we typically only have the information from the webhook payload available which is to say information about the upstream entity. We even don't know exactly which provider and which project are we handling. We used to sort of wing it in the github webhook handler and were just searching by upstream ID as if it was globally unique, but that no longer works with the introduction of multiple providers where several entities might have the same upstream ID, just in different providers. To handle that, we add a new method to the properties service that searches by upstream ID with a hint. At the moment, the interface supports providerID, projectID and provider implements as a hint. The immediate use is that the github provider will search by the upstream ID with the hint set to ProviderTypeGithub as that's implemented both by the app and OAuth github providers. If any attribute of the hint is set, then the entity's attribute must match the hint, otherwise the entity is filtered out. Additionally, once the entity is found, the hint might contain a property with a value that must match. The use-case there would be deleting entities where we want to ensure that we are deleting an entity with an appropriate upstream hook ID. If no entities match the hints or if multiple entities match a hint, an error is returned. Related: stacklok#4327
10 tasks
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
When processing a webhook payload, we typically only have the information from the webhook payload available which is to say information about the upstream entity. We even don't know exactly which provider and which project are we handling. We used to sort of wing it in the github webhook handler and were just searching by upstream ID as if it was globally unique, but that no longer works with the introduction of multiple providers where several entities might have the same upstream ID or the same name, just in different providers. To handle that, we add a new method to the properties service that searches by upstream properties with a hint. At the moment, the interface supports providerID, projectID and provider implements as a hint. The immediate use is that the github provider will search by the upstream ID with the hint set to ProviderTypeGithub as that's implemented both by the app and OAuth github providers. If any attribute of the hint is set, then the entity's attribute must match the hint, otherwise the entity is filtered out. Additionally, once the entity is found, the hint might contain a property with a value that must match. The use-case there would be deleting entities where we want to ensure that we are deleting an entity with an appropriate upstream hook ID. If no entities match the hints or if multiple entities match a hint, an error is returned. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
When processing a webhook payload, we typically only have the information from the webhook payload available which is to say information about the upstream entity. We even don't know exactly which provider and which project are we handling. We used to sort of wing it in the github webhook handler and were just searching by upstream ID as if it was globally unique, but that no longer works with the introduction of multiple providers where several entities might have the same upstream ID or the same name, just in different providers. To handle that, we add a new method to the properties service that searches by upstream properties with a hint. At the moment, the interface supports providerID, projectID and provider implements as a hint. The immediate use is that the github provider will search by the upstream ID with the hint set to ProviderTypeGithub as that's implemented both by the app and OAuth github providers. If any attribute of the hint is set, then the entity's attribute must match the hint, otherwise the entity is filtered out. Additionally, once the entity is found, the hint might contain a property with a value that must match. The use-case there would be deleting entities where we want to ensure that we are deleting an entity with an appropriate upstream hook ID. If no entities match the hints or if multiple entities match a hint, an error is returned. Related: stacklok#4327
jhrozek
added a commit
that referenced
this issue
Sep 19, 2024
* Rename EntityWithProperties to EntityWithPropertiesByID We're about to add a different `EntityWithPropertiesByUpstreamID` so let's make the names more descriptive. * Add EntityWithPropertiesByUpstreamHint When processing a webhook payload, we typically only have the information from the webhook payload available which is to say information about the upstream entity. We even don't know exactly which provider and which project are we handling. We used to sort of wing it in the github webhook handler and were just searching by upstream ID as if it was globally unique, but that no longer works with the introduction of multiple providers where several entities might have the same upstream ID or the same name, just in different providers. To handle that, we add a new method to the properties service that searches by upstream properties with a hint. At the moment, the interface supports providerID, projectID and provider implements as a hint. The immediate use is that the github provider will search by the upstream ID with the hint set to ProviderTypeGithub as that's implemented both by the app and OAuth github providers. If any attribute of the hint is set, then the entity's attribute must match the hint, otherwise the entity is filtered out. Additionally, once the entity is found, the hint might contain a property with a value that must match. The use-case there would be deleting entities where we want to ensure that we are deleting an entity with an appropriate upstream hook ID. If no entities match the hints or if multiple entities match a hint, an error is returned. Related: #4327 * getAllByProperty returns an empty list, not ErrEntityNotFound, their callers convert it to ErrEntityNotFound if needed
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 19, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 20, 2024
… and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: stacklok#4327
jhrozek
added a commit
that referenced
this issue
Sep 20, 2024
… and call another handler (#4545) * Add new watermill handlers that get or refresh entities by properties and call another handler These will be used in webhook handlers instead of calling the property service directly. The handlers follow the same base logic, just with "pluggable" ways of retrieving the entity, converting the entity to properties and which handler to call next. Related: #4327 * Rename owner to originator
|
Since the work breakdown we did initially was not great and it might have been a bit opaque what we did I'm going to break this issue into 2 - removing the database access from the GH provider was not strictly needed to enable github, but the generic entity lookup and evaluation enabled us to both implement the GL provider and make the GH provider use the same code. That part is now done and to make it clearer what was done and why I split that part of work into #4561 |
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 20, 2024
We added new watermill handlers that allow to refresh and evaluate a generic entity with properties. This is the first patch in a series that takes these handlers into account with the eventual goal of removing all the bespoke code from the GitHub webhook handler and eventually remove direct database access from the webhook handler. I'll expand the same method for the other webhook events we issue, but let's do small PRs so we can have some test runs in between them and revert in case of issues. Related: stacklok#4327
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 20, 2024
We added new watermill handlers that allow to refresh and evaluate a generic entity with properties. This is the first patch in a series that takes these handlers into account with the eventual goal of removing all the bespoke code from the GitHub webhook handler and eventually remove direct database access from the webhook handler. I'll expand the same method for the other webhook events we issue, but let's do small PRs so we can have some test runs in between them and revert in case of issues. Related: stacklok#4327
10 tasks
jhrozek
added a commit
to jhrozek/minder
that referenced
this issue
Sep 22, 2024
We added new watermill handlers that allow to refresh and evaluate a generic entity with properties. This is the first patch in a series that takes these handlers into account with the eventual goal of removing all the bespoke code from the GitHub webhook handler and eventually remove direct database access from the webhook handler. I'll expand the same method for the other webhook events we issue, but let's do small PRs so we can have some test runs in between them and revert in case of issues. Related: stacklok#4327
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Please describe the enhancement
Solution Proposal
Describe alternatives you've considered
No response
Additional context
No response
Acceptance Criteria
No response
The text was updated successfully, but these errors were encountered: