diff --git a/pkg/matcher/annotation_matcher_test.go b/pkg/matcher/annotation_matcher_test.go index e405d6d3fb..2df88cb5d4 100644 --- a/pkg/matcher/annotation_matcher_test.go +++ b/pkg/matcher/annotation_matcher_test.go @@ -1623,9 +1623,8 @@ func TestMatchPipelinerunAnnotationAndRepositories(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) fakeclient, mux, ghTestServerURL, teardown := ghtesthelper.SetupGH() defer teardown() - vcx := &ghprovider.Provider{ - Token: github.Ptr("None"), - } + vcx := ghprovider.New() + vcx.Token = github.Ptr("None") vcx.SetGithubClient(fakeclient) if tt.args.runevent.Request == nil { tt.args.runevent.Request = &info.Request{Header: http.Header{}, Payload: nil} @@ -2338,7 +2337,7 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) { eventEmitter := events.NewEventEmitter(cs.Clients.Kube, logger) repo := tt.repo - matches, err := MatchPipelinerunByAnnotation(ctx, logger, tt.args.pruns, cs, &tt.args.runevent, &ghprovider.Provider{}, eventEmitter, repo, true) + matches, err := MatchPipelinerunByAnnotation(ctx, logger, tt.args.pruns, cs, &tt.args.runevent, ghprovider.New(), eventEmitter, repo, true) if tt.wantErrNoFailedPipelineToRetest { assert.Assert(t, err != nil, "expected ErrNoFailedPipelineToRetest") assert.Assert(t, errors.Is(err, NoFailedPipelineToRetestError("/pac ")), "expected ErrNoFailedPipelineToRetest, got: %v", err) @@ -3284,7 +3283,7 @@ func TestFilterSuccessfulTemplates(t *testing.T) { return } - filtered := filterSuccessfulTemplates(ctx, logger, cs, event, repo, &ghprovider.Provider{}, tt.matchedPRs) + filtered := filterSuccessfulTemplates(ctx, logger, cs, event, repo, ghprovider.New(), tt.matchedPRs) // Check that the correct number of templates remain assert.Equal(t, len(tt.expectedNames), len(filtered), diff --git a/pkg/pipelineascode/match_test.go b/pkg/pipelineascode/match_test.go index 18f4333a8a..194aa735eb 100644 --- a/pkg/pipelineascode/match_test.go +++ b/pkg/pipelineascode/match_test.go @@ -557,10 +557,9 @@ func TestGetPipelineRunsFromRepo(t *testing.T) { k8int := &kitesthelper.KinterfaceTest{ ConsoleURL: "https://console.url", } - vcx := &ghprovider.Provider{ - Token: github.Ptr("None"), - Logger: logger, - } + vcx := ghprovider.New() + vcx.Token = github.Ptr("None") + vcx.Logger = logger vcx.SetGithubClient(fakeclient) pacInfo := &info.PacOpts{ Settings: settings.Settings{ @@ -789,7 +788,9 @@ func TestVerifyRepoAndUser(t *testing.T) { func(rw http.ResponseWriter, _ *http.Request) { fmt.Fprint(rw, `{}`) }, ) - vcx := &ghprovider.Provider{Token: github.Ptr("token"), Logger: logger} + vcx := ghprovider.New() + vcx.Token = github.Ptr("token") + vcx.Logger = logger vcx.SetGithubClient(ghClient) vcx.SetPacInfo(pacInfo) diff --git a/pkg/pipelineascode/pipelineascode_startpr_test.go b/pkg/pipelineascode/pipelineascode_startpr_test.go index 9748ea2e07..822a8e4089 100644 --- a/pkg/pipelineascode/pipelineascode_startpr_test.go +++ b/pkg/pipelineascode/pipelineascode_startpr_test.go @@ -207,11 +207,10 @@ func setupStartPRTest(t *testing.T) (*params.Run, *info.Event, *zap.SugaredLogge // setupProviderForTest creates and configures a GitHub provider for testing. func setupProviderForTest(cs *params.Run, logger *zap.SugaredLogger, fakeclient *github.Client, pacInfo *info.PacOpts) *ghprovider.Provider { - vcx := &ghprovider.Provider{ - Run: cs, - Token: github.Ptr("test-token"), - Logger: logger, - } + vcx := ghprovider.New() + vcx.Run = cs + vcx.Token = github.Ptr("test-token") + vcx.Logger = logger vcx.SetGithubClient(fakeclient) vcx.SetPacInfo(pacInfo) return vcx diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 6397af7790..a19342ca61 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -662,11 +662,10 @@ func TestRun(t *testing.T) { HubCatalogs: &hubCatalogs, }, } - vcx := &ghprovider.Provider{ - Run: cs, - Token: github.Ptr("None"), - Logger: logger, - } + vcx := ghprovider.New() + vcx.Run = cs + vcx.Token = github.Ptr("None") + vcx.Logger = logger vcx.SetGithubClient(fakeclient) vcx.SetPacInfo(pacInfo) p := NewPacs(&tt.runevent, vcx, cs, pacInfo, k8int, logger, nil) diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index 8580e65e11..8df2ebadcd 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -72,6 +72,7 @@ type Provider struct { pacUserLogin string // user/bot login used by PAC clock clockwork.Clock graphQLClient *graphQLClient + checkRunsCache checkRunsCache } type skippedRun struct { @@ -79,6 +80,17 @@ type skippedRun struct { checkRunID int64 } +type checkRunsCache struct { + mu sync.Mutex + entries map[string]*checkRunsCacheEntry +} + +type checkRunsCacheEntry struct { + runs []*github.CheckRun + loading bool + done chan struct{} +} + func New() *Provider { return &Provider{ APIURL: github.Ptr(keys.PublicGithubAPIURL), @@ -87,6 +99,9 @@ func New() *Provider { mutex: &sync.Mutex{}, }, clock: clockwork.NewRealClock(), + checkRunsCache: checkRunsCache{ + entries: map[string]*checkRunsCacheEntry{}, + }, } } diff --git a/pkg/provider/github/status.go b/pkg/provider/github/status.go index b8a5051614..0980c6fced 100644 --- a/pkg/provider/github/status.go +++ b/pkg/provider/github/status.go @@ -23,8 +23,10 @@ import ( ) const ( - botType = "Bot" - pendingApproval = "Pending approval, waiting for an /ok-to-test" + botType = "Bot" + pendingApproval = "Pending approval, waiting for an /ok-to-test" + checkRunsFetchMaxRetries = 2 + checkRunsFetchInitialBackoff = 200 * time.Millisecond ) const taskStatusTemplate = ` @@ -42,7 +44,9 @@ const taskStatusTemplate = ` {{- end }} ` -func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Event, status providerstatus.StatusOpts) (*int64, error) { +// fetchAllCheckRunPages retrieves every page of check runs for the event SHA. +func (v *Provider) fetchAllCheckRunPages(ctx context.Context, runevent *info.Event) ([]*github.CheckRun, error) { + var all []*github.CheckRun opt := github.ListOptions{PerPage: v.PaginedNumber} for { res, resp, err := wrapAPI(v, "list_check_runs_for_ref", func() (*github.ListCheckRunsResults, *github.Response, error) { @@ -55,25 +59,114 @@ func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Eve if err != nil { return nil, err } + all = append(all, res.CheckRuns...) + if resp.NextPage == 0 { + break + } + opt.Page = resp.NextPage + } + return all, nil +} - for _, checkrun := range res.CheckRuns { - // if it is a Pending approval CheckRun then overwrite it - if isPendingApprovalCheckrun(checkrun) || isFailedCheckrun(checkrun) { - if v.canIUseCheckrunID(checkrun.ID) { - return checkrun.ID, nil - } +func (v *Provider) searchCheckRuns(runs []*github.CheckRun, status providerstatus.StatusOpts) *int64 { + for _, checkrun := range runs { + if isPendingApprovalCheckrun(checkrun) || isFailedCheckrun(checkrun) { + if v.canIUseCheckrunID(checkrun.ID) { + return checkrun.ID } - if *checkrun.ExternalID == status.PipelineRunName { - return checkrun.ID, nil + } + if checkrun.ExternalID != nil && *checkrun.ExternalID == status.PipelineRunName { + return checkrun.ID + } + } + return nil +} + +func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Event, status providerstatus.StatusOpts) (*int64, error) { + appID := "none" + if v.ApplicationID != nil { + appID = strconv.FormatInt(*v.ApplicationID, 10) + } + cacheKey := fmt.Sprintf("%s/%s/%s/%s", runevent.Organization, runevent.Repository, appID, runevent.SHA) + + // Loop handles the wait from channel and fetch of the check runs from the API + for { + // Check if the check runs are already cached + v.checkRunsCache.mu.Lock() + entry, ok := v.checkRunsCache.entries[cacheKey] + if ok { + // Wait for fetch to complete if the check runs are still loading. + if entry.loading { + done := entry.done + v.checkRunsCache.mu.Unlock() + <-done + continue } + // Return the check run ID if the check runs are loaded. + runs := entry.runs + v.checkRunsCache.mu.Unlock() + return v.searchCheckRuns(runs, status), nil } - if resp.NextPage == 0 { - break + + // Create a new "loading" entry if the check runs are not cached. + entry = &checkRunsCacheEntry{ + loading: true, + done: make(chan struct{}), } - opt.Page = resp.NextPage + v.checkRunsCache.entries[cacheKey] = entry + v.checkRunsCache.mu.Unlock() + + // Fetch check runs from the API. + runs, err := v.fetchAllCheckRunPagesWithRetry(ctx, runevent) + + v.checkRunsCache.mu.Lock() + // Delete the entry if fetch failed. + if err != nil { + delete(v.checkRunsCache.entries, cacheKey) + } else { + // Update the entry if fetch succeeded. + entry.runs = runs + entry.loading = false + } + close(entry.done) + v.checkRunsCache.mu.Unlock() + + // Return the error if fetch failed. + if err != nil { + return nil, err + } + // Return the check run ID if fetch succeeded. + return v.searchCheckRuns(runs, status), nil } +} + +func (v *Provider) fetchAllCheckRunPagesWithRetry(ctx context.Context, runevent *info.Event) ([]*github.CheckRun, error) { + var lastErr error + for attempt := range checkRunsFetchMaxRetries + 1 { + runs, err := v.fetchAllCheckRunPages(ctx, runevent) + if err == nil { + return runs, nil + } + lastErr = err + + if attempt == checkRunsFetchMaxRetries { + return nil, err + } + + backoff := time.Duration(1<