diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index b78c82b271..f53e799c59 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -395,7 +395,6 @@ func (g *GitlabClient) SupportsDetailedMergeStatus(logger logging.SimpleLogging) // UpdateStatus updates the build status of a commit. func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error { - logger.Debug("Updating GitLab commit status for '%s' to '%s'", src, state) gitlabState := gitlab.Pending switch state { case models.PendingCommitStatus: @@ -406,43 +405,47 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re gitlabState = gitlab.Success } - // refTarget is only set to the head branch of the MR if HeadPipeline is not found - // when HeadPipeline is found we set the pipelineID for the request instead - var refTarget *string - var pipelineID *int + logger.Info("Updating GitLab commit status for '%s' to '%s'", src, gitlabState) + + setCommitStatusOptions := &gitlab.SetCommitStatusOptions{ + State: gitlabState, + Context: gitlab.Ptr(src), + Description: gitlab.Ptr(description), + TargetURL: &url, + } retries := 1 delay := 2 * time.Second - var mr *gitlab.MergeRequest + var commit *gitlab.Commit + var resp *gitlab.Response var err error - // Try to get the MR details a couple of times in case the pipeline is not yet assigned to the MR + // Try a couple of times to get the pipeline ID for the commit for i := 0; i <= retries; i++ { - mr, err = g.GetMergeRequest(logger, pull.BaseRepo.FullName, pull.Num) + commit, resp, err = g.Client.Commits.GetCommit(repo.FullName, pull.HeadCommit, nil) + if resp != nil { + logger.Debug("GET /projects/%s/repository/commits/%d: %d", pull.BaseRepo.ID(), pull.HeadCommit, resp.StatusCode) + } if err != nil { return err } - if mr.HeadPipeline != nil { - logger.Debug("Head pipeline found for merge request %d, source '%s'. refTarget '%s'", - pull.Num, mr.HeadPipeline.Source, mr.HeadPipeline.Ref) - // set pipeline ID for the req once found - pipelineID = gitlab.Ptr(mr.HeadPipeline.ID) + if commit.LastPipeline != nil { + logger.Info("Pipeline found for commit %s, setting pipeline ID to %d", pull.HeadCommit, commit.LastPipeline.ID) + // Set the pipeline ID to the last pipeline that ran for the commit + setCommitStatusOptions.PipelineID = gitlab.Ptr(commit.LastPipeline.ID) break } if i != retries { - logger.Debug("Head pipeline not found for merge request %d. Retrying in %s", - pull.Num, delay) + logger.Info("No pipeline found for commit %s, retrying in %s", pull.HeadCommit, delay) time.Sleep(delay) } else { - // set the ref target here if the pipeline wasn't found - refTarget = gitlab.Ptr(pull.HeadBranch) - logger.Debug("Head pipeline not found for merge request %d.", - pull.Num) + // If we've exhausted all retries, set the Ref to the branch name + logger.Info("No pipeline found for commit %s, setting Ref to %s", pull.HeadCommit, pull.HeadBranch) + setCommitStatusOptions.Ref = gitlab.Ptr(pull.HeadBranch) } } var ( - resp *gitlab.Response maxAttempts = 10 retryer = &backoff.Backoff{ Jitter: true, @@ -455,19 +458,11 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re "attempt", i+1, "max_attempts", maxAttempts, "repo", repo.FullName, - "commit", pull.HeadCommit, + "commit", commit.ShortID, "state", state.String(), ) - _, resp, err = g.Client.Commits.SetCommitStatus(repo.FullName, pull.HeadCommit, &gitlab.SetCommitStatusOptions{ - State: gitlabState, - Context: gitlab.Ptr(src), - Description: gitlab.Ptr(description), - TargetURL: &url, - // only one of these should get sent in the request - PipelineID: pipelineID, - Ref: refTarget, - }) + _, resp, err = g.Client.Commits.SetCommitStatus(repo.FullName, pull.HeadCommit, setCommitStatusOptions) if resp != nil { logger.Debug("POST /projects/%s/statuses/%s returned: %d", repo.FullName, pull.HeadCommit, resp.StatusCode) diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index 98de58287f..8aee1e865a 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -3,7 +3,6 @@ package vcs import ( "encoding/json" "fmt" - "io" "net/http" "net/http/httptest" "os" @@ -25,10 +24,39 @@ var projectID = 4580910 const gitlabPipelineSuccessMrID = 488598 +const updateStatusDescription = "description" +const updateStatusTargetUrl = "https://google.com" +const updateStatusSrc = "src" +const updateStatusHeadBranch = "test" + +/* UpdateStatus request JSON body object */ +type UpdateStatusJsonBody struct { + State string `json:"state"` + Context string `json:"context"` + TargetUrl string `json:"target_url"` + Description string `json:"description"` + PipelineId int `json:"pipeline_id"` + Ref string `json:"ref"` +} + +/* GetCommit response last_pipeline JSON object */ +type GetCommitResponseLastPipeline struct { + ID int `json:"id"` +} + +/* GetCommit response JSON object */ +type GetCommitResponse struct { + LastPipeline GetCommitResponseLastPipeline `json:"last_pipeline"` +} + +/* Empty struct for JSON marshalling */ +type EmptyStruct struct{} + // Test that the base url gets set properly. func TestNewGitlabClient_BaseURL(t *testing.T) { gitlabClientUnderTest = true defer func() { gitlabClientUnderTest = false }() + cases := []struct { Hostname string ExpBaseURL string @@ -273,8 +301,6 @@ func TestGitlabClient_MergePull(t *testing.T) { func TestGitlabClient_UpdateStatus(t *testing.T) { logger := logging.NewNoopLogger(t) - pipelineSuccess, err := os.ReadFile("testdata/gitlab-pipeline-success.json") - Ok(t, err) cases := []struct { status models.CommitStatus @@ -302,18 +328,42 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { case "/api/v4/projects/runatlantis%2Fatlantis/statuses/sha": gotRequest = true - body, err := io.ReadAll(r.Body) + var updateStatusJsonBody UpdateStatusJsonBody + err := json.NewDecoder(r.Body).Decode(&updateStatusJsonBody) Ok(t, err) - exp := fmt.Sprintf(`{"state":"%s","context":"src","target_url":"https://google.com","description":"description","pipeline_id":%d}`, c.expState, gitlabPipelineSuccessMrID) - Equals(t, exp, string(body)) - defer r.Body.Close() // nolint: errcheck - w.Write([]byte("{}")) // nolint: errcheck - case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/1": + + Equals(t, c.expState, updateStatusJsonBody.State) + Equals(t, updateStatusSrc, updateStatusJsonBody.Context) + Equals(t, updateStatusTargetUrl, updateStatusJsonBody.TargetUrl) + Equals(t, updateStatusDescription, updateStatusJsonBody.Description) + Equals(t, gitlabPipelineSuccessMrID, updateStatusJsonBody.PipelineId) + + defer r.Body.Close() // nolint: errcheck + + setStatusJsonResponse, err := json.Marshal(EmptyStruct{}) + Ok(t, err) + + _, err = w.Write(setStatusJsonResponse) + Ok(t, err) + + case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha": w.WriteHeader(http.StatusOK) - w.Write(pipelineSuccess) // nolint: errcheck + + getCommitResponse := GetCommitResponse{ + LastPipeline: GetCommitResponseLastPipeline{ + ID: gitlabPipelineSuccessMrID, + }, + } + getCommitJsonResponse, err := json.Marshal(getCommitResponse) + Ok(t, err) + + _, err = w.Write(getCommitJsonResponse) + Ok(t, err) + case "/api/v4/": // Rate limiter requests. w.WriteHeader(http.StatusOK) + default: t.Errorf("got unexpected request at %q", r.RequestURI) http.Error(w, "not found", http.StatusNotFound) @@ -339,18 +389,158 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { Num: 1, BaseRepo: repo, HeadCommit: "sha", - HeadBranch: "test", - }, c.status, "src", "description", "https://google.com") + HeadBranch: updateStatusHeadBranch, + }, + c.status, + updateStatusSrc, + updateStatusDescription, + updateStatusTargetUrl, + ) Ok(t, err) Assert(t, gotRequest, "expected to get the request") }) } } -func TestGitlabClient_UpdateStatusRetryable(t *testing.T) { +func TestGitlabClient_UpdateStatusGetCommitRetryable(t *testing.T) { + logger := logging.NewNoopLogger(t) + + cases := []struct { + title string + status models.CommitStatus + commitsWithNoLastPipeline int + expNumberOfRequests int + expRefOrPipelineId string + }{ + // Ensure that GetCommit with last pipeline id sets the pipeline id. + { + title: "GetCommit with a pipeline id", + status: models.PendingCommitStatus, + commitsWithNoLastPipeline: 0, + expNumberOfRequests: 1, + expRefOrPipelineId: "PipelineId", + }, + // Ensure that 1 x GetCommit with no pipelines sets the pipeline id. + { + title: "1 x GetCommit with no last pipeline id", + status: models.PendingCommitStatus, + commitsWithNoLastPipeline: 1, + expNumberOfRequests: 2, + expRefOrPipelineId: "PipelineId", + }, + // Ensure that 2 x GetCommit with no last pipeline id sets the ref. + { + title: "2 x GetCommit with no last pipeline id", + status: models.PendingCommitStatus, + commitsWithNoLastPipeline: 2, + expNumberOfRequests: 2, + expRefOrPipelineId: "Ref", + }, + } + + for _, c := range cases { + t.Run(c.title, func(t *testing.T) { + handledNumberOfRequests := 0 + + testServer := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/v4/projects/runatlantis%2Fatlantis/statuses/sha": + var updateStatusJsonBody UpdateStatusJsonBody + err := json.NewDecoder(r.Body).Decode(&updateStatusJsonBody) + Ok(t, err) + + Equals(t, "running", updateStatusJsonBody.State) + Equals(t, updateStatusSrc, updateStatusJsonBody.Context) + Equals(t, updateStatusTargetUrl, updateStatusJsonBody.TargetUrl) + Equals(t, updateStatusDescription, updateStatusJsonBody.Description) + if c.expRefOrPipelineId == "Ref" { + Equals(t, updateStatusHeadBranch, updateStatusJsonBody.Ref) + } else { + Equals(t, gitlabPipelineSuccessMrID, updateStatusJsonBody.PipelineId) + } + + defer r.Body.Close() + + getCommitJsonResponse, err := json.Marshal(EmptyStruct{}) + Ok(t, err) + + _, err = w.Write(getCommitJsonResponse) + Ok(t, err) + + case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha": + handledNumberOfRequests++ + noCommitLastPipeline := handledNumberOfRequests <= c.commitsWithNoLastPipeline + + w.WriteHeader(http.StatusOK) + if noCommitLastPipeline { + getCommitJsonResponse, err := json.Marshal(EmptyStruct{}) + Ok(t, err) + + _, err = w.Write(getCommitJsonResponse) + Ok(t, err) + } else { + getCommitResponse := GetCommitResponse{ + LastPipeline: GetCommitResponseLastPipeline{ + ID: gitlabPipelineSuccessMrID, + }, + } + getCommitJsonResponse, err := json.Marshal(getCommitResponse) + Ok(t, err) + + _, err = w.Write(getCommitJsonResponse) + Ok(t, err) + } + + case "/api/v4/": + // Rate limiter requests. + w.WriteHeader(http.StatusOK) + + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + } + })) + + internalClient, err := gitlab.NewClient("token", gitlab.WithBaseURL(testServer.URL)) + Ok(t, err) + + client := &GitlabClient{ + Client: internalClient, + Version: nil, + PollingInterval: 10 * time.Millisecond, + } + + repo := models.Repo{ + FullName: "runatlantis/atlantis", + Owner: "runatlantis", + Name: "atlantis", + } + + err = client.UpdateStatus( + logger, + repo, + models.PullRequest{ + Num: 1, + BaseRepo: repo, + HeadCommit: "sha", + HeadBranch: updateStatusHeadBranch, + }, + c.status, + updateStatusSrc, + updateStatusDescription, + updateStatusTargetUrl, + ) + Ok(t, err) + + Assert(t, c.expNumberOfRequests == handledNumberOfRequests, + fmt.Sprintf("expected %d number of requests, but processed %d", c.expNumberOfRequests, handledNumberOfRequests)) + }) + } +} + +func TestGitlabClient_UpdateStatusSetCommitStatusConflictRetryable(t *testing.T) { logger := logging.NewNoopLogger(t) - pipelineSuccess, err := os.ReadFile("testdata/gitlab-pipeline-success.json") - Ok(t, err) cases := []struct { status models.CommitStatus @@ -393,21 +583,40 @@ func TestGitlabClient_UpdateStatusRetryable(t *testing.T) { handledNumberOfRequests++ shouldSendConflict := handledNumberOfRequests <= c.numberOfConflicts - body, err := io.ReadAll(r.Body) + var updateStatusJsonBody UpdateStatusJsonBody + err := json.NewDecoder(r.Body).Decode(&updateStatusJsonBody) Ok(t, err) - exp := fmt.Sprintf(`{"state":"%s","context":"src","target_url":"https://google.com","description":"description","pipeline_id":%d}`, c.expState, gitlabPipelineSuccessMrID) - Equals(t, exp, string(body)) + + Equals(t, c.expState, updateStatusJsonBody.State) + Equals(t, updateStatusSrc, updateStatusJsonBody.Context) + Equals(t, updateStatusTargetUrl, updateStatusJsonBody.TargetUrl) + Equals(t, updateStatusDescription, updateStatusJsonBody.Description) + defer r.Body.Close() // nolint: errcheck if shouldSendConflict { w.WriteHeader(http.StatusConflict) } - w.Write([]byte("{}")) // nolint: errcheck + getCommitJsonResponse, err := json.Marshal(EmptyStruct{}) + Ok(t, err) + + _, err = w.Write(getCommitJsonResponse) + Ok(t, err) - case "/api/v4/projects/runatlantis%2Fatlantis/merge_requests/1": + case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha": w.WriteHeader(http.StatusOK) - w.Write(pipelineSuccess) // nolint: errcheck + + getCommitResponse := GetCommitResponse{ + LastPipeline: GetCommitResponseLastPipeline{ + ID: gitlabPipelineSuccessMrID, + }, + } + getCommitJsonResponse, err := json.Marshal(getCommitResponse) + Ok(t, err) + + _, err = w.Write(getCommitJsonResponse) + Ok(t, err) case "/api/v4/": // Rate limiter requests. @@ -440,7 +649,12 @@ func TestGitlabClient_UpdateStatusRetryable(t *testing.T) { BaseRepo: repo, HeadCommit: "sha", HeadBranch: "test", - }, c.status, "src", "description", "https://google.com") + }, + c.status, + updateStatusSrc, + updateStatusDescription, + updateStatusTargetUrl, + ) if c.expError { ErrContains(t, "failed to update commit status for 'runatlantis/atlantis' @ 'sha' to 'src' after 10 attempts", err) @@ -449,7 +663,8 @@ func TestGitlabClient_UpdateStatusRetryable(t *testing.T) { Ok(t, err) } - Assert(t, c.expNumberOfRequests == handledNumberOfRequests, fmt.Sprintf("expected %d number of requests, but processed %d", c.expNumberOfRequests, handledNumberOfRequests)) + Assert(t, c.expNumberOfRequests == handledNumberOfRequests, + fmt.Sprintf("expected %d number of requests, but processed %d", c.expNumberOfRequests, handledNumberOfRequests)) }) } }