diff --git a/pkg/pullrequest/api.go b/pkg/pullrequest/api.go index f11b6b76393..0b6b0b197d4 100644 --- a/pkg/pullrequest/api.go +++ b/pkg/pullrequest/api.go @@ -272,15 +272,26 @@ 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 + // Because of https://github.com/jenkins-x/go-scm/issues/79, in case + // of the GitHub backend the ListStatus API may return multiple statuses + // with the same label that cannot be distinguished between each other + // Until the issues is fixed, we only consider the most recent status with + // a given label. We can do that because statuses are returned in reverse + // chronological order by the GitHub API. + if _, ok := csMap[s.Label]; ok { + continue + } + // + csMap[s.Label] = *s } var merr error for _, s := range statuses { - if csMap[s.Label] == s.State { + oldState := csMap[s.Label] + if oldState.State == s.State && oldState.Target == s.Target && oldState.Desc == s.Desc { 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..19d88ef303d 100644 --- a/pkg/pullrequest/api_test.go +++ b/pkg/pullrequest/api_test.go @@ -268,6 +268,50 @@ func TestUpload_UpdateStatus(t *testing.T) { } } +func TestUpload_UpdateStatus_Duplicate(t *testing.T) { + // Because of https://github.com/jenkins-x/go-scm/issues/79, in case + // of the GitHub backend the ListStatus API may return multiple statuses + // with the same label that cannot be distinguished between each other + // Until the issues is fixed, we only consider the most recent status with + // a give label. We can do that because statuses are returned in reverse + // chronological order by the GitHub API. + // Once the issues is fixed on go-scm side, we won't need this test anymore. + 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..7f36dbc7b02 100644 --- a/pkg/pullrequest/disk.go +++ b/pkg/pullrequest/disk.go @@ -133,12 +133,23 @@ 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 { + // Because of https://github.com/jenkins-x/go-scm/issues/79, in case + // of the GitHub backend the ListStatus API may return multiple statuses + // with the same label that cannot be distinguished between each other + // Until the issues is fixed, we only consider the most recent status with + // a give label. We can do that because statuses are returned in reverse + // chronological order by the GitHub API. + 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..8266d75295b 100644 --- a/pkg/pullrequest/disk_test.go +++ b/pkg/pullrequest/disk_test.go @@ -32,6 +32,47 @@ 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", + }, + } + // Because of https://github.com/jenkins-x/go-scm/issues/79, in case + // of the GitHub backend the ListStatus API may return multiple statuses + // with the same label that cannot be distinguished between each other + // Until the issues is fixed, we only consider the most recent status with + // a give label. We can do that because statuses are returned in reverse + // chronological order by the GitHub API. + 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 +92,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 +141,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)