Take the property service into use in webhook and repository service #4299
Conversation
| minderv1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" | ||
| ) | ||
|
|
||
| type EntityInstance struct { |
There was a problem hiding this comment.
This is just a convenience struct to avoid returning and carrying around both db.EntityInstance and properties when handling an entity with properties. We might want to use the newly introduced protobuf message instead.
There was a problem hiding this comment.
I agree that we should not use the database structures but should instead have either a middle-struct (as you did here) or even the protobuf. I leave that to your discretion.
pkg/providers/v1/providers.go
Outdated
| @@ -56,6 +56,8 @@ type Provider interface { | |||
| // The name is used to identify the entity within minder and is how | |||
| // it will be stored in the database. | |||
| GetEntityName(entType minderv1.Entity, props *properties.Properties) (string, error) | |||
| // OperationalProperties returns the operational properties for an entity | |||
| OperationalProperties(entType minderv1.Entity) []string | |||
There was a problem hiding this comment.
Question: Can we come up with a way to expose the operational properties without adding a new method to the generic Provider interface? It feels like the interface is getting too big already.
There was a problem hiding this comment.
Why do we need to expose or filter them?
There was a problem hiding this comment.
We need to filter them because at the same time we need to be able to remove properties that were deleted upstream but we need to retain the operational attributes like hook ID.
Anyway, we discussed that offline and agreed to move the attributes handling to the provider completely so that minder-core is oblivious to their existence.
|
@JAORMX I think the only thing missing now is to fix up all the repeated webhook tests.. |
database/migrations/000102_properties_upstream_id_index.down.sql
Outdated
Show resolved
Hide resolved
If we had called these methods in a transaction, we would have effectivelly created another parallel transaction which might cause entities to not be referenced correctly.
These will be useful later.
This is just a shortcut to avoid passing the properties and project ID and entity etc around
This makes it easier to handle multiple entities inside the github provider as all that is needed is to implement the PropertyFetcher interface. It also makes moves the logic around formatting the name and what properties are operational into the provider to code that is per-entity.
minder saves attributes like hook ID which do not come from the upstream entity. We need to avoid deleting them on updates.
… repository service Related: mindersec#4179
Changes fetchRepo to refresh the repository being processed and return an EntityInstance model structure instead of a database repo Related: mindersec#4173
Summary
The PR moves us in the direction of using the generic entity tables for
repositories. On receiving a webhook, the repository entity along with the
properties are refreshed if needed (with caching provided by the property
service) and returns the entity and its properties. If this is a good
approach, we can also use it for repo reconciliation and profile
reconciliation. When sending the event, still the Repository protobuf message
is sent, but we could as well modify the entityInfoWrapper code to just accept
a message with the entity and properties in a more generic way.
The refresh is handled by a new method of the github repository service - we
could even use a more generic way and not code up a per-property, but we use
the repo-specific layer to migrate the "legacy" table entries to properties as
they are refreshed.
Because we store some attributes along with the repos that are not fetched
from upstream (webhook-related), we treat them as operational attributes and
carry them over during updates. What the operational attributes are is handled
by the provider without minder-core knowing or caring what they are.
Fixes: #4179
Change Type
Testing
unit test + manual testing
Review Checklist: