Implement general entity registration for the GitHub provider #4376

JAORMX · 2024-09-05T08:51:32Z

Summary

This moves around the webhook handling code into the general entity
registration interface. Thus removing the need for the webhook manager.
Instead, each provider implements it's own registration, which could
be webhook-based.

The repo service was moved outside of the github package since it can
now work in a more general manner.

Fixes #4326

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

stacklok-cloud-staging · 2024-09-05T08:51:38Z

Minder Vulnerability Report ✅

Minder analyzed this PR and found it does not add any new vulnerable dependencies.

Vulnerability scan of b0d4f902:

🐞 vulnerable packages: 0

🛠 fixes available for: 0

evankanderson

I love that this is overall producing less code, and providing a general cleanup / garbage-collection mechanism.

evankanderson · 2024-09-05T20:12:32Z

internal/providers/github/entities.go

+		return errors.New("repo owner property not found")
+	}
+
+	hookIDP := props.GetProperty(ghprop.RepoPropertyHookId)


Are these the same properties used for selectors? Does this mean that I could write a selector on the GitHub webhook ID?

I'm not trying to stir things up, just trying to test my understanding of the changes.

Yes they are, we are piggy backing that functionality. We are yet to define a way to filter out properties for certain functionality. One case where that would be handy is displaying: We'd want to mark some properties as displayable and some not. Being able to filter in selectors might be a similar if not the same.

specifically for the hook ID - since this is not an attribute that you can get with an upstream call (well you can list all the webhooks but then you get them all and you need to figure out yourself which hooks are yours), the properties module treats the attribute as "operational", that is, it won't ever try to fetch it from provider and if replacing attributes for an entity, the property module won't replace an operational attribute.

So an easy thing we could do would be to just not pass the operational properties to the selectors.

evankanderson · 2024-09-05T20:17:13Z

database/query/entities.sql

+-- GetEntitiesByProvider retrieves all entities of a given provider.
+-- this is how one would get all repositories, artifacts, etc. for a given provider.
+
+-- name: GetEntitiesByProvider :many


Can / should we include a project ID in here to avoid potential confused-deputy problems like GHSA-v627-69v2-xx37 ? (I realize we might have other defenses, but that bug has made me a bit paranoid.)

Sure, we can! I regret not having an index for provider + project.

oh! Yeah, this is going to get tricky since a provider can live in a parent project, and the entities originating from that provider can be in subprojects. Now I remember why this was done without the ProjectID as part of the query.

should we pass a list of projects that are subprojects of the provider?
(I am guilty of removing the project ID from the getByID call as well thinking that "well, the ID is unique" and forgot about that potential issue as well)

coveralls · 2024-09-06T08:54:14Z

coverage: 53.257% (-0.5%) from 53.722%
when pulling 36c7449 on register-entities
into 614ba33 on main.

jhrozek · 2024-09-06T12:58:32Z

internal/providers/github/entities.go

+		return errors.New("repo owner property not found")
+	}
+
+	hookIDP := props.GetProperty(ghprop.RepoPropertyHookId)


specifically for the hook ID - since this is not an attribute that you can get with an upstream call (well you can list all the webhooks but then you get them all and you need to figure out yourself which hooks are yours), the properties module treats the attribute as "operational", that is, it won't ever try to fetch it from provider and if replacing attributes for an entity, the property module won't replace an operational attribute.

So an easy thing we could do would be to just not pass the operational properties to the selectors.

jhrozek · 2024-09-06T13:02:14Z

internal/providers/github/entities.go

+
+	err := c.DeleteHook(ctx, repoOwner, repoName, hookID)
+	if err != nil {
+		return fmt.Errorf("error deleting hook: %w", err)


Should we treat this error that calls upstream and the errors where we don't have the required properties differently? e.g. the upstream call error could be transient and the user might just want to retry but if the properties are not there, then there's no point in retrying, shouldn't we just proceed with deleting the entity in that case?

jhrozek · 2024-09-06T13:03:47Z

database/query/entities.sql

+-- GetEntitiesByProvider retrieves all entities of a given provider.
+-- this is how one would get all repositories, artifacts, etc. for a given provider.
+
+-- name: GetEntitiesByProvider :many


should we pass a list of projects that are subprojects of the provider?
(I am guilty of removing the project ID from the getByID call as well thinking that "well, the ID is unique" and forgot about that potential issue as well)

This moves around the webhook handling code into the general entity registration interface. Thus removing the need for the webhook manager. Instead, each provider implements it's own registration, which _could_ be webhook-based. The repo service was moved outside of the `github` package since it can now work in a more general manner. Fixes #4326 Signed-off-by: Juan Antonio Osorio <[email protected]>

jhrozek

This is great. We can remove some more references to github in a follow up - e.g. I think we should no longer need pbRepoFromProperties but instead could call RepoV1FromProperties and remove some more references to github from the generic code. But that's not blocking really.

JAORMX force-pushed the register-entities branch 6 times, most recently from 8c236a4 to 61a3232 Compare September 5, 2024 12:07

evankanderson reviewed Sep 5, 2024

View reviewed changes

JAORMX force-pushed the register-entities branch 3 times, most recently from d0257ab to b0d4f90 Compare September 6, 2024 08:44

JAORMX force-pushed the register-entities branch from b0d4f90 to 21fbfd6 Compare September 6, 2024 11:34

JAORMX marked this pull request as ready for review September 6, 2024 12:24

jhrozek reviewed Sep 6, 2024

View reviewed changes

JAORMX force-pushed the register-entities branch from 21fbfd6 to ef0f33f Compare September 9, 2024 10:39

JAORMX requested a review from jhrozek September 9, 2024 11:10

JAORMX force-pushed the register-entities branch from ef0f33f to 36c7449 Compare September 9, 2024 11:14

jhrozek approved these changes Sep 9, 2024

View reviewed changes

JAORMX merged commit 23f5cc1 into main Sep 9, 2024
21 checks passed

JAORMX deleted the register-entities branch September 9, 2024 12:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement general entity registration for the GitHub provider #4376

Implement general entity registration for the GitHub provider #4376

JAORMX commented Sep 5, 2024

stacklok-cloud-staging bot commented Sep 5, 2024 •

edited

Loading

Vulnerability scan of `b0d4f902:`

evankanderson left a comment

evankanderson Sep 5, 2024

JAORMX Sep 6, 2024

jhrozek Sep 6, 2024

evankanderson Sep 5, 2024

JAORMX Sep 6, 2024

JAORMX Sep 6, 2024

jhrozek Sep 6, 2024

coveralls commented Sep 6, 2024 •

edited

Loading

jhrozek Sep 6, 2024

jhrozek Sep 6, 2024

jhrozek Sep 6, 2024

jhrozek left a comment

Implement general entity registration for the GitHub provider #4376

Implement general entity registration for the GitHub provider #4376

Conversation

JAORMX commented Sep 5, 2024

Summary

Change Type

Testing

Review Checklist:

stacklok-cloud-staging bot commented Sep 5, 2024 • edited Loading

Minder Vulnerability Report ✅

Vulnerability scan of b0d4f902:

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhrozek left a comment

Choose a reason for hiding this comment

stacklok-cloud-staging bot commented Sep 5, 2024 •

edited

Loading

Vulnerability scan of `b0d4f902:`

coveralls commented Sep 6, 2024 •

edited

Loading