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

@@ -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
  }

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)

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
 }

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)