From a30b14a6fe3fc6fb0e327208ef0263f38142d6cd Mon Sep 17 00:00:00 2001 From: Andrea Frittoli Date: Mon, 9 Mar 2020 17:54:01 +0000 Subject: [PATCH] Handle the case of multiple versions of a status The GitHub API keeps old versions of a status for historical reasons. The statuses are returned in reverse chronological order. Let's ignore everything but the most recent status, both when storing the resource to disk, as well as when checking for the need to update before the upload. Fixes #2188 --- pkg/pullrequest/api.go | 13 ++++++--- pkg/pullrequest/api_test.go | 37 +++++++++++++++++++++++++ pkg/pullrequest/disk.go | 7 +++++ pkg/pullrequest/disk_test.go | 52 +++++++++++++++++++++++++----------- 4 files changed, 91 insertions(+), 18 deletions(-) diff --git a/pkg/pullrequest/api.go b/pkg/pullrequest/api.go index f11b6b76393..54bcb826c70 100644 --- a/pkg/pullrequest/api.go +++ b/pkg/pullrequest/api.go @@ -272,15 +272,22 @@ func (h *Handler) uploadStatuses(ctx context.Context, statuses []*scm.Status, sh } // Index the statuses so we can avoid sending them if they already exist. - csMap := map[string]scm.State{} + csMap := map[string]scm.Status{} for _, s := range cs { - csMap[s.Label] = s.State + // Statuses are returned by the API in reverse chronological order: + // https://developer.github.com/v3/repos/statuses/#list-statuses-for-a-specific-ref + // We only consider the most recent status with a give label. + if _, ok := csMap[s.Label]; ok { + continue + } + // + csMap[s.Label] = *s } var merr error for _, s := range statuses { - if csMap[s.Label] == s.State { + if csMap[s.Label].State == s.State && csMap[s.Label].Target == s.Target { h.logger.Infof("Skipping setting %s because it already matches", s.Label) continue } diff --git a/pkg/pullrequest/api_test.go b/pkg/pullrequest/api_test.go index df6ffa46826..80120bd26ec 100644 --- a/pkg/pullrequest/api_test.go +++ b/pkg/pullrequest/api_test.go @@ -268,6 +268,43 @@ func TestUpload_UpdateStatus(t *testing.T) { } } +func TestUpload_UpdateStatus_Duplicate(t *testing.T) { + ctx := context.Background() + h, _ := newHandler(t) + + // Prepare the ground with a resource with two statuses + rDouble := defaultResource() + cancelledStatus := &scm.Status{ + Label: "Tekton", + State: scm.StateCanceled, + Desc: "Test all the things!", + Target: "https://tekton.dev/2", + } + rDouble.Statuses = append(rDouble.Statuses, cancelledStatus) + populateManifest(rDouble) + if err := h.Upload(ctx, rDouble); err != nil { + t.Fatal(err) + } + + // Create the actual resource with the update that we want to upload + // The upload should not be ignored because of the second resource that + // we uploaded previously + r := defaultResource() + r.Statuses[0].State = scm.StateCanceled + if err := h.Upload(ctx, r); err != nil { + t.Fatal(err) + } + + got, err := h.Download(ctx) + if err != nil { + t.Fatal(err) + } + + if diff := cmp.Diff(r, got); diff != "" { + t.Errorf("-want +got: %s", diff) + } +} + func TestUpload_NewLabel(t *testing.T) { ctx := context.Background() h, _ := newHandler(t) diff --git a/pkg/pullrequest/disk.go b/pkg/pullrequest/disk.go index 9de471b8132..e990dc55d4c 100644 --- a/pkg/pullrequest/disk.go +++ b/pkg/pullrequest/disk.go @@ -133,12 +133,19 @@ func labelsToDisk(path string, labels []*scm.Label) error { } func statusToDisk(path string, statuses []*scm.Status) error { + writtenStatuses := make(map[string]interface{}) for _, s := range statuses { + // Statuses are returned by the API in chronological reverse order. Only + // write the first one. + if _, ok := writtenStatuses[s.Label]; ok { + continue + } statusName := url.QueryEscape(s.Label) + ".json" statusPath := filepath.Join(path, statusName) if err := toDisk(statusPath, s, 0600); err != nil { return err } + writtenStatuses[s.Label] = nil } return nil } diff --git a/pkg/pullrequest/disk_test.go b/pkg/pullrequest/disk_test.go index a3086ad2474..e4b4460022d 100644 --- a/pkg/pullrequest/disk_test.go +++ b/pkg/pullrequest/disk_test.go @@ -32,6 +32,41 @@ import ( ) func TestToDisk(t *testing.T) { + allStatuses := []*scm.Status{ + { + Label: "123", + State: scm.StateSuccess, + Desc: "foobar", + Target: "https://foo.bar", + }, + { + Label: "123", + State: scm.StateFailure, + Desc: "foobar", + Target: "https://foo.bar", + }, + { + Label: "cla/foo", + State: scm.StateSuccess, + Desc: "bazbat", + Target: "https://baz.bat", + }, + } + wantStatuses := []*scm.Status{ + { + Label: "123", + State: scm.StateSuccess, + Desc: "foobar", + Target: "https://foo.bar", + }, + { + Label: "cla/foo", + State: scm.StateSuccess, + Desc: "bazbat", + Target: "https://baz.bat", + }, + } + rsrc := &Resource{ PR: &scm.PullRequest{ Number: 123, @@ -51,20 +86,7 @@ func TestToDisk(t *testing.T) { {Name: "foo/bar"}, }, }, - Statuses: []*scm.Status{ - { - Label: "123", - State: scm.StateSuccess, - Desc: "foobar", - Target: "https://foo.bar", - }, - { - Label: "cla/foo", - State: scm.StateSuccess, - Desc: "bazbat", - Target: "https://baz.bat", - }, - }, + Statuses: allStatuses, Comments: []*scm.Comment{{ ID: 123, Body: "hey", @@ -113,7 +135,7 @@ func TestToDisk(t *testing.T) { readAndUnmarshal(t, filepath.Join(d, "status", fi.Name()), &status) statuses[status.Target] = status } - for _, s := range rsrc.Statuses { + for _, s := range wantStatuses { actualStatus, ok := statuses[s.Target] if !ok { t.Errorf("Expected status with ID: %s, not found: %v", s.Target, statuses)