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 tektoncd#2188

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
  }

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)

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
 }

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)