From 4f72f2334980674fff1500cbe5b190e6c93636c1 Mon Sep 17 00:00:00 2001 From: Aayush Gupta <43479002+Aayyush@users.noreply.github.com> Date: Thu, 26 May 2022 16:04:50 -0700 Subject: [PATCH] [WENGINES-4505] Github Client Checks Implementation (#254) --- server/controllers/events/handlers/comment.go | 5 +- .../events/handlers/pull_request.go | 5 +- server/events/output_updater.go | 37 ++- server/events/vcs/github_client.go | 148 ++++++++++- server/events/vcs/github_client_test.go | 230 ++++++++++++++++++ server/events/vcs/instrumented_client.go | 2 +- server/events/vcs/types/status.go | 1 + 7 files changed, 411 insertions(+), 17 deletions(-) diff --git a/server/controllers/events/handlers/comment.go b/server/controllers/events/handlers/comment.go index 65bfc9396d..dd26c8610b 100644 --- a/server/controllers/events/handlers/comment.go +++ b/server/controllers/events/handlers/comment.go @@ -92,10 +92,11 @@ type asyncHandler struct { func (h *asyncHandler) Handle(ctx context.Context, request *http.BufferedRequest, event event_types.Comment, command *command.Comment) error { go func() { - err := h.commandHandler.Handle(ctx, request, event, command) + // Passing background context to avoid context cancellation since the parent goroutine does not wait for this goroutine to finish execution. + err := h.commandHandler.Handle(context.Background(), request, event, command) if err != nil { - h.logger.ErrorContext(ctx, err.Error()) + h.logger.ErrorContext(context.Background(), err.Error()) } }() return nil diff --git a/server/controllers/events/handlers/pull_request.go b/server/controllers/events/handlers/pull_request.go index 733462e47a..674a209e80 100644 --- a/server/controllers/events/handlers/pull_request.go +++ b/server/controllers/events/handlers/pull_request.go @@ -40,10 +40,11 @@ type asyncAutoplanner struct { func (p *asyncAutoplanner) Handle(ctx context.Context, request *http.BufferedRequest, event event_types.PullRequest) error { go func() { - err := p.autoplanner.Handle(ctx, request, event) + // Passing background context to avoid context cancellation since the parent goroutine does not wait for this goroutine to finish execution. + err := p.autoplanner.Handle(context.Background(), request, event) if err != nil { - p.logger.ErrorContext(ctx, err.Error()) + p.logger.ErrorContext(context.Background(), err.Error()) } }() return nil diff --git a/server/events/output_updater.go b/server/events/output_updater.go index 1570cc86f6..eb972fcced 100644 --- a/server/events/output_updater.go +++ b/server/events/output_updater.go @@ -2,6 +2,7 @@ package events import ( "fmt" + "strings" "github.com/runatlantis/atlantis/server/events/command" "github.com/runatlantis/atlantis/server/events/models" @@ -49,19 +50,53 @@ type ChecksOutputUpdater struct { func (c *ChecksOutputUpdater) UpdateOutput(ctx *command.Context, cmd PullCommand, res command.Result) { + if res.Error != nil || res.Failure != "" { + output := c.MarkdownRenderer.Render(res, cmd.CommandName(), ctx.Pull.BaseRepo) + updateStatusReq := types.UpdateStatusRequest{ + Repo: ctx.HeadRepo, + Ref: ctx.Pull.HeadCommit, + StatusName: c.TitleBuilder.Build(cmd.CommandName().String()), + PullNum: ctx.Pull.Num, + Description: fmt.Sprintf("%s failed", strings.Title(cmd.CommandName().String())), + Output: output, + State: models.FailedCommitStatus, + } + + if err := c.VCSClient.UpdateStatus(ctx.RequestCtx, updateStatusReq); err != nil { + ctx.Log.Error("updable to update check run", map[string]interface{}{ + "error": err.Error(), + }) + } + return + } + // iterate through all project results and the update the github check for _, projectResult := range res.ProjectResults { statusName := c.TitleBuilder.Build(cmd.CommandName().String(), vcs.StatusTitleOptions{ ProjectName: projectResult.ProjectName, }) + // Description is a required field + var description string + var state models.CommitStatus + if projectResult.Error != nil || projectResult.Failure != "" { + description = fmt.Sprintf("%s failed for %s", strings.Title(projectResult.Command.String()), projectResult.ProjectName) + state = models.FailedCommitStatus + } else { + description = fmt.Sprintf("%s succeeded for %s", strings.Title(projectResult.Command.String()), projectResult.ProjectName) + state = models.SuccessCommitStatus + } + + // TODO: Make the mark down rendered project specific output := c.MarkdownRenderer.Render(res, cmd.CommandName(), ctx.Pull.BaseRepo) updateStatusReq := types.UpdateStatusRequest{ Repo: ctx.HeadRepo, Ref: ctx.Pull.HeadCommit, StatusName: statusName, PullNum: ctx.Pull.Num, - Description: output, + Description: description, + Output: output, + State: state, } if err := c.VCSClient.UpdateStatus(ctx.RequestCtx, updateStatusReq); err != nil { diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index fc974caf57..deee2e30d7 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -32,6 +32,57 @@ import ( "github.com/shurcooL/githubv4" ) +// github checks conclusion +type ChecksConclusion int + +const ( + Neutral ChecksConclusion = iota + TimedOut + ActionRequired + Cancelled + Failure + Success +) + +func (e ChecksConclusion) String() string { + switch e { + case Neutral: + return "neutral" + case TimedOut: + return "timed_out" + case ActionRequired: + return "action_required" + case Cancelled: + return "cancelled" + case Failure: + return "failure" + case Success: + return "success" + } + return "" +} + +// github checks status +type CheckStatus int + +const ( + Queued CheckStatus = iota + InProgress + Completed +) + +func (e CheckStatus) String() string { + switch e { + case Queued: + return "queued" + case InProgress: + return "in_progress" + case Completed: + return "completed" + } + return "" +} + // maxCommentLength is the maximum number of chars allowed in a single comment // by GitHub. const ( @@ -299,7 +350,7 @@ func (g *GithubClient) PullIsMergeable(repo models.Repo, pull models.PullRequest return false, errors.Wrap(err, "getting commit statuses") } - checks, err := g.GetRepoChecks(repo, pull) + checks, err := g.GetRepoChecks(repo, pull.HeadCommit) if err != nil { return false, errors.Wrapf(err, "getting check runs") @@ -341,7 +392,7 @@ func (g *GithubClient) GetPullRequest(repo models.Repo, num int) (*github.PullRe return g.GetPullRequestFromName(repo.Name, repo.Owner, num) } -func (g *GithubClient) GetRepoChecks(repo models.Repo, pull models.PullRequest) ([]*github.CheckRun, error) { +func (g *GithubClient) GetRepoChecks(repo models.Repo, commitSHA string) ([]*github.CheckRun, error) { nextPage := 0 var results []*github.CheckRun @@ -357,7 +408,7 @@ func (g *GithubClient) GetRepoChecks(repo models.Repo, pull models.PullRequest) opts.Page = nextPage } - result, response, err := g.client.Checks.ListCheckRunsForRef(g.ctx, repo.Owner, repo.Name, pull.HeadCommit, opts) + result, response, err := g.client.Checks.ListCheckRunsForRef(g.ctx, repo.Owner, repo.Name, commitSHA, opts) if err != nil { return results, errors.Wrapf(err, "getting check runs for page %d", nextPage) @@ -429,17 +480,92 @@ func (g *GithubClient) UpdateStatus(ctx context.Context, request types.UpdateSta return err } +func (g *GithubClient) findCheckRun(statusName string, checkRuns []*github.CheckRun) *github.CheckRun { + for _, checkRun := range checkRuns { + if *checkRun.Name == statusName { + return checkRun + } + } + return nil +} + // [WENGINES-4643] TODO: Move the checks implementation to UpdateStatus once github checks is stable -// UpdateChecksStatus updates the status check func (g *GithubClient) UpdateChecksStatus(ctx context.Context, request types.UpdateStatusRequest) error { - // TODO: Implement updating github checks - // - Get all checkruns for this SHA - // - Match the UpdateReqIdentifier with the check run. If it exists, update the checkrun. If it does not, create a new check run. + checkRuns, err := g.GetRepoChecks(request.Repo, request.Ref) + if err != nil { + return err + } - // Checks uses Status and Conlusion. Need to map models.CommitStatus to Status and Conclusion - // Status -> queued, in_progress, completed - // Conclusion -> failure, neutral, cancelled, timed_out, or action_required. (Optional. Required if you provide a status of "completed".) - return nil + status, conclusion := g.resolveChecksStatus(request.State) + checkRunOutput := github.CheckRunOutput{ + Title: &request.StatusName, + Summary: &request.Description, + } + if request.Output != "" { + checkRunOutput.Text = &request.Output + } + + if checkRun := g.findCheckRun(request.StatusName, checkRuns); checkRun != nil { + updateCheckRunOpts := github.UpdateCheckRunOptions{ + Name: request.StatusName, + HeadSHA: &request.Ref, + Status: &status, + Output: &checkRunOutput, + } + + if request.DetailsURL != "" { + updateCheckRunOpts.DetailsURL = &request.DetailsURL + } + + // Conclusion is required if status is Completed + if status == Completed.String() { + updateCheckRunOpts.Conclusion = &conclusion + } + _, _, err := g.client.Checks.UpdateCheckRun(ctx, request.Repo.Owner, request.Repo.Name, *checkRun.ID, updateCheckRunOpts) + return err + } + + createCheckRunOpts := github.CreateCheckRunOptions{ + Name: request.StatusName, + HeadSHA: request.Ref, + Status: &status, + Output: &checkRunOutput, + } + + if request.DetailsURL != "" { + createCheckRunOpts.DetailsURL = &request.DetailsURL + } + + // Conclusion is required if status is Completed + if status == Completed.String() { + createCheckRunOpts.Conclusion = &conclusion + } + + _, _, err = g.client.Checks.CreateCheckRun(ctx, request.Repo.Owner, request.Repo.Name, createCheckRunOpts) + return err +} + +// Github Checks uses Status and Conclusion to report status of the check run. Need to map models.CommitStatus to Status and Conclusion +// Status -> queued, in_progress, completed +// Conclusion -> failure, neutral, cancelled, timed_out, or action_required. (Optional. Required if you provide a status of "completed".) +func (g *GithubClient) resolveChecksStatus(state models.CommitStatus) (string, string) { + status := Queued + conclusion := Neutral + + switch state { + case models.SuccessCommitStatus: + status = Completed + conclusion = Success + + case models.PendingCommitStatus: + status = InProgress + + case models.FailedCommitStatus: + status = Completed + conclusion = Failure + } + + return status.String(), conclusion.String() } // MarkdownPullLink specifies the string used in a pull request comment to reference another pull request. diff --git a/server/events/vcs/github_client_test.go b/server/events/vcs/github_client_test.go index a17fb929ae..355f3c329e 100644 --- a/server/events/vcs/github_client_test.go +++ b/server/events/vcs/github_client_test.go @@ -17,6 +17,7 @@ import ( "github.com/runatlantis/atlantis/server/events/vcs/types" "github.com/runatlantis/atlantis/server/logging" . "github.com/runatlantis/atlantis/testing" + "github.com/stretchr/testify/assert" "golang.org/x/net/context" "github.com/shurcooL/githubv4" @@ -332,6 +333,235 @@ func TestGithubClient_HideOldComments(t *testing.T) { Equals(t, githubv4.ReportedContentClassifiersOutdated, gotMinimizeCalls[0].Variables.Input.Classifier) } +func TestGithubClient_UpdateChecksStatus(t *testing.T) { + + listCheckRunRespFormat := ` + { + "total_count": 1, + "check_runs": [ + { + "id": 1, + "head_sha": "ce587453ced02b1526dfb4cb910479d431683101", + "status": "completed", + "conclusion": "neutral", + "started_at": "2018-05-04T01:14:52Z", + "completed_at": "2018-05-04T01:14:52Z", + "name": "%s", + "check_suite": { + "id": 5 + } + } + ] + } + ` + + cases := []struct { + name string + newCheckRunName string + existingCheckRunName string + listCheckRunResp string + }{ + { + name: "create new check run when check run dne", + newCheckRunName: "atlantis/apply", + listCheckRunResp: fmt.Sprintf(listCheckRunRespFormat, "atlantis/plan"), + }, + { + name: "update check run when check run exists", + existingCheckRunName: "atlantis/apply", + listCheckRunResp: fmt.Sprintf(listCheckRunRespFormat, "atlantis/apply"), + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/v3/repos/owner/repo/commits/sha/check-runs?per_page=100": + _, err := w.Write([]byte(c.listCheckRunResp)) + Ok(t, err) + + case "/api/v3/repos/owner/repo/check-runs": + // parse req and assert CreateNewCheckRun was called for new check run + body, err := ioutil.ReadAll(r.Body) + Ok(t, err) + m := make(map[string]interface{}) + err = json.Unmarshal(body, &m) + Ok(t, err) + assert.Equal(t, c.newCheckRunName, m["name"]) + case "/api/v3/repos/owner/repo/check-runs/1": + // parse req and assert UpdateCheckRun was called for existing check run + body, err := ioutil.ReadAll(r.Body) + Ok(t, err) + m := make(map[string]interface{}) + err = json.Unmarshal(body, &m) + Ok(t, err) + assert.Equal(t, c.existingCheckRunName, m["name"]) + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + mergeabilityChecker := vcs.NewPullMergeabilityChecker("atlantis") + client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, logging.NewNoopCtxLogger(t), mergeabilityChecker) + Ok(t, err) + defer disableSSLVerification()() + + req := types.UpdateStatusRequest{ + Repo: models.Repo{ + FullName: "owner/repo", + Owner: "owner", + Name: "repo", + CloneURL: "", + SanitizedCloneURL: "", + VCSHost: models.VCSHost{ + Type: models.Github, + Hostname: "github.com", + }, + }, + PullNum: 1, + State: models.PendingCommitStatus, + Description: "description", + DetailsURL: "https://google.com", + Ref: "sha", + } + + if c.newCheckRunName != "" { + req.StatusName = c.newCheckRunName + } else { + req.StatusName = c.existingCheckRunName + } + + err = client.UpdateChecksStatus(context.TODO(), req) + Ok(t, err) + }) + } +} + +func TestGithubClient_UpdateChecksStatus_ConclusionWhenStatusComplete(t *testing.T) { + checkRunName := "atlantis/apply" + listCheckRunResp := ` + { + "total_count": 1, + "check_runs": [ + { + "id": 1, + "head_sha": "ce587453ced02b1526dfb4cb910479d431683101", + "status": "completed", + "conclusion": "neutral", + "started_at": "2018-05-04T01:14:52Z", + "completed_at": "2018-05-04T01:14:52Z", + "name": "%s", + "check_suite": { + "id": 5 + } + } + ] + } + ` + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/v3/repos/owner/repo/commits/sha/check-runs?per_page=100": + _, err := w.Write([]byte(fmt.Sprintf(listCheckRunResp, checkRunName))) + Ok(t, err) + case "/api/v3/repos/owner/repo/check-runs/1": + body, err := ioutil.ReadAll(r.Body) + Ok(t, err) + m := make(map[string]interface{}) + err = json.Unmarshal(body, &m) + Ok(t, err) + + // assert conclusion was set to success when status is complete + assert.Equal(t, checkRunName, m["name"]) + assert.Equal(t, "success", m["conclusion"]) + + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + mergeabilityChecker := vcs.NewPullMergeabilityChecker("atlantis") + client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, logging.NewNoopCtxLogger(t), mergeabilityChecker) + Ok(t, err) + defer disableSSLVerification()() + + err = client.UpdateChecksStatus(context.TODO(), types.UpdateStatusRequest{ + Repo: models.Repo{ + FullName: "owner/repo", + Owner: "owner", + Name: "repo", + CloneURL: "", + SanitizedCloneURL: "", + VCSHost: models.VCSHost{ + Type: models.Github, + Hostname: "github.com", + }, + }, + PullNum: 1, + State: models.SuccessCommitStatus, + StatusName: checkRunName, + Description: "description", + DetailsURL: "https://google.com", + Ref: "sha", + }) + Ok(t, err) + +} + +func TestGithubClient_UpdateChecksStatus_ErrorWhenListCheckRunsFails(t *testing.T) { + listCheckRunResp := `error response` + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/v3/repos/owner/repo/commits/sha/check-runs?per_page=100": + _, err := w.Write([]byte(listCheckRunResp)) + Ok(t, err) + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + mergeabilityChecker := vcs.NewPullMergeabilityChecker("atlantis") + client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, logging.NewNoopCtxLogger(t), mergeabilityChecker) + Ok(t, err) + defer disableSSLVerification()() + + err = client.UpdateChecksStatus(context.TODO(), types.UpdateStatusRequest{ + Repo: models.Repo{ + FullName: "owner/repo", + Owner: "owner", + Name: "repo", + CloneURL: "", + SanitizedCloneURL: "", + VCSHost: models.VCSHost{ + Type: models.Github, + Hostname: "github.com", + }, + }, + PullNum: 1, + State: models.PendingCommitStatus, + StatusName: "atlantis/plan", + Description: "description", + DetailsURL: "https://google.com", + Ref: "sha", + }) + assert.Error(t, err) +} + func TestGithubClient_UpdateStatus(t *testing.T) { cases := []struct { status models.CommitStatus diff --git a/server/events/vcs/instrumented_client.go b/server/events/vcs/instrumented_client.go index 70cddb8408..450db2e37e 100644 --- a/server/events/vcs/instrumented_client.go +++ b/server/events/vcs/instrumented_client.go @@ -127,7 +127,7 @@ func (c *InstrumentedGithubClient) GetRepoChecks(repo models.Repo, pull models.P executionSuccess := scope.Counter(metrics.ExecutionSuccessMetric) executionError := scope.Counter(metrics.ExecutionErrorMetric) - statuses, err := c.GhClient.GetRepoChecks(repo, pull) + statuses, err := c.GhClient.GetRepoChecks(repo, pull.HeadCommit) if err != nil { executionError.Inc(1) diff --git a/server/events/vcs/types/status.go b/server/events/vcs/types/status.go index 566a92fe1a..97f5dbea3a 100644 --- a/server/events/vcs/types/status.go +++ b/server/events/vcs/types/status.go @@ -11,4 +11,5 @@ type UpdateStatusRequest struct { StatusName string Description string DetailsURL string + Output string }