Reuses skipped checkrun, as we can't remove it

skipped runs were not getting removed a authorised user adds /ok-to-test
on a pull req from a new user, this is to fix that case.

we cannot delete a checkrun so we reuse it for the pipelinerun, when the
CI is triggered later.
as we execute pipelineruns parallely, to make sure only one pipelinerun uses that
checkrunid we use mutex to store that checkrunid.

Author: Shivam Mukhade

pkg/adapter/adapter.go

@@ -212,7 +212,7 @@ func (l listener) detectProvider(req *http.Request, reqBody string) (provider.In
 		return nil, &log, fmt.Errorf("invalid event body format: %w", err)
 	}
 
-	gitHub := &github.Provider{}
+	gitHub := github.New()
 	isGH, processReq, logger, reason, err := gitHub.Detect(req, reqBody, &log)
 	if isGH {
 		return l.processRes(processReq, gitHub, logger, reason, err)

pkg/adapter/incoming.go

@@ -98,7 +98,7 @@ func (l *listener) processIncoming(targetRepo *v1alpha1.Repository) (provider.In
 	var provider provider.Interface
 	switch targetRepo.Spec.GitProvider.Type {
 	case "github":
-		provider = &github.Provider{}
+		provider = github.New()
 	case "gitlab":
 		provider = &gitlab.Provider{}
 	case "gitea":

pkg/adapter/incoming_test.go

@@ -429,7 +429,7 @@ func Test_listener_processIncoming(t *testing.T) {
 	}{
 		{
 			name:     "process/github",
-			want:     &github.Provider{},
+			want:     github.New(),
 			wantOrg:  "owner",
 			wantRepo: "repo",
 			targetRepo: &v1alpha1.Repository{

pkg/cmd/tknpac/resolve/resolve.go

@@ -148,7 +148,7 @@ func resolveFilenames(cs *params.Run, filenames []string, params map[string]stri
 		SkipInlining: skipInlining,
 	}
 	// We use github here but since we don't do remotetask we would not care
-	providerintf := &github.Provider{}
+	providerintf := github.New()
 	event := info.NewEvent()
 	prun, err := resolve.Resolve(ctx, cs, cs.Clients.Log, providerintf, event, allTemplates, ropt)
 	if err != nil {

pkg/provider/github/github.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
+	"sync"
 
 	"github.com/google/go-github/v47/github"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
@@ -29,12 +30,25 @@ const (
 )
 
 type Provider struct {
-	Client                *github.Client
-	Logger                *zap.SugaredLogger
-	Token, APIURL         *string
-	ApplicationID         *int64
-	providerName          string
-	AutoConfigureNewRepos bool
+	Client        *github.Client
+	Logger        *zap.SugaredLogger
+	Token, APIURL *string
+	ApplicationID *int64
+	providerName  string
+	skippedRun
+}
+
+type skippedRun struct {
+	mutex      *sync.Mutex
+	checkRunID int64
+}
+
+func New() *Provider {
+	return &Provider{
+		skippedRun: skippedRun{
+			mutex: &sync.Mutex{},
+		},
+	}
 }
 
 // splitGithubURL Take a Github url and split it with org/repo path ref, supports rawURL

pkg/provider/github/status.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"time"
 
 	"github.com/google/go-github/v47/github"
@@ -58,6 +59,12 @@ func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Eve
 	}
 
 	for _, checkrun := range res.CheckRuns {
+		// if it is a skipped checkrun then overwrite it
+		if isSkippedCheckrun(checkrun) {
+			if v.canIUseCheckrunID(checkrun.ID) {
+				return checkrun.ID, nil
+			}
+		}
 		if *checkrun.ExternalID == status.PipelineRunName {
 			return checkrun.ID, nil
 		}
@@ -66,6 +73,29 @@ func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Eve
 	return nil, nil
 }
 
+func isSkippedCheckrun(run *github.CheckRun) bool {
+	if run == nil || run.Output == nil {
+		return false
+	}
+	if run.Output.Title != nil && *run.Output.Title == "Skipped" &&
+		run.Output.Summary != nil &&
+		strings.Contains(*run.Output.Summary, "is skipping this commit") {
+		return true
+	}
+	return false
+}
+
+func (v *Provider) canIUseCheckrunID(checkrunid *int64) bool {
+	v.skippedRun.mutex.Lock()
+	defer v.skippedRun.mutex.Unlock()
+
+	if v.skippedRun.checkRunID == 0 {
+		v.skippedRun.checkRunID = *checkrunid
+		return true
+	}
+	return false
+}
+
 func (v *Provider) createCheckRunStatus(ctx context.Context, runevent *info.Event, pacopts *info.PacOpts, status provider.StatusOpts) (*int64, error) {
 	now := github.Timestamp{Time: time.Now()}
 	checkrunoption := github.CreateCheckRunOptions{
@@ -103,7 +133,6 @@ func (v *Provider) getOrUpdateCheckRunStatus(ctx context.Context, tekton version
 			checkRunID = github.Int64(int64(checkID))
 		}
 	}
-
 	if !found {
 		if checkRunID, _ = v.getExistingCheckRunID(ctx, runevent, status); checkRunID == nil {
 			checkRunID, err = v.createCheckRunStatus(ctx, runevent, pacopts, status)
@@ -121,12 +150,17 @@ func (v *Provider) getOrUpdateCheckRunStatus(ctx context.Context, tekton version
 		Summary: &status.Summary,
 		Text:    &status.Text,
 	}
-
 	opts := github.UpdateCheckRunOptions{
 		Name:   getCheckName(status, pacopts),
-		Status: &status.Status,
+		Status: github.String(status.Status),
 		Output: checkRunOutput,
 	}
+	if status.PipelineRunName != "" {
+		opts.ExternalID = github.String(status.PipelineRunName)
+	}
+	if pacopts.LogURL != "" {
+		opts.DetailsURL = github.String(pacopts.LogURL)
+	}
 
 	if status.DetailsURL != "" {
 		opts.DetailsURL = &status.DetailsURL

pkg/provider/github/status_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
 	ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"gotest.tools/v3/assert"
@@ -90,6 +91,45 @@ func TestGetExistingCheckRunIDFromMultiple(t *testing.T) {
 	assert.Equal(t, *id, chosenID)
 }
 
+func TestGetExistingSkippedCheckRunID(t *testing.T) {
+	ctx, _ := rtesting.SetupFakeContext(t)
+	client, mux, _, teardown := ghtesthelper.SetupGH()
+	defer teardown()
+
+	cnx := New()
+	cnx.Client = client
+
+	event := &info.Event{
+		Organization: "owner",
+		Repository:   "repository",
+		SHA:          "sha",
+	}
+
+	chosenOne := "chosenOne"
+	chosenID := int64(55555)
+	mux.HandleFunc(fmt.Sprintf("/repos/%v/%v/commits/%v/check-runs", event.Organization, event.Repository, event.SHA), func(w http.ResponseWriter, r *http.Request) {
+		_, _ = fmt.Fprintf(w, `{
+			"total_count": 1,
+			"check_runs": [
+				{
+					"id": %v,
+					"external_id": "%s",
+					"output": {
+						"title": "Skipped",
+						"summary": "My CI is skipping this commit"
+					}
+				}
+			]
+		}`, chosenID, chosenOne)
+	})
+
+	id, err := cnx.getExistingCheckRunID(ctx, event, provider.StatusOpts{
+		PipelineRunName: chosenOne,
+	})
+	assert.NilError(t, err)
+	assert.Equal(t, *id, chosenID)
+}
+
 func TestGithubProviderCreateStatus(t *testing.T) {
 	checkrunid := int64(2026)
 	resultid := int64(666)
@@ -117,11 +157,13 @@ func TestGithubProviderCreateStatus(t *testing.T) {
 		githubApps         bool
 	}
 	tests := []struct {
-		name    string
-		args    args
-		want    *github.CheckRun
-		wantErr bool
-		notoken bool
+		name                 string
+		args                 args
+		pr                   *v1beta1.PipelineRun
+		want                 *github.CheckRun
+		wantErr              bool
+		notoken              bool
+		addExistingCheckruns bool
 	}{
 		{
 			name: "success",
@@ -137,6 +179,26 @@ func TestGithubProviderCreateStatus(t *testing.T) {
 			want:    &github.CheckRun{ID: &resultid},
 			wantErr: false,
 		},
+		{
+			name: "success with using existing skipped run checkrun",
+			args: args{
+				runevent:    runEvent,
+				status:      "completed",
+				conclusion:  "success",
+				text:        "Yay",
+				detailsURL:  "https://cireport.com",
+				titleSubstr: "Success",
+				githubApps:  true,
+			},
+			pr: &v1beta1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: prname,
+				},
+			},
+			addExistingCheckruns: true,
+			want:                 &github.CheckRun{ID: &resultid},
+			wantErr:              false,
+		},
 		{
 			name: "success coming from webhook",
 			args: args{
@@ -219,9 +281,10 @@ func TestGithubProviderCreateStatus(t *testing.T) {
 			defer teardown()
 
 			ctx, _ := rtesting.SetupFakeContext(t)
-			gcvs := Provider{
-				Client: fakeclient,
-			}
+			gcvs := New()
+			gcvs.Client = fakeclient
+			gcvs.Logger = getLogger()
+
 			mux.HandleFunc("/repos/check/run/statuses/sha", func(rw http.ResponseWriter, r *http.Request) {})
 			mux.HandleFunc(fmt.Sprintf("/repos/check/run/check-runs/%d", checkrunid), func(rw http.ResponseWriter, r *http.Request) {
 				bit, _ := io.ReadAll(r.Body)
@@ -242,6 +305,24 @@ func TestGithubProviderCreateStatus(t *testing.T) {
 				_, err = fmt.Fprintf(rw, `{"id": %d}`, resultid)
 				assert.NilError(t, err)
 			})
+			if tt.addExistingCheckruns {
+				tt.args.runevent.SHA = "sha"
+				mux.HandleFunc(fmt.Sprintf("/repos/%v/%v/commits/%v/check-runs", tt.args.runevent.Organization, tt.args.runevent.Repository, tt.args.runevent.SHA), func(w http.ResponseWriter, r *http.Request) {
+					_, _ = fmt.Fprintf(w, `{
+						"total_count": 1,
+						"check_runs": [
+							{
+								"id": %v,
+								"external_id": "%v",
+								"output": {
+									"title": "Skipped",
+									"summary": "My CI is skipping this commit"
+								}
+							}
+						]
+					}`, checkrunid, resultid)
+				})
+			}
 
 			status := provider.StatusOpts{
 				PipelineRunName: prname,
@@ -251,6 +332,9 @@ func TestGithubProviderCreateStatus(t *testing.T) {
 				Text:            tt.args.text,
 				DetailsURL:      tt.args.detailsURL,
 			}
+			if tt.pr != nil {
+				status.PipelineRun = tt.pr
+			}
 			pacopts := &info.PacOpts{
 				LogURL:   "https://log",
 				Settings: &settings.Settings{},
@@ -268,7 +352,15 @@ func TestGithubProviderCreateStatus(t *testing.T) {
 					tt.args.runevent.SHA = "sha"
 				}
 			}
-			err := gcvs.CreateStatus(ctx, nil, tt.args.runevent, pacopts, status)
+
+			testData := testclient.Data{}
+			if tt.pr != nil {
+				testData = testclient.Data{
+					PipelineRuns: []*v1beta1.PipelineRun{tt.pr},
+				}
+			}
+			stdata, _ := testclient.SeedTestData(t, ctx, testData)
+			err := gcvs.CreateStatus(ctx, stdata.Pipeline, tt.args.runevent, pacopts, status)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("GithubProvider.CreateStatus() error = %v, wantErr %v", err, tt.wantErr)
 				return

pkg/reconciler/event.go

@@ -30,7 +30,7 @@ func (r *Reconciler) detectProvider(ctx context.Context, logger *zap.SugaredLogg
 	var provider provider.Interface
 	switch gitProvider {
 	case "github", "github-enterprise":
-		gh := &github.Provider{}
+		gh := github.New()
 		if event.InstallationID != 0 {
 			if err := gh.InitAppClient(ctx, r.run.Clients.Kube, event); err != nil {
 				return nil, nil, err

test/pkg/github/pr.go

@@ -85,7 +85,7 @@ func PushFilesToRef(ctx context.Context, client *github.Client, commitMessage, b
 	return commit.GetSHA(), nil
 }
 
-func PRCreate(ctx context.Context, cs *params.Run, ghcnx ghprovider.Provider, owner, repo, targetRef, defaultBranch, title string) (int, error) {
+func PRCreate(ctx context.Context, cs *params.Run, ghcnx *ghprovider.Provider, owner, repo, targetRef, defaultBranch, title string) (int, error) {
 	pr, _, err := ghcnx.Client.PullRequests.Create(ctx, owner, repo, &github.NewPullRequest{
 		Title: &title,
 		Head:  &targetRef,
@@ -99,7 +99,7 @@ func PRCreate(ctx context.Context, cs *params.Run, ghcnx ghprovider.Provider, ow
 	return pr.GetNumber(), nil
 }
 
-func RunPullRequest(ctx context.Context, t *testing.T, label string, yamlFiles []string, webhook bool) (*params.Run, ghprovider.Provider, options.E2E, string, string, int, string) {
+func RunPullRequest(ctx context.Context, t *testing.T, label string, yamlFiles []string, webhook bool) (*params.Run, *ghprovider.Provider, options.E2E, string, string, int, string) {
 	targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
 	runcnx, opts, ghcnx, err := Setup(ctx, webhook)
 	assert.NilError(t, err)