[gh app] Update exiting check run if there is one

When there was an already exiting check_run we discover it. It fixes the
issue when we :

- Create a commit
- There is a failure before we have checked out the pipelinerun match
  (ie: hub is down and fail to get the taskRef remotely)
- We do a /retest and the error is fixed.
- We would still have the status of the first failure as well a second
  successfull one.

We now list the check_runs attached to the ref created by our
ApplicationID and re-use that ID instead of creating a new one.

There is a slight bug in github interface, if we update the check run
and set as pending, it doesn't reflect it in the UI. It does change it
when the check_run went from failure to successfull so that's fine.

Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: chmouel

pkg/provider/github/events.go

@@ -49,6 +49,7 @@ func (v *Provider) getAppToken(ctx context.Context, info *info.PacOpts) error {
 	if err != nil {
 		return fmt.Errorf("could not parse installation_id: %w", err)
 	}
+	v.ApplicationID = &applicationID
 
 	// check if the path exists
 	if _, err := os.Stat(workspacePath); os.IsNotExist(err) {

pkg/provider/github/github.go

@@ -17,6 +17,7 @@ const apiPublicURL = "https://api.github.com/"
 type Provider struct {
 	Client        *github.Client
 	Token, APIURL *string
+	ApplicationID *int64
 }
 
 func (v *Provider) GetConfig() *info.ProviderConfig {

pkg/provider/github/status.go

@@ -35,25 +35,53 @@ func getCheckName(status provider.StatusOpts, pacopts *info.PacOpts) string {
 	return status.OriginalPipelineRunName
 }
 
-// createCheckRunStatus create a status via the checkRun API, which is only
+func (v *Provider) getExistingCheckRunID(ctx context.Context, runevent *info.Event) (*int64, error) {
+	res, _, err := v.Client.Checks.ListCheckRunsForRef(ctx, runevent.Organization, runevent.Repository,
+		runevent.SHA, &github.ListCheckRunsOptions{
+			AppID: v.ApplicationID,
+		})
+	if err != nil {
+		return nil, err
+	}
+	if *res.Total > 0 {
+		// Their should be only one, since we CRUD it.. maybe one day we
+		// will offer the ability to do multiple pipelineruns per commit then
+		// things will get more complicated here.
+		return res.CheckRuns[0].ID, nil
+	}
+	return nil, nil
+}
+
+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{
+		Name:       getCheckName(status, pacopts),
+		HeadSHA:    runevent.SHA,
+		Status:     github.String("in_progress"),
+		DetailsURL: github.String(pacopts.LogURL),
+		StartedAt:  &now,
+	}
+
+	checkRun, _, err := v.Client.Checks.CreateCheckRun(ctx, runevent.Organization, runevent.Repository, checkrunoption)
+	if err != nil {
+		return nil, err
+	}
+	return checkRun.ID, nil
+}
+
+// getOrUpdateCheckRunStatus create a status via the checkRun API, which is only
 // available with Github apps tokens.
-func (v *Provider) createCheckRunStatus(ctx context.Context, runevent *info.Event, pacopts *info.PacOpts, status provider.StatusOpts) error {
+func (v *Provider) getOrUpdateCheckRunStatus(ctx context.Context, runevent *info.Event, pacopts *info.PacOpts, status provider.StatusOpts) error {
+	var err error
+
 	now := github.Timestamp{Time: time.Now()}
 	if runevent.CheckRunID == nil {
-		now := github.Timestamp{Time: time.Now()}
-		checkrunoption := github.CreateCheckRunOptions{
-			Name:       getCheckName(status, pacopts),
-			HeadSHA:    runevent.SHA,
-			Status:     github.String("in_progress"),
-			DetailsURL: github.String(pacopts.LogURL),
-			StartedAt:  &now,
-		}
-
-		checkRun, _, err := v.Client.Checks.CreateCheckRun(ctx, runevent.Organization, runevent.Repository, checkrunoption)
-		if err != nil {
-			return err
+		if runevent.CheckRunID, _ = v.getExistingCheckRunID(ctx, runevent); runevent.CheckRunID == nil {
+			runevent.CheckRunID, err = v.createCheckRunStatus(ctx, runevent, pacopts, status)
+			if err != nil {
+				return err
+			}
 		}
-		runevent.CheckRunID = checkRun.ID
 	}
 
 	checkRunOutput := &github.CheckRunOutput{
@@ -78,7 +106,7 @@ func (v *Provider) createCheckRunStatus(ctx context.Context, runevent *info.Even
 		opts.Conclusion = &status.Conclusion
 	}
 
-	_, _, err := v.Client.Checks.UpdateCheckRun(ctx, runevent.Organization, runevent.Repository, *runevent.CheckRunID, opts)
+	_, _, err = v.Client.Checks.UpdateCheckRun(ctx, runevent.Organization, runevent.Repository, *runevent.CheckRunID, opts)
 	return err
 }
 
@@ -156,5 +184,5 @@ func (v *Provider) CreateStatus(ctx context.Context, runevent *info.Event, pacop
 	if pacopts.ProviderInfoFromRepo {
 		return v.createStatusCommit(ctx, runevent, pacopts, status)
 	}
-	return v.createCheckRunStatus(ctx, runevent, pacopts, status)
+	return v.getOrUpdateCheckRunStatus(ctx, runevent, pacopts, status)
 }

pkg/provider/github/status_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"reflect"
 	"strings"
 	"testing"
 
@@ -23,13 +24,45 @@ func TestGithubProviderCreateCheckRun(t *testing.T) {
 	event := &info.Event{
 		Organization: "check",
 		Repository:   "info",
+		SHA:          "createCheckRunSHA",
 	}
 
-	err := gcvs.createCheckRunStatus(ctx, event, &info.PacOpts{LogURL: "http://nowhere"}, provider.StatusOpts{Status: "hello moto"})
+	err := gcvs.getOrUpdateCheckRunStatus(ctx, event, &info.PacOpts{LogURL: "http://nowhere"}, provider.StatusOpts{Status: "hello moto"})
 	assert.NilError(t, err)
 	assert.Equal(t, *event.CheckRunID, int64(555))
 }
 
+func TestGetExistingCheckRunID(t *testing.T) {
+	ctx, _ := rtesting.SetupFakeContext(t)
+	client, mux, _, teardown := ghtesthelper.SetupGH()
+	defer teardown()
+
+	gvcs := &Provider{
+		Client: client,
+	}
+	event := &info.Event{
+		Organization: "owner",
+		Repository:   "repository",
+		SHA:          "sha",
+	}
+
+	checkRunID := 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
+				}
+			]
+		}`, checkRunID)
+	})
+
+	id, err := gvcs.getExistingCheckRunID(ctx, event)
+	assert.NilError(t, err)
+	assert.Equal(t, *id, checkRunID)
+}
+
 func TestGithubProviderCreateStatus(t *testing.T) {
 	checkrunid := int64(2026)
 	resultid := int64(666)
@@ -304,3 +337,63 @@ func TestGetCheckName(t *testing.T) {
 		})
 	}
 }
+
+func TestProviderGetExistingCheckRunID(t *testing.T) {
+	tests := []struct {
+		name       string
+		jsonret    string
+		expectedID *int64
+		wantErr    bool
+	}{
+		{
+			name: "has check runs",
+			jsonret: `{
+			"total_count": 1,
+			"check_runs": [
+				{
+					"id": 55555
+				}
+			]
+		}`,
+			expectedID: github.Int64(55555),
+		},
+		{
+			name:       "no check runs",
+			jsonret:    `{"total_count": 0,"check_runs": []}`,
+			expectedID: nil,
+		},
+		{
+			name:       "error it",
+			jsonret:    `BLAHALALACLCALWA`,
+			expectedID: nil,
+			wantErr:    true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, _ := rtesting.SetupFakeContext(t)
+			client, mux, _, teardown := ghtesthelper.SetupGH()
+			defer teardown()
+			event := &info.Event{
+				Organization: "owner",
+				Repository:   "repository",
+				SHA:          "sha",
+			}
+			v := &Provider{
+				Client: client,
+			}
+			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, tt.jsonret)
+			})
+
+			got, err := v.getExistingCheckRunID(ctx, event)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getExistingCheckRunID() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.expectedID) {
+				t.Errorf("getExistingCheckRunID() got = %v, want %v", got, tt.expectedID)
+			}
+		})
+	}
+}