From 5f4d93ec15f8c5589b6a692b6be0b9e20459faa4 Mon Sep 17 00:00:00 2001 From: Andrew Fitzpatrick Date: Wed, 24 Jul 2024 22:22:07 +0100 Subject: [PATCH] fix: dont set gitlab status if new commit has been pushed, also send the pipeline id in the request rather than ref (#6) --- server/events/vcs/gitlab_client.go | 20 +++++++++++++++----- server/events/vcs/gitlab_client_test.go | 18 +++++++++--------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 16ffa38421..10ed28aa6b 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -22,14 +22,14 @@ import ( "strings" "time" - version "github.com/hashicorp/go-version" + "github.com/hashicorp/go-version" "github.com/jpillora/backoff" "github.com/pkg/errors" "github.com/runatlantis/atlantis/server/events/command" "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/events/vcs/common" "github.com/runatlantis/atlantis/server/logging" - gitlab "github.com/xanzy/go-gitlab" + "github.com/xanzy/go-gitlab" ) // gitlabMaxCommentLength is the maximum number of chars allowed by Gitlab in a @@ -410,7 +410,8 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re // of the MR. This is needed because the commit status is only shown in the MR if the pipeline is // assigned to an MR reference. // Try to get the MR details a couple of times in case the pipeline is not yet assigned to the MR - refTarget := pull.HeadBranch + var refTarget *string + var pipelineID *int retries := 1 delay := 2 * time.Second @@ -425,7 +426,12 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re 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) - refTarget = mr.HeadPipeline.Ref + // set pipeline ID for the req once found + pipelineID = gitlab.Ptr(mr.HeadPipeline.ID) + // if these are different then there is already a new commit so let's stop setting any statuses and return + if mr.HeadPipeline.SHA != pull.HeadCommit { + return errors.Errorf("mr.HeadPipeline.SHA: '%s' does not match pull.HeadCommit '%s'", mr.HeadPipeline.SHA, pull.HeadCommit) + } break } if i != retries { @@ -433,6 +439,8 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re pull.Num, 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) } @@ -461,7 +469,9 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re Context: gitlab.Ptr(src), Description: gitlab.Ptr(description), TargetURL: &url, - Ref: gitlab.Ptr(refTarget), + // only one of these should get sent in the request + PipelineID: pipelineID, + Ref: refTarget, }) if resp != nil { diff --git a/server/events/vcs/gitlab_client_test.go b/server/events/vcs/gitlab_client_test.go index a32a75d74a..a0edf077ce 100644 --- a/server/events/vcs/gitlab_client_test.go +++ b/server/events/vcs/gitlab_client_test.go @@ -12,11 +12,11 @@ import ( "testing" "time" - version "github.com/hashicorp/go-version" + "github.com/hashicorp/go-version" "github.com/runatlantis/atlantis/server/events/command" "github.com/runatlantis/atlantis/server/events/models" "github.com/runatlantis/atlantis/server/logging" - gitlab "github.com/xanzy/go-gitlab" + "github.com/xanzy/go-gitlab" . "github.com/runatlantis/atlantis/testing" ) @@ -297,12 +297,12 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { testServer := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { - case "/api/v4/projects/runatlantis%2Fatlantis/statuses/sha": + case "/api/v4/projects/runatlantis%2Fatlantis/statuses/67cb91d3f6198189f433c045154a885784ba6977": gotRequest = true body, err := io.ReadAll(r.Body) Ok(t, err) - exp := fmt.Sprintf(`{"state":"%s","ref":"patch-1-merger","context":"src","target_url":"https://google.com","description":"description"}`, c.expState) + exp := fmt.Sprintf(`{"state":"%s","context":"src","target_url":"https://google.com","description":"description","pipeline_id":488598}`, c.expState) Equals(t, exp, string(body)) defer r.Body.Close() // nolint: errcheck w.Write([]byte("{}")) // nolint: errcheck @@ -336,7 +336,7 @@ func TestGitlabClient_UpdateStatus(t *testing.T) { models.PullRequest{ Num: 1, BaseRepo: repo, - HeadCommit: "sha", + HeadCommit: "67cb91d3f6198189f433c045154a885784ba6977", HeadBranch: "test", }, c.status, "src", "description", "https://google.com") Ok(t, err) @@ -387,13 +387,13 @@ func TestGitlabClient_UpdateStatusRetryable(t *testing.T) { testServer := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.RequestURI { - case "/api/v4/projects/runatlantis%2Fatlantis/statuses/sha": + case "/api/v4/projects/runatlantis%2Fatlantis/statuses/67cb91d3f6198189f433c045154a885784ba6977": handledNumberOfRequests++ shouldSendConflict := handledNumberOfRequests <= c.numberOfConflicts body, err := io.ReadAll(r.Body) Ok(t, err) - exp := fmt.Sprintf(`{"state":"%s","ref":"patch-1-merger","context":"src","target_url":"https://google.com","description":"description"}`, c.expState) + exp := fmt.Sprintf(`{"state":"%s","context":"src","target_url":"https://google.com","description":"description","pipeline_id":488598}`, c.expState) Equals(t, exp, string(body)) defer r.Body.Close() // nolint: errcheck @@ -436,12 +436,12 @@ func TestGitlabClient_UpdateStatusRetryable(t *testing.T) { models.PullRequest{ Num: 1, BaseRepo: repo, - HeadCommit: "sha", + HeadCommit: "67cb91d3f6198189f433c045154a885784ba6977", HeadBranch: "test", }, c.status, "src", "description", "https://google.com") if c.expError { - ErrContains(t, "failed to update commit status for 'runatlantis/atlantis' @ 'sha' to 'src' after 10 attempts", err) + ErrContains(t, "failed to update commit status for 'runatlantis/atlantis' @ '67cb91d3f6198189f433c045154a885784ba6977' to 'src' after 10 attempts", err) ErrContains(t, "409", err) } else { Ok(t, err)