From 37a31ddafb9115cc74199a96baa74cb20cc5bee5 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 16 Sep 2024 22:02:06 +0200 Subject: [PATCH 1/2] Remove unused setters of repoID attribute for entity reconciliation --- internal/controlplane/handlers_githubwebhooks.go | 6 ++---- internal/controlplane/handlers_githubwebhooks_test.go | 8 -------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go index 1b0241c466..7437d94b20 100644 --- a/internal/controlplane/handlers_githubwebhooks.go +++ b/internal/controlplane/handlers_githubwebhooks.go @@ -885,8 +885,7 @@ func (s *Server) processRelevantRepositoryEvent( WithProjectID(repoEntity.Entity.ProjectID). WithProviderID(repoEntity.Entity.ProviderID). WithEntityType("repository"). - WithEntityID(repoEntity.Entity.ID). - WithAttribute("repoID", repoEntity.Entity.ID.String()) + WithEntityID(repoEntity.Entity.ID) return &processingResult{ topic: events.TopicQueueReconcileEntityDelete, @@ -1221,8 +1220,7 @@ func (s *Server) repositoryRemoved( WithProjectID(repoEnt.Entity.ProjectID). WithProviderID(repoEnt.Entity.ProviderID). WithEntityType("repository"). - WithEntityID(repoEnt.Entity.ID). - WithAttribute("repoID", repoEnt.Entity.ID.String()) + WithEntityID(repoEnt.Entity.ID) return &processingResult{ topic: events.TopicQueueReconcileEntityDelete, diff --git a/internal/controlplane/handlers_githubwebhooks_test.go b/internal/controlplane/handlers_githubwebhooks_test.go index 46594a3081..2afdf0619b 100644 --- a/internal/controlplane/handlers_githubwebhooks_test.go +++ b/internal/controlplane/handlers_githubwebhooks_test.go @@ -418,7 +418,6 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() { require.NoError(t, validator.New().Struct(&inner)) require.Equal(t, providerID, inner.ProviderID) require.Equal(t, projectID, inner.ProjectID) - require.Equal(t, repositoryID.String(), inner.Entity["repoID"]) require.Equal(t, nil, inner.Entity["repoName"]) // optional require.Equal(t, nil, inner.Entity["repoOwner"]) // optional @@ -1236,7 +1235,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.NoError(t, validator.New().Struct(&evt)) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) require.Equal(t, nil, evt.Entity["repoName"]) // optional require.Equal(t, nil, evt.Entity["repoOwner"]) // optional }, @@ -1288,7 +1286,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.NoError(t, validator.New().Struct(&evt)) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) require.Equal(t, nil, evt.Entity["repoName"]) // optional require.Equal(t, nil, evt.Entity["repoOwner"]) // optional }, @@ -1837,7 +1834,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.NoError(t, validator.New().Struct(&evt)) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) require.Equal(t, nil, evt.Entity["repoName"]) // optional require.Equal(t, nil, evt.Entity["repoOwner"]) // optional }, @@ -1888,7 +1884,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.NoError(t, validator.New().Struct(&evt)) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) require.Equal(t, nil, evt.Entity["repoName"]) // optional require.Equal(t, nil, evt.Entity["repoOwner"]) // optional }, @@ -2143,7 +2138,6 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() { require.NoError(t, validator.New().Struct(&evt)) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) require.Equal(t, nil, evt.Entity["repoName"]) // optional require.Equal(t, nil, evt.Entity["repoOwner"]) // optional }, @@ -4675,7 +4669,6 @@ func (s *UnitTestSuite) TestHandleGitHubAppWebHook() { require.NoError(t, err) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) received = withTimeout(ch, timeout) require.NotNilf(t, received, "no event received after waiting %s", timeout) @@ -4687,7 +4680,6 @@ func (s *UnitTestSuite) TestHandleGitHubAppWebHook() { require.NoError(t, err) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) - require.Equal(t, repositoryID.String(), evt.Entity["repoID"]) }, }, From 0c7e29869441e5b1ccd116c9b1b1bb95a53aafca Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 16 Sep 2024 22:02:24 +0200 Subject: [PATCH 2/2] Use generic property keys instead of repoName/repoOwner when adding repos --- internal/controlplane/handlers_githubwebhooks.go | 3 +-- .../controlplane/handlers_githubwebhooks_test.go | 2 ++ internal/reconcilers/entity_add.go | 16 +++++----------- internal/reconcilers/entity_add_test.go | 13 +++++-------- 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go index 7437d94b20..7f15fa19fc 100644 --- a/internal/controlplane/handlers_githubwebhooks.go +++ b/internal/controlplane/handlers_githubwebhooks.go @@ -1241,8 +1241,7 @@ func (_ *Server) repositoryAdded( WithProjectID(installation.ProjectID.UUID). WithProviderID(installation.ProviderID.UUID). WithEntityType("repository"). - WithAttribute("repoName", repo.GetName()). - WithAttribute("repoOwner", repo.GetOwner()) + WithAttribute(properties.PropertyName, repo.GetFullName()) return &processingResult{ topic: events.TopicQueueReconcileEntityAdd, diff --git a/internal/controlplane/handlers_githubwebhooks_test.go b/internal/controlplane/handlers_githubwebhooks_test.go index 2afdf0619b..6a31afb8c2 100644 --- a/internal/controlplane/handlers_githubwebhooks_test.go +++ b/internal/controlplane/handlers_githubwebhooks_test.go @@ -4517,6 +4517,7 @@ func (s *UnitTestSuite) TestHandleGitHubAppWebHook() { require.NoError(t, err) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) + require.Equal(t, "stacklok/minder", evt.Entity[properties.PropertyName]) received = withTimeout(ch, timeout) require.NotNilf(t, received, "no event received after waiting %s", timeout) @@ -4528,6 +4529,7 @@ func (s *UnitTestSuite) TestHandleGitHubAppWebHook() { require.NoError(t, err) require.Equal(t, providerID, evt.ProviderID) require.Equal(t, projectID, evt.ProjectID) + require.Equal(t, "stacklok/trusty", evt.Entity[properties.PropertyName]) }, }, { diff --git a/internal/reconcilers/entity_add.go b/internal/reconcilers/entity_add.go index c98c266c48..f2c6565b6c 100644 --- a/internal/reconcilers/entity_add.go +++ b/internal/reconcilers/entity_add.go @@ -16,7 +16,6 @@ package reconcilers import ( "encoding/json" - "errors" "fmt" "github.com/ThreeDotsLabs/watermill/message" @@ -52,21 +51,17 @@ func (r *Reconciler) handleEntityAddEvent(msg *message.Message) error { return nil } - var repoOwner string - var repoName string + var entName string var ok bool - if repoOwner, ok = event.Entity["repoOwner"].(string); !ok { - return errors.New("invalid repo owner") - } - if repoName, ok = event.Entity["repoName"].(string); !ok { - return errors.New("invalid repo name") + if entName, ok = event.Entity[properties.PropertyName].(string); !ok { + return fmt.Errorf("invalid or unset entity name: %s", entName) } // TODO: This should be using the properties map coming from the event // as-is, but for now we are using the repoOwner and repoName to create // the properties map. fetchByProps, err := properties.NewProperties(map[string]interface{}{ - properties.PropertyName: fmt.Sprintf("%s/%s", repoOwner, repoName), + properties.PropertyName: entName, }) if err != nil { return fmt.Errorf("error creating properties: %w", err) @@ -75,8 +70,7 @@ func (r *Reconciler) handleEntityAddEvent(msg *message.Message) error { l = zerolog.Ctx(ctx).With(). Str("provider_id", event.ProviderID.String()). Str("project_id", event.ProjectID.String()). - Str("repo_name", repoName). - Str("repo_owner", repoOwner). + Str("ent_name", entName). Logger() // Telemetry logging diff --git a/internal/reconcilers/entity_add_test.go b/internal/reconcilers/entity_add_test.go index 4082b192e2..dc05583c51 100644 --- a/internal/reconcilers/entity_add_test.go +++ b/internal/reconcilers/entity_add_test.go @@ -25,14 +25,14 @@ import ( df "github.com/stacklok/minder/database/mock/fixtures" db "github.com/stacklok/minder/internal/db" + "github.com/stacklok/minder/internal/entities/properties" "github.com/stacklok/minder/internal/reconcilers/messages" rf "github.com/stacklok/minder/internal/repositories/mock/fixtures" pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) var ( - repoOwner = "stacklok" - repoName = "minder" + repoFullName = "stacklok/minder" ) func TestHandleEntityAdd(t *testing.T) { @@ -64,8 +64,7 @@ func TestHandleEntityAdd(t *testing.T) { WithProviderID(providerID). WithProjectID(projectID). WithEntityType("repository"). - WithAttribute("repoName", repoName). - WithAttribute("repoOwner", repoOwner). + WithAttribute(properties.PropertyName, repoFullName). ToMessage(m) require.NoError(t, err, "invalid message") return m @@ -86,8 +85,7 @@ func TestHandleEntityAdd(t *testing.T) { WithProviderID(providerID). WithProjectID(projectID). WithEntityType("repository"). - WithAttribute("repoName", repoName). - WithAttribute("repoOwner", repoOwner). + WithAttribute(properties.PropertyName, repoFullName). ToMessage(m) require.NoError(t, err, "invalid message") return m @@ -119,8 +117,7 @@ func TestHandleEntityAdd(t *testing.T) { WithProviderID(providerID). WithProjectID(projectID). WithEntityType("repository"). - WithAttribute("repoName", repoName). - WithAttribute("repoOwner", repoOwner). + WithAttribute(properties.PropertyName, repoFullName). ToMessage(m) require.NoError(t, err, "invalid message") return m