Repository: jenkins-x/lighthouse
Pull request #723 (2020-05-08 15:29:59)

fix: Failed pipeline report comments should use PR API, not issues
Thanks to GitHub treating PRs and issues the same for a lot of things, we inherited this logic using `ListIssueComments`, but we should actually be using `ListPullRequestComments`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>


Patch:
diff --git a/pkg/scmprovider/reporter/reporter.go b/pkg/scmprovider/reporter/reporter.go
index e8f78cc8..bfcb956c 100644
--- a/pkg/scmprovider/reporter/reporter.go
+++ b/pkg/scmprovider/reporter/reporter.go
@@ -36,7 +36,7 @@ const (
 // through GitHub comments.
 type SCMProviderClient interface {
 	BotName() (string, error)
-	ListIssueComments(string, string, int) ([]*scm.Comment, error)
+	ListPullRequestComments(string, string, int) ([]*scm.Comment, error)
 	CreateComment(string, string, int, bool, string) error
 	DeleteComment(string, string, int, int, bool) error
 	EditComment(string, string, int, int, string, bool) error
@@ -83,7 +83,7 @@ func Report(spc SCMProviderClient, reportTemplate *template.Template, lhj *v1alp
 		return nil
 	}
 
-	ics, err := spc.ListIssueComments(refs.Org, refs.Repo, refs.Pulls[0].Number)
+	prcs, err := spc.ListPullRequestComments(refs.Org, refs.Repo, refs.Pulls[0].Number)
 	if err != nil {
 		return fmt.Errorf("error listing comments: %v", err)
 	}
@@ -91,7 +91,7 @@ func Report(spc SCMProviderClient, reportTemplate *template.Template, lhj *v1alp
 	if err != nil {
 		return fmt.Errorf("error getting bot name: %v", err)
 	}
-	deletes, entries, updateID := parseIssueComments(lhj, botName, ics)
+	deletes, entries, updateID := parsePRComments(lhj, botName, prcs)
 	for _, delete := range deletes {
 		if err := spc.DeleteComment(refs.Org, refs.Repo, refs.Pulls[0].Number, delete, true); err != nil {
 			return fmt.Errorf("error deleting comment: %v", err)
@@ -115,12 +115,12 @@ func Report(spc SCMProviderClient, reportTemplate *template.Template, lhj *v1alp
 	return nil
 }
 
-// parseIssueComments returns a list of comments to delete, a list of table
+// parsePRComments returns a list of comments to delete, a list of table
 // entries, and the ID of the comment to update. If there are no table entries
 // then don't make a new comment. Otherwise, if the comment to update is 0,
 // create a new comment.
-func parseIssueComments(lhj *v1alpha1.LighthouseJob, botName string, ics []*scm.Comment) ([]int, []string, int) {
-	var delete []int
+func parsePRComments(lhj *v1alpha1.LighthouseJob, botName string, ics []*scm.Comment) ([]int, []string, int) {
+	var toDelete []int
 	var previousComments []int
 	var latestComment int
 	var entries []string
@@ -132,7 +132,7 @@ func parseIssueComments(lhj *v1alpha1.LighthouseJob, botName string, ics []*scm.
 		// Old report comments started with the context. Delete them.
 		// TODO(spxtr): Delete this check a few weeks after this merges.
 		if strings.HasPrefix(ic.Body, lhj.Spec.Context) {
-			delete = append(delete, ic.ID)
+			toDelete = append(toDelete, ic.ID)
 		}
 		if !strings.Contains(ic.Body, commentTag) {
 			continue
@@ -181,12 +181,12 @@ func parseIssueComments(lhj *v1alpha1.LighthouseJob, botName string, ics []*scm.
 		newEntries = append(newEntries, createEntry(lhj))
 		createNewComment = true
 	}
-	delete = append(delete, previousComments...)
+	toDelete = append(toDelete, previousComments...)
 	if (createNewComment || len(newEntries) == 0) && latestComment != 0 {
-		delete = append(delete, latestComment)
+		toDelete = append(toDelete, latestComment)
 		latestComment = 0
 	}
-	return delete, newEntries, latestComment
+	return toDelete, newEntries, latestComment
 }
 
 func createEntry(lhj *v1alpha1.LighthouseJob) string {


Test patch:
diff --git a/pkg/scmprovider/reporter/reporter_test.go b/pkg/scmprovider/reporter/reporter_test.go
index e8aab3ba..97b635a8 100644
--- a/pkg/scmprovider/reporter/reporter_test.go
+++ b/pkg/scmprovider/reporter/reporter_test.go
@@ -24,12 +24,12 @@ import (
 	"github.com/jenkins-x/lighthouse/pkg/apis/lighthouse/v1alpha1"
 )
 
-func TestParseIssueComment(t *testing.T) {
+func TestParsePRComment(t *testing.T) {
 	var testcases = []struct {
 		name             string
 		context          string
 		state            v1alpha1.PipelineState
-		ics              []*scm.Comment
+		prcs             []*scm.Comment
 		expectedDeletes  []int
 		expectedContexts []string
 		expectedUpdate   int
@@ -38,7 +38,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should delete old style comments",
 			context: "Jenkins foo test",
 			state:   v1alpha1.SuccessState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "Jenkins foo test **failed** for such-and-such.",
@@ -72,7 +72,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should not delete an up-to-date comment",
 			context: "bla test",
 			state:   v1alpha1.SuccessState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\nfoo test | something | or other\n\n",
@@ -83,7 +83,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should delete when all tests pass",
 			context: "bla test",
 			state:   v1alpha1.SuccessState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\nbla test | something | or other\n\n" + commentTag,
@@ -97,7 +97,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should delete a passing test with \\r",
 			context: "bla test",
 			state:   v1alpha1.SuccessState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\r\nbla test | something | or other\r\n\r\n" + commentTag,
@@ -112,7 +112,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should update a failed test",
 			context: "bla test",
 			state:   v1alpha1.FailureState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\nbla test | something | or other\n\n" + commentTag,
@@ -126,7 +126,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should preserve old results when updating",
 			context: "bla test",
 			state:   v1alpha1.FailureState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\nbla test | something | or other\nfoo test | wow | aye\n\n" + commentTag,
@@ -140,7 +140,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should merge duplicates",
 			context: "bla test",
 			state:   v1alpha1.FailureState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\nbla test | something | or other\nfoo test | wow such\n\n" + commentTag,
@@ -159,7 +159,7 @@ func TestParseIssueComment(t *testing.T) {
 			name:    "should update an old comment when a test passes",
 			context: "bla test",
 			state:   v1alpha1.SuccessState,
-			ics: []*scm.Comment{
+			prcs: []*scm.Comment{
 				{
 					Author: scm.User{Login: "k8s-ci-robot"},
 					Body:   "--- | --- | ---\nbla test | something | or other\nfoo test | wow | aye\n\n" + commentTag,
@@ -182,7 +182,7 @@ func TestParseIssueComment(t *testing.T) {
 					State: tc.state,
 				},
 			}
-			deletes, entries, update := parseIssueComments(lhj, "k8s-ci-robot", tc.ics)
+			deletes, entries, update := parsePRComments(lhj, "k8s-ci-robot", tc.prcs)
 			if len(deletes) != len(tc.expectedDeletes) {
 				t.Errorf("It %s: wrong number of deletes. Got %v, expected %v", tc.name, deletes, tc.expectedDeletes)
 			} else {