Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: gitlab - ensure lastpipeline ref is the same as head branch #5259

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+163 −35
Open

feat: gitlab - ensure lastpipeline ref is the same as head branch #5259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
35fb58e
gitlab - ensure lastpipeline ref is the same as head branch
rhariady Jan 21, 2025
a54a04f
gofmt
rhariady Jan 21, 2025
e581a4d
only check for commit status if LastPipelineID is differ
rhariady Jan 22, 2025
8594e35
use Gitlab get commit statuses API to get previous pipeline
rhariady Jan 22, 2025
cb4f07d
add more info in log when pipeline is found or not found
rhariady Jan 22, 2025
b47ae9a
check that all commit statuses have the same pipeline Id
rhariady Feb 18, 2025
dce43a6
Merge branch 'main' into fix-5228-gitlab-pipeline-status
X-Guardian Feb 21, 2025
83090b2
update test (remove per-page param)
rhariady Feb 21, 2025
2f08e70
fix formatting in test file
rhariady Feb 21, 2025
db357fe
fix goimport error
rhariady Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 38 additions & 9 deletions server/events/vcs/gitlab_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"net"
"net/http"
"net/url"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -419,27 +420,55 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re

retries := 1
delay := 2 * time.Second
var commit *gitlab.Commit
var commitStatuses []*gitlab.CommitStatus
var resp *gitlab.Response
var err error

// get the last commit status with the same ref
getCommitStatusesOptions := &gitlab.GetCommitStatusesOptions{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have better handling on this if the function returns multiple results? Forcing the PerPage to 1 is pretty ugly.

If we are not expecting more than one result, we should be checking the length and making a decision on what we want to do in that scenario. i.e. using the first result, but at least report a warning that more than one result was returned, or just erroring.

Copy link
Author

@rhariady rhariady Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @X-Guardian! Sorry i just got back now after a while.

It's actually very possible that the response contain multiple results, in case the pipeline contain multiple job, but all should have identical PipelineID because for each commit there should only one pipeline per ref (gitlab will screaming error when creating new pipeline with same ref for single commit). So, in the end it doesn't matter which job (element) of the list to use.

On contrary, in existing implementation, we sometime mixed between pipeline from different ref. By filtering with specific ref, we can prevent this mixing from happened.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if this is the case, rather than making the assumption, the code should verify that.

Copy link
Author

@rhariady rhariady Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @X-Guardian , i have some more updates to accommodate your concerns. It will check first that all the commit status has same pipeline ID, and use that value for as pipeline ID for new status. Otherwise, it will use the pipeline ID from latest commit status (first in the response list).

ListOptions: gitlab.ListOptions{
Sort: "desc",
},
Ref: gitlab.Ptr(pull.HeadBranch),
}

// Try a couple of times to get the pipeline ID for the commit
for i := 0; i <= retries; i++ {
commit, resp, err = g.Client.Commits.GetCommit(repo.FullName, pull.HeadCommit, nil)
commitStatuses, resp, err = g.Client.Commits.GetCommitStatuses(repo.FullName, pull.HeadCommit, getCommitStatusesOptions)

if resp != nil {
logger.Debug("GET /projects/%s/repository/commits/%d: %d", pull.BaseRepo.ID(), pull.HeadCommit, resp.StatusCode)
logger.Debug("GET /projects/%s/repository/commits/%d/statuses: %d", pull.BaseRepo.ID(), pull.HeadCommit, resp.StatusCode)
}
if err != nil {
return err
}
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)
if len(commitStatuses) > 0 {
var pipelineIds []int

// Loop through the commitStatuses and collect unique pipelineId
for _, commitStatus := range commitStatuses {
if !(slices.Contains(pipelineIds, commitStatus.PipelineId)) {
pipelineIds = append(pipelineIds, commitStatus.PipelineId)
}
}

// Check that all commit statuses has identical pipeline ID reflected by only one item in pipelineIds
if len(pipelineIds) == 1 {
logger.Info("Exactly one PipelineID found for commit %s and ref %s, setting new status PipelineID to %d", pull.HeadCommit, pull.HeadBranch, pipelineIds[0])

// Set the pipeline ID to the only item in pipelineIds
setCommitStatusOptions.PipelineID = gitlab.Ptr(pipelineIds[0])
} else {
logger.Warn("Commit %s has statuses from more than one PipelineID (ids=%v) for ref %s. Set PipelineID from the last commit status: %s", pull.HeadCommit, pipelineIds, pull.HeadBranch, commitStatuses[0].PipelineId)

// Set the pipeline ID from the latest commit status
setCommitStatusOptions.PipelineID = gitlab.Ptr(commitStatuses[0].PipelineId)
}

break
}
if i != retries {
logger.Info("No pipeline found for commit %s, retrying in %s", pull.HeadCommit, delay)
logger.Info("No pipeline found for commit %s and ref %s, retrying in %s", pull.HeadCommit, pull.HeadBranch, delay)
time.Sleep(delay)
} else {
// If we've exhausted all retries, set the Ref to the branch name
Expand All @@ -462,7 +491,7 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re
"attempt", attempt,
"max_attempts", maxAttempts,
"repo", repo.FullName,
"commit", commit.ShortID,
"commit", pull.HeadCommit,
"state", state.String(),
)

Expand Down
151 changes: 125 additions & 26 deletions server/events/vcs/gitlab_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,13 @@ type UpdateStatusJsonBody struct {
Ref string `json:"ref"`
}

/* GetCommit response last_pipeline JSON object */
type GetCommitResponseLastPipeline struct {
ID int `json:"id"`
type CommitStatus struct {
Ref string `json:"ref"`
PipelineID int `json:"pipeline_id"`
}

/* GetCommit response JSON object */
type GetCommitResponse struct {
LastPipeline GetCommitResponseLastPipeline `json:"last_pipeline"`
}
/* GetCommitStatuses response JSON object */
type GetCommitStatusesResponse []CommitStatus

/* Empty struct for JSON marshalling */
type EmptyStruct struct{}
Expand Down Expand Up @@ -346,18 +344,19 @@ func TestGitlabClient_UpdateStatus(t *testing.T) {
_, err = w.Write(setStatusJsonResponse)
Ok(t, err)

case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha":
case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha/statuses?ref=test&sort=desc":
w.WriteHeader(http.StatusOK)

getCommitResponse := GetCommitResponse{
LastPipeline: GetCommitResponseLastPipeline{
ID: gitlabPipelineSuccessMrID,
getCommitStatusesResponse := GetCommitStatusesResponse{
CommitStatus{
Ref: updateStatusHeadBranch,
PipelineID: gitlabPipelineSuccessMrID,
},
}
getCommitJsonResponse, err := json.Marshal(getCommitResponse)
getCommitStatusesJsonResponse, err := json.Marshal(getCommitStatusesResponse)
Ok(t, err)

_, err = w.Write(getCommitJsonResponse)
_, err = w.Write(getCommitStatusesJsonResponse)
Ok(t, err)

case "/api/v4/":
Expand Down Expand Up @@ -468,24 +467,25 @@ func TestGitlabClient_UpdateStatusGetCommitRetryable(t *testing.T) {
_, err = w.Write(getCommitJsonResponse)
Ok(t, err)

case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha":
case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha/statuses?ref=test&sort=desc":
handledNumberOfRequests++
noCommitLastPipeline := handledNumberOfRequests <= c.commitsWithNoLastPipeline

w.WriteHeader(http.StatusOK)
if noCommitLastPipeline {
getCommitJsonResponse, err := json.Marshal(EmptyStruct{})
getCommitStatusesJsonResponse, err := json.Marshal([]EmptyStruct{})
Ok(t, err)

_, err = w.Write(getCommitJsonResponse)
_, err = w.Write(getCommitStatusesJsonResponse)
Ok(t, err)
} else {
getCommitResponse := GetCommitResponse{
LastPipeline: GetCommitResponseLastPipeline{
ID: gitlabPipelineSuccessMrID,
getCommitStatusesResponse := GetCommitStatusesResponse{
CommitStatus{
Ref: updateStatusHeadBranch,
PipelineID: gitlabPipelineSuccessMrID,
},
}
getCommitJsonResponse, err := json.Marshal(getCommitResponse)
getCommitJsonResponse, err := json.Marshal(getCommitStatusesResponse)
Ok(t, err)

_, err = w.Write(getCommitJsonResponse)
Expand Down Expand Up @@ -604,18 +604,19 @@ func TestGitlabClient_UpdateStatusSetCommitStatusConflictRetryable(t *testing.T)
_, err = w.Write(getCommitJsonResponse)
Ok(t, err)

case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha":
case "/api/v4/projects/runatlantis%2Fatlantis/repository/commits/sha/statuses?ref=test&sort=desc":
w.WriteHeader(http.StatusOK)

getCommitResponse := GetCommitResponse{
LastPipeline: GetCommitResponseLastPipeline{
ID: gitlabPipelineSuccessMrID,
getCommitStatusesResponse := GetCommitStatusesResponse{
CommitStatus{
Ref: updateStatusHeadBranch,
PipelineID: gitlabPipelineSuccessMrID,
},
}
getCommitJsonResponse, err := json.Marshal(getCommitResponse)
getCommitStatusesJsonResponse, err := json.Marshal(getCommitStatusesResponse)
Ok(t, err)

_, err = w.Write(getCommitJsonResponse)
_, err = w.Write(getCommitStatusesJsonResponse)
Ok(t, err)

case "/api/v4/":
Expand Down Expand Up @@ -669,6 +670,104 @@ func TestGitlabClient_UpdateStatusSetCommitStatusConflictRetryable(t *testing.T)
}
}

func TestGitlabClient_UpdateStatusDifferentRef(t *testing.T) {
logger := logging.NewNoopLogger(t)

cases := []struct {
status models.CommitStatus
expState string
}{
{
models.PendingCommitStatus,
"running",
},
{
models.SuccessCommitStatus,
"success",
},
{
models.FailedCommitStatus,
"failed",
},
}
for _, c := range cases {
t.Run(c.expState, func(t *testing.T) {
gotRequest := false
testServer := httptest.NewServer(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.RequestURI {
case "/api/v4/projects/runatlantis%2Fatlantis/statuses/sha":
gotRequest = true

var updateStatusJsonBody UpdateStatusJsonBody
err := json.NewDecoder(r.Body).Decode(&updateStatusJsonBody)
Ok(t, err)

Equals(t, c.expState, updateStatusJsonBody.State)
Equals(t, updateStatusSrc, updateStatusJsonBody.Context)
Equals(t, updateStatusTargetUrl, updateStatusJsonBody.TargetUrl)
Equals(t, updateStatusDescription, updateStatusJsonBody.Description)
Equals(t, updateStatusHeadBranch, updateStatusJsonBody.Ref)

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/statuses?ref=test&sort=desc":
w.WriteHeader(http.StatusOK)

getCommitStatusesJsonResponse, err := json.Marshal([]EmptyStruct{})
Ok(t, err)

_, err = w.Write(getCommitStatusesJsonResponse)
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,
}

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, gotRequest, "expected to get the request")
})
}
}

func TestGitlabClient_PullIsMergeable(t *testing.T) {
logger := logging.NewNoopLogger(t)
gitlabClientUnderTest = true
Expand Down
Loading