Add support for comment and label manifests.

Introduces `.MANIFEST` files to comment and label directories to keep
track of sub-resources when the PullRequest resource is initialized.
This allows us to keep track of what resources the user explicitly
deleted, and not accidentally overwrite new comments/lables that were
introduced during execution.

Minor changes included:
* Changed fake GitHub server to make copies of PullRequests to avoid
  pointer collision across tests.
* Changed uploadLabels to Add/Delete labels explicitly instead of
  replacing all.

Change manifest path to .MANIFEST to avoid collisions.

Fixes #1286

Author: wlynch

cmd/pullrequest-init/disk.go

@@ -17,7 +17,9 @@ limitations under the License.
 package main
 
 import (
+	"bufio"
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"net/url"
 	"os"
@@ -40,6 +42,10 @@ import (
 
 // Filenames for labels and statuses are URL encoded for safety.
 
+const (
+	manifestPath = ".MANIFEST"
+)
+
 // ToDisk converts a PullRequest object to an on-disk representation at the specified path.
 func ToDisk(pr *PullRequest, path string) error {
 	labelsPath := filepath.Join(path, "labels")
@@ -80,28 +86,53 @@ func ToDisk(pr *PullRequest, path string) error {
 }
 
 func commentsToDisk(path string, comments []*Comment) error {
+	// Create a manifest to keep track of the comments that existed when the
+	// resource was initialized. This is used to verify that a comment that
+	// doesn't exist on disk was actually deleted by the user and not newly
+	// created during upload.
+	f, err := os.Create(filepath.Join(path, manifestPath))
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	manifest := bufio.NewWriter(f)
+
 	for _, c := range comments {
 		commentPath := filepath.Join(path, strconv.FormatInt(c.ID, 10)+".json")
 		b, err := json.Marshal(c)
 		if err != nil {
 			return err
 		}
-		if err := ioutil.WriteFile(commentPath, b, 0700); err != nil {
+		if err := ioutil.WriteFile(commentPath, b, 0600); err != nil {
+			return err
+		}
+		if _, err := manifest.WriteString(fmt.Sprintf("%s\n", commentPath)); err != nil {
 			return err
 		}
 	}
-	return nil
+	return manifest.Flush()
 }
 
 func labelsToDisk(path string, labels []*Label) error {
+	f, err := os.Create(filepath.Join(path, manifestPath))
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	manifest := bufio.NewWriter(f)
+
 	for _, l := range labels {
 		name := url.QueryEscape(l.Text)
 		labelPath := filepath.Join(path, name)
-		if err := ioutil.WriteFile(labelPath, []byte{}, 0700); err != nil {
+		if err := ioutil.WriteFile(labelPath, []byte{}, 0600); err != nil {
+			return err
+		}
+		if _, err := manifest.WriteString(fmt.Sprintf("%s\n", labelPath)); err != nil {
 			return err
 		}
+
 	}
-	return nil
+	return manifest.Flush()
 }
 
 func statusToDisk(path string, statuses []*Status) error {
@@ -127,57 +158,86 @@ func refToDisk(name, path string, r *GitReference) error {
 	return ioutil.WriteFile(filepath.Join(path, name+".json"), b, 0700)
 }
 
+// Manifest is a list of sub-resources that exist within the PR resource to
+// determine whether an item existed when the resource was initialized.
+type Manifest map[string]bool
+
 // FromDisk outputs a PullRequest object from an on-disk representation at the specified path.
-func FromDisk(path string) (*PullRequest, error) {
+func FromDisk(path string) (*PullRequest, map[string]Manifest, error) {
 	labelsPath := filepath.Join(path, "labels")
 	commentsPath := filepath.Join(path, "comments")
 	statusesPath := filepath.Join(path, "status")
 
 	pr := PullRequest{}
+	manifests := make(map[string]Manifest)
 
 	var err error
+	var manifest Manifest
 
 	// Start with comments
-	pr.Comments, err = commentsFromDisk(commentsPath)
+	pr.Comments, manifest, err = commentsFromDisk(commentsPath)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
+	manifests["comments"] = manifest
 
 	// Now Labels
-	pr.Labels, err = labelsFromDisk(labelsPath)
+	pr.Labels, manifest, err = labelsFromDisk(labelsPath)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
+	manifests["labels"] = manifest
 
 	// Now statuses
 	pr.Statuses, err = statusesFromDisk(statusesPath)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	// Finally refs
 
 	pr.Base, err = refFromDisk(path, "base.json")
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	pr.Head, err = refFromDisk(path, "head.json")
+	if err != nil {
+		return nil, nil, err
+	}
+	return &pr, manifests, nil
+}
+
+func manifestFromDisk(path string) (map[string]bool, error) {
+	f, err := os.Open(path)
 	if err != nil {
 		return nil, err
 	}
-	return &pr, nil
+	defer f.Close()
+
+	out := make(map[string]bool)
+	s := bufio.NewScanner(f)
+	for s.Scan() {
+		out[s.Text()] = true
+	}
+	if s.Err() != nil {
+		return nil, err
+	}
+	return out, nil
 }
 
-func commentsFromDisk(path string) ([]*Comment, error) {
+func commentsFromDisk(path string) ([]*Comment, Manifest, error) {
 	fis, err := ioutil.ReadDir(path)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	comments := []*Comment{}
 	for _, fi := range fis {
+		if fi.Name() == manifestPath {
+			continue
+		}
 		b, err := ioutil.ReadFile(filepath.Join(path, fi.Name()))
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		comment := Comment{}
 		if err := json.Unmarshal(b, &comment); err != nil {
@@ -186,23 +246,38 @@ func commentsFromDisk(path string) ([]*Comment, error) {
 		}
 		comments = append(comments, &comment)
 	}
-	return comments, nil
+
+	manifest, err := manifestFromDisk(filepath.Join(path, manifestPath))
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return comments, manifest, nil
 }
 
-func labelsFromDisk(path string) ([]*Label, error) {
+func labelsFromDisk(path string) ([]*Label, Manifest, error) {
 	fis, err := ioutil.ReadDir(path)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	labels := []*Label{}
 	for _, fi := range fis {
+		if fi.Name() == manifestPath {
+			continue
+		}
 		text, err := url.QueryUnescape(fi.Name())
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 		labels = append(labels, &Label{Text: text})
 	}
-	return labels, nil
+
+	manifest, err := manifestFromDisk(filepath.Join(path, manifestPath))
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return labels, manifest, nil
 }
 
 func statusesFromDisk(path string) ([]*Status, error) {

cmd/pullrequest-init/disk_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package main
 
 import (
+	"bufio"
 	"encoding/json"
 	"io/ioutil"
 	"net/url"
@@ -25,6 +26,7 @@ import (
 	"reflect"
 	"sort"
 	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -113,7 +115,6 @@ func TestToDisk(t *testing.T) {
 			t.Errorf("Get Status: -want +got: %s", diff)
 		}
 	}
-
 	// Check the labels
 	fis, err = ioutil.ReadDir(filepath.Join(d, "labels"))
 	if err != nil {
@@ -141,10 +142,16 @@ func TestToDisk(t *testing.T) {
 	}
 
 	comments := map[int64]Comment{}
+	commentManifest := map[string]bool{}
 	for _, fi := range fis {
+		if fi.Name() == manifestPath {
+			continue
+		}
 		comment := Comment{}
-		readAndUnmarshal(t, filepath.Join(d, "comments", fi.Name()), &comment)
+		path := filepath.Join(d, "comments", fi.Name())
+		readAndUnmarshal(t, path, &comment)
 		comments[comment.ID] = comment
+		commentManifest[path] = true
 	}
 	for _, c := range tektonPr.Comments {
 		actualComment, ok := comments[c.ID]
@@ -155,6 +162,33 @@ func TestToDisk(t *testing.T) {
 			t.Errorf("Get Comment: -want +got: %s", diff)
 		}
 	}
+	gotManifest := readManifest(t, filepath.Join(d, "comments", manifestPath))
+	for _, m := range gotManifest {
+		if _, ok := commentManifest[m]; !ok {
+			t.Errorf("Comment %s not found in manifest", m)
+		}
+	}
+	if len(commentManifest) != len(gotManifest) {
+		t.Errorf("Comment manifest does not match expected length. Got %+v", gotManifest)
+	}
+}
+
+func readManifest(t *testing.T, path string) []string {
+	f, err := os.Open(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	var out []string
+	s := bufio.NewScanner(f)
+	for s.Scan() {
+		out = append(out, s.Text())
+	}
+	if s.Err() != nil {
+		t.Fatal(err)
+	}
+	return out
 }
 
 func TestFromDisk(t *testing.T) {
@@ -218,6 +252,9 @@ func TestFromDisk(t *testing.T) {
 			t.Fatal(err)
 		}
 	}
+	if err := ioutil.WriteFile(filepath.Join(d, "labels", manifestPath), []byte(strings.Join(labels, "\n")), 0600); err != nil {
+		t.Errorf("Error creating label manifest: %s", err)
+	}
 
 	// Write some comments
 	comments := []Comment{
@@ -236,16 +273,22 @@ func TestFromDisk(t *testing.T) {
 	if err := os.MkdirAll(filepath.Join(d, "comments"), 0750); err != nil {
 		t.Fatal(err)
 	}
+	manifest := make([]string, 0, len(comments))
 	for _, c := range comments {
-		writeFile(filepath.Join(d, "comments", strconv.FormatInt(c.ID, 10)+".json"), &c)
+		id := strconv.FormatInt(c.ID, 10)
+		writeFile(filepath.Join(d, "comments", id+".json"), &c)
+		manifest = append(manifest, id)
+	}
+	if err := ioutil.WriteFile(filepath.Join(d, "comments", manifestPath), []byte(strings.Join(manifest, "\n")), 0700); err != nil {
+		t.Errorf("Error creating comment manifest: %s", err)
 	}
 
 	// Comments can also be plain text.
 	if err := ioutil.WriteFile(filepath.Join(d, "comments", "plain"), []byte("plaincomment"), 0700); err != nil {
 		t.Fatal(err)
 	}
 
-	pr, err := FromDisk(d)
+	pr, gotManifest, err := FromDisk(d)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -271,6 +314,13 @@ func TestFromDisk(t *testing.T) {
 			t.Errorf("Get comments: -want +got: %s", diff)
 		}
 	}
+	commentManifest := Manifest{}
+	for _, c := range comments {
+		commentManifest[strconv.FormatInt(c.ID, 10)] = true
+	}
+	if diff := cmp.Diff(commentManifest, gotManifest["comments"]); diff != "" {
+		t.Errorf("Comment manifest: -want + got: %s", diff)
+	}
 
 	// Check the labels
 	labelsMap := map[string]struct{}{}
@@ -283,6 +333,13 @@ func TestFromDisk(t *testing.T) {
 			t.Errorf("Get labels: -want +got: %s", diff)
 		}
 	}
+	labelManifest := Manifest{}
+	for _, l := range labels {
+		labelManifest[l] = true
+	}
+	if diff := cmp.Diff(labelManifest, gotManifest["labels"]); diff != "" {
+		t.Errorf("Label manifest: -want + got: %s", diff)
+	}
 
 	// Check the statuses
 	statusMap := map[string]Status{}
@@ -495,7 +552,10 @@ func Test_labelsFromDisk(t *testing.T) {
 					t.Errorf("Error creating label: %s", err)
 				}
 			}
-			got, err := labelsFromDisk(d)
+			if err := ioutil.WriteFile(filepath.Join(d, manifestPath), []byte(strings.Join(tt.args.fileNames, "\n")), 0700); err != nil {
+				t.Errorf("Error creating label manifest: %s", err)
+			}
+			got, _, err := labelsFromDisk(d)
 			if err != nil {
 				t.Errorf("labelsFromDisk() error = %v", err)
 			}