From 0996d937132d6101aac2bab4d837ae4214d49faa Mon Sep 17 00:00:00 2001 From: Billy Lynch Date: Wed, 26 Jun 2019 11:30:32 -0400 Subject: [PATCH] pullrequest-init: Add getters to make PullRequest nil-safe. Adds getters to all funcs so that libraries can safely handle the struct without needing to worry about doing nil checks to avoid panics. This is heavily influenced by the behavior of Go protobufs and other libraries like https://github.com/google/go-github. Also adds test coverage for uploadComments and uploadLabels for nil lists. --- cmd/pullrequest-init/fake_github_test.go | 5 +- cmd/pullrequest-init/github.go | 40 +++-- cmd/pullrequest-init/github_test.go | 194 ++++++++++++++--------- cmd/pullrequest-init/types.go | 119 ++++++++++++++ 4 files changed, 270 insertions(+), 88 deletions(-) diff --git a/cmd/pullrequest-init/fake_github_test.go b/cmd/pullrequest-init/fake_github_test.go index e6dc65ce3e2..e89b6fa84d6 100644 --- a/cmd/pullrequest-init/fake_github_test.go +++ b/cmd/pullrequest-init/fake_github_test.go @@ -41,7 +41,10 @@ func TestFakeGitHubComments(t *testing.T) { t.Fatalf("List Issues: wanted [], got %+v, %+v, %v", got, resp, err) } - if _, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, comment); err != nil { + comment, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ + Body: github.String("tacocat"), + }) + if err != nil { t.Fatalf("CreateComment: %v", err) } diff --git a/cmd/pullrequest-init/github.go b/cmd/pullrequest-init/github.go index 04bf4f41825..960e3abfb0d 100644 --- a/cmd/pullrequest-init/github.go +++ b/cmd/pullrequest-init/github.go @@ -197,11 +197,11 @@ func (h *GitHubHandler) Upload(ctx context.Context, path string) error { } var merr error - if err := h.uploadLabels(ctx, pr.Labels); err != nil { + if err := h.uploadLabels(ctx, pr.GetLabels()); err != nil { merr = multierror.Append(merr, err) } - if err := h.uploadComments(ctx, pr.Comments); err != nil { + if err := h.uploadComments(ctx, pr.GetComments()); err != nil { merr = multierror.Append(merr, err) } @@ -209,9 +209,13 @@ func (h *GitHubHandler) Upload(ctx context.Context, path string) error { } func (h *GitHubHandler) uploadLabels(ctx context.Context, labels []*Label) error { + if len(labels) == 0 { + return nil + } + labelNames := make([]string, 0, len(labels)) for _, l := range labels { - labelNames = append(labelNames, l.Text) + labelNames = append(labelNames, l.GetText()) } h.Logger.Infof("Setting labels for PR %d to %v", h.prNum, labelNames) _, _, err := h.Issues.ReplaceLabelsForIssue(ctx, h.owner, h.repo, h.prNum, labelNames) @@ -219,6 +223,10 @@ func (h *GitHubHandler) uploadLabels(ctx context.Context, labels []*Label) error } func (h *GitHubHandler) uploadComments(ctx context.Context, comments []*Comment) error { + if comments == nil { + return nil + } + h.Logger.Infof("Setting comments for PR %d to: %v", h.prNum, comments) // Sort comments into whether they are new or existing comments (based on @@ -226,8 +234,8 @@ func (h *GitHubHandler) uploadComments(ctx context.Context, comments []*Comment) existingComments := map[int64]*Comment{} newComments := []*Comment{} for _, c := range comments { - if c.ID != 0 { - existingComments[c.ID] = c + if c.GetID() != 0 { + existingComments[c.GetID()] = c } else { newComments = append(newComments, c) } @@ -251,9 +259,6 @@ func (h *GitHubHandler) updateExistingComments(ctx context.Context, comments map return err } - h.Logger.Info(existingComments) - h.Logger.Info(comments) - var merr error for _, ec := range existingComments { dc, ok := comments[ec.GetID()] @@ -265,14 +270,14 @@ func (h *GitHubHandler) updateExistingComments(ctx context.Context, comments map merr = multierror.Append(merr, err) continue } - } else if dc.Text != ec.GetBody() { + } else if dc.GetText() != ec.GetBody() { // Update c := &github.IssueComment{ ID: ec.ID, - Body: github.String(dc.Text), + Body: github.String(dc.GetText()), User: ec.User, } - h.Logger.Infof("Updating comment %d for PR %d to %s", ec.GetID(), h.prNum, dc.Text) + h.Logger.Infof("Updating comment %d for PR %d to %s", ec.GetID(), h.prNum, dc.GetText()) if _, _, err := h.Issues.EditComment(ctx, h.owner, h.repo, ec.GetID(), c); err != nil { h.Logger.Warnf("Error editing comment: %v", err) merr = multierror.Append(merr, err) @@ -287,9 +292,9 @@ func (h *GitHubHandler) createNewComments(ctx context.Context, comments []*Comme var merr error for _, dc := range comments { c := &github.IssueComment{ - Body: github.String(dc.Text), + Body: github.String(dc.GetText()), } - h.Logger.Infof("Creating comment %s for PR %d", dc.Text, h.prNum) + h.Logger.Infof("Creating comment %s for PR %d", dc.GetText(), h.prNum) if _, _, err := h.Issues.CreateComment(ctx, h.owner, h.repo, h.prNum, c); err != nil { h.Logger.Warnf("Error creating comment: %v", err) merr = multierror.Append(merr, err) @@ -297,3 +302,12 @@ func (h *GitHubHandler) createNewComments(ctx context.Context, comments []*Comme } return merr } + +func githubCommentToTekton(c *github.IssueComment, pathPrefix string) *Comment { + return &Comment{ + ID: c.GetID(), + Author: c.GetUser().GetLogin(), + Text: c.GetBody(), + Raw: filepath.Join(pathPrefix, fmt.Sprintf("github/comments/%d.json", c.GetID())), + } +} diff --git a/cmd/pullrequest-init/github_test.go b/cmd/pullrequest-init/github_test.go index ee2bc9e7307..1c9d768e5c0 100644 --- a/cmd/pullrequest-init/github_test.go +++ b/cmd/pullrequest-init/github_test.go @@ -96,10 +96,7 @@ var ( SHA: github.String("2"), }, } - comment = &github.IssueComment{ - ID: github.Int64(1), - Body: github.String("hello world!"), - } + comments []*github.IssueComment ) func githubClient(t *testing.T, gh *FakeGitHub) (*github.Client, func()) { @@ -122,10 +119,22 @@ func newHandler(ctx context.Context, t *testing.T) (*GitHubHandler, func()) { gh := NewFakeGitHub() client, close := githubClient(t, gh) - // Automatically prepopulate GitHub server to ease test setup. + // Prepopulate GitHub server with defaults to ease test setup. gh.AddPullRequest(pr) - if _, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, comment); err != nil { - t.Fatalf("Create Comment: %v", err) + commentBody := []string{ + "hello world!", + "a", + "b", + } + comments = make([]*github.IssueComment, 0, len(commentBody)) + for _, b := range commentBody { + c, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ + Body: github.String(b), + }) + if err != nil { + t.Fatalf("error creating comment: %v", err) + } + comments = append(comments, c) } h, err := NewGitHubHandler(ctx, zaptest.NewLogger(t, zaptest.WrapOptions(zap.AddCaller())).Sugar(), pr.GetHTMLURL()) @@ -149,7 +158,12 @@ func TestGitHub(t *testing.T) { prPath := filepath.Join(dir, "pr.json") rawPRPath := filepath.Join(dir, "github/pr.json") - rawCommentPath := filepath.Join(dir, "github/comments/1.json") + + tknComments := []*Comment{ + githubCommentToTekton(comments[0], dir), + githubCommentToTekton(comments[1], dir), + githubCommentToTekton(comments[2], dir), + } wantPR := &PullRequest{ Type: "github", @@ -164,14 +178,9 @@ func TestGitHub(t *testing.T) { Branch: pr.GetBase().GetRef(), SHA: pr.GetBase().GetSHA(), }, - Comments: []*Comment{{ - ID: comment.GetID(), - Author: comment.GetUser().GetLogin(), - Text: comment.GetBody(), - Raw: rawCommentPath, - }}, - Labels: []*Label{}, - Raw: rawPRPath, + Comments: tknComments, + Labels: []*Label{}, + Raw: rawPRPath, } gotPR := new(PullRequest) @@ -180,9 +189,13 @@ func TestGitHub(t *testing.T) { if rawPRPath != gotPR.Raw { t.Errorf("Raw PR path: want [%s], got [%s]", rawPRPath, gotPR.Raw) } - diffFile(t, rawCommentPath, comment, new(github.IssueComment)) - if rawCommentPath != gotPR.Comments[0].Raw { - t.Errorf("Raw PR path: want [%s], got [%s]", rawCommentPath, gotPR.Comments[0].Raw) + for i, c := range tknComments { + t.Run(fmt.Sprintf("Comment%d", c.GetID()), func(t *testing.T) { + diffFile(t, c.GetRaw(), comments[i], new(github.IssueComment)) + if c.GetRaw() != gotPR.Comments[i].Raw { + t.Errorf("Raw PR path: want [%s], got [%s]", c.GetRaw(), gotPR.Comments[i].Raw) + } + }) } } @@ -205,9 +218,9 @@ func TestUpload(t *testing.T) { SHA: pr.GetBase().GetSHA(), }, Comments: []*Comment{{ - ID: comment.GetID(), - Author: comment.GetUser().GetLogin(), - Text: comment.GetBody(), + ID: comments[0].GetID(), + Author: comments[0].GetUser().GetLogin(), + Text: comments[0].GetBody(), }, { Text: "abc123", }}, @@ -245,7 +258,7 @@ func TestUpload(t *testing.T) { if err != nil { t.Fatal(err) } - wantComments := []*github.IssueComment{comment, &github.IssueComment{ + wantComments := []*github.IssueComment{comments[0], &github.IssueComment{ ID: github.Int64(2), Body: github.String(tektonPR.Comments[1].Text), }} @@ -356,7 +369,7 @@ func TestCreateNewComments(t *testing.T) { h, close := newHandler(ctx, t) defer close() - comments := []*Comment{ + req := []*Comment{ { // Forces comment creation to fail. Text: ErrorKeyword, @@ -365,7 +378,7 @@ func TestCreateNewComments(t *testing.T) { Text: "b", }, } - if err := h.createNewComments(ctx, comments); err == nil { + if err := h.createNewComments(ctx, req); err == nil { t.Error("expected error, got nil") } @@ -373,13 +386,10 @@ func TestCreateNewComments(t *testing.T) { if err != nil { t.Fatalf("GetComments: %v", err) } - want := []*github.IssueComment{ - comment, - { - ID: github.Int64(2), - Body: github.String(comments[1].Text), - }, - } + want := append(comments, &github.IssueComment{ + ID: github.Int64(int64(len(comments) + 1)), + Body: github.String(req[1].GetText()), + }) if diff := cmp.Diff(want, got); diff != "" { t.Errorf("-want +got: %v", diff) } @@ -390,39 +400,25 @@ func TestUpdateExistingComments(t *testing.T) { h, close := newHandler(ctx, t) defer close() - comment2, _, err := h.Client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ - Body: github.String("a"), - }) - if err != nil { - t.Fatalf("error creating comment: %v", err) - } - comment3, _, err := h.Client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ - Body: github.String("b"), - }) - if err != nil { - t.Fatalf("error creating comment: %v", err) - } - comment3.Body = github.String("tacocat") - - comments := map[int64]*Comment{ + req := map[int64]*Comment{ // Non-existant comment. Should be ignored. 8675309: &Comment{ ID: 8675309, - Text: comment.GetBody(), + Text: comments[0].GetBody(), }, // Comment that fails to update. Should not affect later updates. - comment2.GetID(): &Comment{ - ID: comment2.GetID(), + comments[1].GetID(): &Comment{ + ID: comments[1].GetID(), Text: ErrorKeyword, }, // Normal update. - comment3.GetID(): &Comment{ - ID: comment3.GetID(), - Text: comment3.GetBody(), + comments[2].GetID(): &Comment{ + ID: comments[2].GetID(), + Text: "tacocat", }, // Comment 1 should be deleted. } - if err := h.updateExistingComments(ctx, comments); err == nil { + if err := h.updateExistingComments(ctx, req); err == nil { t.Error("expected error, got nil") } @@ -430,7 +426,13 @@ func TestUpdateExistingComments(t *testing.T) { if err != nil { t.Fatalf("GetComments: %v", err) } - want := []*github.IssueComment{comment2, comment3} + want := []*github.IssueComment{ + comments[1], + { + ID: comments[2].ID, + Body: github.String(req[comments[2].GetID()].GetText()), + }, + } if diff := cmp.Diff(want, got); diff != "" { t.Errorf("-want +got: %v", diff) } @@ -441,39 +443,29 @@ func TestUploadComments(t *testing.T) { h, close := newHandler(ctx, t) defer close() - comment2, _, err := h.Client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ - Body: github.String("a"), - }) - if err != nil { - t.Fatalf("error creating comment: %v", err) + if err := h.uploadComments(ctx, nil); err != nil { + t.Errorf("uploadComments(nil): %v", err) } - comment3, _, err := h.Client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ - Body: github.String("b"), - }) - if err != nil { - t.Fatalf("error creating comment: %v", err) - } - comment3.Body = github.String("tacocat") - comments := []*Comment{ + req := []*Comment{ // Non-existant comment. Should be ignored. { ID: 8675309, - Text: comment.GetBody(), + Text: comments[0].GetBody(), }, // Comment that fails to update. Should not affect later updates. { - ID: comment2.GetID(), + ID: comments[1].GetID(), Text: ErrorKeyword, }, // Normal update. { - ID: comment3.GetID(), - Text: comment3.GetBody(), + ID: comments[2].GetID(), + Text: "tacocat", }, // Comment 1 should be deleted. } - if err := h.uploadComments(ctx, comments); err == nil { + if err := h.uploadComments(ctx, req); err == nil { t.Error("expected error, got nil") } @@ -481,8 +473,62 @@ func TestUploadComments(t *testing.T) { if err != nil { t.Fatalf("GetComments: %v", err) } - want := []*github.IssueComment{comment2, comment3} + want := []*github.IssueComment{ + comments[1], + { + ID: comments[2].ID, + Body: github.String(req[2].GetText()), + }, + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("-want +got: %v", diff) + } + + // Perform a no-op upload, verify it doesn't error and that it doesn't delete + // all existing comments. + if err := h.uploadComments(ctx, nil); err != nil { + t.Fatalf("uploadComments: %v", err) + } + got, _, err = h.Client.Issues.ListComments(ctx, owner, repo, prNum, nil) + if err != nil { + t.Fatalf("GetComments: %v", err) + } if diff := cmp.Diff(want, got); diff != "" { t.Errorf("-want +got: %v", diff) } } + +func TestUploadLabels(t *testing.T) { + ctx := context.Background() + h, close := newHandler(ctx, t) + defer close() + + text := "tacocat" + l := []*Label{{Text: text}} + + if err := h.uploadLabels(ctx, l); err != nil { + t.Errorf("uploadLabels: %v", err) + } + + pr, _, err := h.Client.PullRequests.Get(ctx, h.owner, h.repo, h.prNum) + if err != nil { + t.Fatalf("GetPullRequest: %v", err) + } + want := []*github.Label{{Name: github.String(text)}} + if diff := cmp.Diff(want, pr.Labels); diff != "" { + t.Errorf("-want +got: %v", diff) + } + + // Perform a no-op upload, verify it doesn't error and that it doesn't delete + // all existing labels. + if err := h.uploadLabels(ctx, nil); err != nil { + t.Errorf("uploadLabels(nil): %v", err) + } + pr, _, err = h.Client.PullRequests.Get(ctx, h.owner, h.repo, h.prNum) + if err != nil { + t.Fatalf("GetPullRequest: %v", err) + } + if diff := cmp.Diff(want, pr.Labels); diff != "" { + t.Errorf("-want +got: %v", diff) + } +} diff --git a/cmd/pullrequest-init/types.go b/cmd/pullrequest-init/types.go index 4b71bc63a7d..54d355054c5 100644 --- a/cmd/pullrequest-init/types.go +++ b/cmd/pullrequest-init/types.go @@ -15,6 +15,62 @@ type PullRequest struct { Raw string } +// GetType returns the Type field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetType() string { + if pr == nil { + return "" + } + return pr.Type +} + +// GetID returns the ID field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetID() int64 { + if pr == nil { + return 0 + } + return pr.ID +} + +// GetHead returns the Head field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetHead() *GitReference { + if pr == nil { + return nil + } + return pr.Head +} + +// GetBase returns the Base field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetBase() *GitReference { + if pr == nil { + return nil + } + return pr.Base +} + +// GetComments returns the Comments field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetComments() []*Comment { + if pr == nil { + return nil + } + return pr.Comments +} + +// GetLabels returns the Labels field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetLabels() []*Label { + if pr == nil { + return nil + } + return pr.Labels +} + +// GetRaw returns the Raw field, or zero value if PullRequest is nil. +func (pr *PullRequest) GetRaw() string { + if pr == nil { + return "" + } + return pr.Raw +} + // GitReference represents a git ref object. See // https://git-scm.com/book/en/v2/Git-Internals-Git-References for more details. type GitReference struct { @@ -23,6 +79,30 @@ type GitReference struct { SHA string } +// GetRepo returns the Repo field, or zero value if GitReference is nil. +func (r *GitReference) GetRepo() string { + if r == nil { + return "" + } + return r.Repo +} + +// GetBranch returns the Branch field, or zero value if GitReference is nil. +func (r *GitReference) GetBranch() string { + if r == nil { + return "" + } + return r.Branch +} + +// GetSHA returns the SHA field, or zero value if GitReference is nil. +func (r *GitReference) GetSHA() string { + if r == nil { + return "" + } + return r.SHA +} + // Comment represents a pull request comment. type Comment struct { Text string @@ -31,7 +111,46 @@ type Comment struct { Raw string } +func (c *Comment) GetText() string { + if c == nil { + return "" + } + return c.Text +} + +// GetAuthor returns the Author field, or zero value if Comment is nil. +func (c *Comment) GetAuthor() string { + if c == nil { + return "" + } + return c.Author +} + +// GetID returns the ID field, or zero value if Comment is nil. +func (c *Comment) GetID() int64 { + if c == nil { + return 0 + } + return c.ID +} + +// GetRaw returns the Raw field, or zero value if Comment is nil. +func (c *Comment) GetRaw() string { + if c == nil { + return "" + } + return c.Raw +} + // Label represents a Pull Request Label type Label struct { Text string } + +// GetText returns the Text field, or zero value if Label is nil. +func (l *Label) GetText() string { + if l == nil { + return "" + } + return l.Text +}