Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ProviderManager in webhook handler #3202

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Use ProviderManager in webhook handler #3202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47a53e8
Use ProviderBuilder in webhook handler
dmjb Apr 29, 2024
5dc0c93
Merge branch 'main' into controlplane-provider-builder-cleanup
dmjb Apr 30, 2024
eaba89a
Merge branch 'main' into controlplane-provider-builder-cleanup
dmjb Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 22 additions & 41 deletions internal/controlplane/handlers_githubwebhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import (
"github.com/stacklok/minder/internal/engine/entities"
"github.com/stacklok/minder/internal/events"
"github.com/stacklok/minder/internal/projects/features"
"github.com/stacklok/minder/internal/providers"
"github.com/stacklok/minder/internal/providers/github/installations"
ghprov "github.com/stacklok/minder/internal/providers/github/service"
"github.com/stacklok/minder/internal/repositories"
Expand Down Expand Up @@ -501,22 +500,6 @@ func (s *Server) parseGithubEventForProcessing(
return fmt.Errorf("error getting repo information from payload: %w", err)
}

// get the provider for the repository
prov, err := s.providerStore.GetByID(ctx, dbRepo.ProviderID)
if err != nil {
return fmt.Errorf("error getting provider: %w", err)
}

pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
providers.WithRestClientCache(s.restClientCache),
}
provBuilder, err := providers.GetProviderBuilder(ctx, *prov, s.store, s.cryptoEngine, &s.cfg.Provider,
s.fallbackTokenClient, pbOpts...)
if err != nil {
return fmt.Errorf("error building client: %w", err)
}

var action string // explicit declaration to use the default value
action, err = util.JQReadFrom[string](ctx, ".action", payload)
if err != nil && !errors.Is(err, util.ErrNoValueFound) {
Expand All @@ -526,9 +509,9 @@ func (s *Server) parseGithubEventForProcessing(
// determine if the payload is an artifact published event
// TODO: this needs to be managed via signals
if ent == pb.Entity_ENTITY_ARTIFACTS && action == WebhookActionEventPublished {
return s.parseArtifactPublishedEvent(ctx, payload, msg, dbRepo, provBuilder, action)
return s.parseArtifactPublishedEvent(ctx, payload, msg, dbRepo, dbRepo.ProviderID, action)
} else if ent == pb.Entity_ENTITY_PULL_REQUESTS {
return parsePullRequestModEvent(ctx, payload, msg, dbRepo, s.store, provBuilder, action)
return s.parsePullRequestModEvent(ctx, payload, msg, dbRepo, dbRepo.ProviderID, action)
} else if ent == pb.Entity_ENTITY_REPOSITORIES {
return parseRepoEvent(payload, msg, dbRepo, action)
}
Expand Down Expand Up @@ -581,7 +564,7 @@ func (s *Server) parseArtifactPublishedEvent(
whPayload map[string]any,
msg *message.Message,
dbrepo db.Repository,
prov *providers.ProviderBuilder,
providerID uuid.UUID,
action string,
) error {
// we need to have information about package and repository
Expand All @@ -590,15 +573,15 @@ func (s *Server) parseArtifactPublishedEvent(
return nil
}

// NOTE(jaosorior): this webhook is very specific to github
if !prov.Implements(db.ProviderTypeGithub) {
log.Printf("provider %s is not supported for github webhook", prov.GetName())
return nil
provider, err := s.providerManager.InstantiateFromID(ctx, providerID)
if err != nil {
log.Printf("error instantiating provider: %v", err)
return err
}

cli, err := prov.GetGitHub()
cli, err := provifv1.As[provifv1.GitHub](provider)
if err != nil {
log.Printf("error creating github provider: %v", err)
log.Printf("error instantiating provider: %v", err)
return err
}

Expand Down Expand Up @@ -641,33 +624,32 @@ func (s *Server) parseArtifactPublishedEvent(
return eiw.ToMessage(msg)
}

func parsePullRequestModEvent(
func (s *Server) parsePullRequestModEvent(
ctx context.Context,
whPayload map[string]any,
msg *message.Message,
dbrepo db.Repository,
store db.Store,
prov *providers.ProviderBuilder,
providerID uuid.UUID,
action string,
) error {
// NOTE(jaosorior): this webhook is very specific to github
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is still true, we just think it's obvious, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - but this particular codepath is so Github-specific that it's not really worth noting any more.

if !prov.Implements(db.ProviderTypeGithub) {
log.Printf("provider %s is not supported for github webhook", prov.GetName())
return nil
provider, err := s.providerManager.InstantiateFromID(ctx, providerID)
if err != nil {
log.Printf("error instantiating provider: %v", err)
return err
}

cli, err := prov.GetGitHub()
cli, err := provifv1.As[provifv1.GitHub](provider)
if err != nil {
log.Printf("error creating github provider: %v", err)
return nil
log.Printf("error instantiating provider: %v", err)
return err
}

prEvalInfo, err := getPullRequestInfoFromPayload(ctx, whPayload)
if err != nil {
return fmt.Errorf("error getting pull request information from payload: %w", err)
}

dbPr, err := reconcilePrWithDb(ctx, store, dbrepo, prEvalInfo)
dbPr, err := s.reconcilePrWithDb(ctx, dbrepo, prEvalInfo)
if errors.Is(err, errNotHandled) {
return err
} else if err != nil {
Expand Down Expand Up @@ -864,9 +846,8 @@ func getPullRequestInfoFromPayload(
}, nil
}

func reconcilePrWithDb(
func (s *Server) reconcilePrWithDb(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed some functions to methods so we do not need to pass around the db.Store pointer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long-term, we want these to be in the provider implementation and not the server, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, yes.

ctx context.Context,
store db.Store,
dbrepo db.Repository,
prEvalInfo *pb.PullRequest,
) (*db.PullRequest, error) {
Expand All @@ -875,7 +856,7 @@ func reconcilePrWithDb(

switch prEvalInfo.Action {
case WebhookActionEventOpened, WebhookActionEventSynchronize:
dbPr, err := store.UpsertPullRequest(ctx, db.UpsertPullRequestParams{
dbPr, err := s.store.UpsertPullRequest(ctx, db.UpsertPullRequestParams{
RepositoryID: dbrepo.ID,
PrNumber: prEvalInfo.Number,
})
Expand All @@ -887,7 +868,7 @@ func reconcilePrWithDb(
retPr = &dbPr
retErr = nil
case WebhookActionEventClosed:
err := store.DeletePullRequest(ctx, db.DeletePullRequestParams{
err := s.store.DeletePullRequest(ctx, db.DeletePullRequestParams{
RepositoryID: dbrepo.ID,
PrNumber: prEvalInfo.Number,
})
Expand Down
32 changes: 0 additions & 32 deletions internal/controlplane/handlers_githubwebhooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"bytes"
"context"
"database/sql"
"encoding/base64"
"encoding/json"
"fmt"
"net/http"
Expand All @@ -35,10 +34,8 @@ import (
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"go.uber.org/mock/gomock"
"golang.org/x/oauth2"

mockdb "github.com/stacklok/minder/database/mock"
"github.com/stacklok/minder/internal/crypto"
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine/entities"
"github.com/stacklok/minder/internal/events"
Expand Down Expand Up @@ -246,27 +243,6 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
projectID := uuid.New()
providerID := uuid.New()

// create a test oauth2 token
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer needed because the token querying is not triggered unless the provider is actually instantiated. The test case for the handler does not need the provider to be instantiated.

token := oauth2.Token{
AccessToken: "test",
TokenType: "test",
}
// marshal the token
byteToken, err := json.Marshal(token)
require.NoError(t, err, "failed to marshal decryptedToken")
// encrypt the token
encryptedToken, err := crypto.EncryptBytes("test", byteToken)
require.NoError(t, err, "failed to encrypt token")

// Encode the token to Base64
encodedToken := base64.StdEncoding.EncodeToString(encryptedToken)
mockStore.EXPECT().GetAccessTokenByProjectID(gomock.Any(), gomock.Any()).Return(db.ProviderAccessToken{
EncryptedToken: encodedToken,
ID: 1,
ProjectID: projectID,
Provider: providerName,
}, nil)

mockStore.EXPECT().
GetRepositoryByRepoID(gomock.Any(), gomock.Any()).
Return(db.Repository{
Expand All @@ -277,14 +253,6 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
ProviderID: providerID,
}, nil)

mockStore.EXPECT().
GetProviderByID(gomock.Any(), gomock.Eq(providerID)).
Return(db.Provider{
ID: providerID,
ProjectID: projectID,
Name: providerName,
}, nil)

hook := srv.HandleGitHubWebHook()
port, err := rand.GetRandomPort()
if err != nil {
Expand Down