Webhook handler now processes installation_repositories events #3447

blkt · 2024-05-28T17:20:01Z

Summary

Such events are triggered when a new repository is added to or removed
from the list of repositories accessible to the app.

Fixes #3267

Change Type

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.

coveralls · 2024-05-30T08:13:19Z

coverage: 53.132% (+0.5%) from 52.676%
when pulling 432ec11 on issue-3267
into 54d663d on main.

internal/reconcilers/entity_add.go

blkt · 2024-05-31T15:26:10Z

internal/controlplane/handlers_githubwebhooks.go

+	// We might want to ignore this case since we can only delete
+	// repositories there were previously registered, which would
+	// be deleted by means of "meta" and "repository" events as
+	// well.


This is interesting: we run into a race condition if, for a given repository that was removed from the org, we get a meta before the installation_repositories event, or vice versa. The first deletion succeeds, while the second fails.

error deleting repository from DB: error retrieving repository: sql: no rows in result set

This is not a problem right now, as it is handled correctly by logging some errors, but we do retry the deletion a handful of times, which might cause issues at scale.

is the meta event for the webhook/installation and the installation_repositories for the repos only? Could we process one or the other? I think in either case they could be solved by a single db lookup, no? (if we are to add and the repo is already there, then skip, if we are to delete and the repo is already deleted, skip).

Either way, nice that you caught this issue already.

I have to test what happens if we revoke access to a repository without deleting it from GitHub.

I believe that we don't get the "meta" event in such a case, i.e. we have to handle "removed" action for "installation_repositories" events as well.

I'll test it manually and reply here.

blkt · 2024-05-31T15:28:42Z

internal/reconcilers/messages/messages.go

+// RepoReconcilerEvent is an event that is sent to the reconciler topic
+type RepoReconcilerEvent struct {
+	// Project is the project that the event is relevant to
+	Project uuid.UUID `json:"project"`
+	// Repository is the repository to be reconciled
+	Repository int64 `json:"repository" validate:"gte=0"`
+}
+
+// NewRepoReconcilerMessage creates a new repos init event
+func NewRepoReconcilerMessage(providerID uuid.UUID, repoID int64, projectID uuid.UUID) (*message.Message, error) {
+	evt := &RepoReconcilerEvent{
+		Repository: repoID,
+		Project:    projectID,
+	}
+
+	evtStr, err := json.Marshal(evt)
+	if err != nil {
+		return nil, fmt.Errorf("error marshalling init event: %w", err)
+	}
+
+	msg := message.NewMessage(uuid.New().String(), evtStr)
+	msg.Metadata.Set("provider_id", providerID.String())
+	return msg, nil
+}


This was moved here to solve a circular dependency between internal/reconcilers/reconcilers.go and internal/service/service.go

jhrozek

Nice work. Left some comments inline, but overall this PR is shaping quite nicely!

jhrozek · 2024-06-02T19:15:43Z

internal/controlplane/handlers_githubwebhooks.go

+
+	installationID := event.GetInstallation().GetID()
+	installation, err := s.store.GetInstallationIDByAppID(ctx, installationID)
+	if errors.Is(err, sql.ErrNoRows) {


Do we special-case this error somewhere?
I also wonder, does an unclaimed installation have an installation ID? (I think yes, but worth testing)

I'm not following, what is an unclaimed installation?

The one you create without logging to minder first, e.g by going to https://github.com/apps/minder-instance/installations/new

I'll test this.

internal/controlplane/handlers_githubwebhooks.go

jhrozek · 2024-06-02T19:16:57Z

internal/controlplane/handlers_githubwebhooks.go

+	}
+	if !installation.ProjectID.Valid {
+		return nil, errors.New("invalid project id")
+	}


we should also test how these behave for an unclaimed install - I /think/ it should be OK as we create this special project (as we saw during pair-session on Friday), but worth testing.

jhrozek · 2024-06-02T19:18:29Z

internal/controlplane/handlers_githubwebhooks.go

+			ctx,
+			repo,
+			event.GetAction(),
+			events.TopicQueueReconcileEntityAdd,


can the topic ever be something else (asking if it needs to be a parameter)

You're right, this can only be a events.TopicQueueReconcileEntityAdd.
Initially the routine was just one, this is a leftover.

jhrozek · 2024-06-02T19:31:37Z

internal/controlplane/handlers_githubwebhooks.go

+	// We might want to ignore this case since we can only delete
+	// repositories there were previously registered, which would
+	// be deleted by means of "meta" and "repository" events as
+	// well.


is the meta event for the webhook/installation and the installation_repositories for the repos only? Could we process one or the other? I think in either case they could be solved by a single db lookup, no? (if we are to add and the repo is already there, then skip, if we are to delete and the repo is already deleted, skip).

Either way, nice that you caught this issue already.

Such events are triggered when a new repository is added to or removed from the list of repositories accessible to the app.

blkt

Here we go! 🚀

blkt · 2024-06-04T11:51:07Z

internal/controlplane/handlers_githubwebhooks.go

+
+	installationID := event.GetInstallation().GetID()
+	installation, err := s.store.GetInstallationIDByAppID(ctx, installationID)
+	if errors.Is(err, sql.ErrNoRows) {


I'm not following, what is an unclaimed installation?

blkt · 2024-06-04T11:52:17Z

internal/controlplane/handlers_githubwebhooks.go

+			ctx,
+			repo,
+			event.GetAction(),
+			events.TopicQueueReconcileEntityAdd,


You're right, this can only be a events.TopicQueueReconcileEntityAdd.
Initially the routine was just one, this is a leftover.

blkt · 2024-06-04T11:53:53Z

internal/controlplane/handlers_githubwebhooks.go

+	// We might want to ignore this case since we can only delete
+	// repositories there were previously registered, which would
+	// be deleted by means of "meta" and "repository" events as
+	// well.


I have to test what happens if we revoke access to a repository without deleting it from GitHub.

I believe that we don't get the "meta" event in such a case, i.e. we have to handle "removed" action for "installation_repositories" events as well.

I'll test it manually and reply here.

blkt · 2024-06-04T11:55:10Z

internal/controlplane/handlers_repositories_test.go

@@ -37,6 +37,7 @@ import (
 	mockmanager "github.com/stacklok/minder/internal/providers/manager/mock"
 	ghrepo "github.com/stacklok/minder/internal/repositories/github"
 	mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock"
+	rf "github.com/stacklok/minder/internal/repositories/github/mock/fixtures"


The diff for this file should just be refactoring, I moved helper routines to this package.

blkt · 2024-06-04T11:57:16Z

internal/controlplane/handlers_githubwebhooks_test.go

 			topic:         installations.ProviderInstallationTopic,
 			statusCode:    http.StatusOK,
 			queued:        nil,
 		},

+		// installation repositories events


Most of the diff for this file is refactoring, relevant tests are here.

blkt · 2024-06-04T12:23:29Z

@jhrozek I'm afraid that handling deletion is going to be trickier than we anticipated.
We might need to have a specific code path for deleting repositories that are not accessible anymore.

See this issue #3518

RepoEvent struct was added to avoid polluting EntityInfoWrapper further.

jhrozek

This is great. I kicked the coverage tests because they seem to be failing while the "normal" unit test run succeeded, so it might be a fluke.

blkt self-assigned this May 28, 2024

blkt force-pushed the webhook-refactor branch from a26fafd to 32e8ac3 Compare May 29, 2024 08:22

blkt force-pushed the issue-3267 branch from d8110ae to ac11873 Compare May 29, 2024 08:23

blkt force-pushed the webhook-refactor branch from 32e8ac3 to ef12c05 Compare May 29, 2024 09:10

blkt force-pushed the issue-3267 branch from ac11873 to 7347245 Compare May 29, 2024 09:12

blkt force-pushed the webhook-refactor branch 2 times, most recently from 3c503bc to a01f93c Compare May 29, 2024 14:43

blkt force-pushed the issue-3267 branch from 7347245 to 6da9fd3 Compare May 29, 2024 14:44

Base automatically changed from webhook-refactor to main May 30, 2024 07:14

blkt force-pushed the issue-3267 branch from 6da9fd3 to af26f42 Compare May 30, 2024 08:04

blkt force-pushed the issue-3267 branch from af26f42 to 91894dd Compare May 30, 2024 15:35

jhrozek reviewed May 30, 2024

View reviewed changes

internal/reconcilers/entity_add.go Outdated Show resolved Hide resolved

blkt force-pushed the issue-3267 branch 3 times, most recently from 931d80c to 979e5c7 Compare May 31, 2024 15:15

blkt commented May 31, 2024

View reviewed changes

jhrozek reviewed Jun 2, 2024

View reviewed changes

jhrozek mentioned this pull request Jun 3, 2024

Only auto-register repositories if the provider configuration enables that #3512

Closed

Webhook handler now processes installation_repositories events.

1e1a2d0

Such events are triggered when a new repository is added to or removed from the list of repositories accessible to the app.

blkt force-pushed the issue-3267 branch from 979e5c7 to 89abd76 Compare June 4, 2024 11:46

blkt marked this pull request as ready for review June 4, 2024 11:49

blkt requested a review from a team as a code owner June 4, 2024 11:49

Reconciler's handlers are now fully tested.

1322bc3

blkt force-pushed the issue-3267 branch from 89abd76 to 1322bc3 Compare June 4, 2024 11:58

blkt commented Jun 4, 2024

View reviewed changes

Replaced EntityInfoWrapper with RepoEvent.

432ec11

RepoEvent struct was added to avoid polluting EntityInfoWrapper further.

blkt force-pushed the issue-3267 branch from 8d4a92b to 432ec11 Compare June 4, 2024 22:04

jhrozek approved these changes Jun 5, 2024

View reviewed changes

blkt merged commit 936892a into main Jun 5, 2024
20 checks passed

blkt deleted the issue-3267 branch June 5, 2024 08:49

evankanderson mentioned this pull request Aug 27, 2024

CLI minder repo delete fails when access to github repository is revoked #3518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhook handler now processes installation_repositories events #3447

Webhook handler now processes installation_repositories events #3447

blkt commented May 28, 2024

coveralls commented May 30, 2024 •

edited

Loading

blkt May 31, 2024

jhrozek Jun 2, 2024

blkt Jun 4, 2024

blkt May 31, 2024

jhrozek left a comment

jhrozek Jun 2, 2024

blkt Jun 4, 2024

jhrozek Jun 4, 2024

jhrozek Jun 4, 2024

jhrozek Jun 2, 2024

jhrozek Jun 2, 2024

blkt Jun 4, 2024

jhrozek Jun 2, 2024

blkt left a comment

blkt Jun 4, 2024

blkt Jun 4, 2024

blkt Jun 4, 2024

blkt Jun 4, 2024

blkt Jun 4, 2024

blkt commented Jun 4, 2024

jhrozek left a comment

Webhook handler now processes installation_repositories events #3447

Webhook handler now processes installation_repositories events #3447

Conversation

blkt commented May 28, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented May 30, 2024 • edited Loading

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

jhrozek 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

Choose a reason for hiding this comment

blkt 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

blkt commented Jun 4, 2024

jhrozek left a comment

Choose a reason for hiding this comment

coveralls commented May 30, 2024 •

edited

Loading