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 5 commits into
base: main
Choose a base branch
from
+143 −33
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
5 commits
Select commit Hold shift + click to select a range
7cb4929
gitlab - ensure lastpipeline ref is the same as head branch
rhariady Jan 21, 2025
7b36f89
gofmt
rhariady Jan 21, 2025
cbf62ff
only check for commit status if LastPipelineID is differ
rhariady Jan 22, 2025
4636a84
use Gitlab get commit statuses API to get previous pipeline
rhariady Jan 22, 2025
6cfdc38
add more info in log when pipeline is found or not found
rhariady Jan 22, 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
24 changes: 17 additions & 7 deletions server/events/vcs/gitlab_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,27 +416,37 @@ 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.

ListOptions: gitlab.ListOptions{
Sort: "desc",
PerPage: 1,
},
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)
}
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)
if len(commitStatuses) > 0 {
logger.Info("Pipeline found for commit %s and ref %s, setting pipeline ID to %d", pull.HeadCommit, pull.HeadBranch, commitStatuses[0].PipelineId)
// Set the pipeline ID to the last pipeline that ran for the commit
setCommitStatusOptions.PipelineID = gitlab.Ptr(commit.LastPipeline.ID)
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 @@ -458,7 +468,7 @@ func (g *GitlabClient) UpdateStatus(logger logging.SimpleLogging, repo models.Re
"attempt", i+1,
"max_attempts", maxAttempts,
"repo", repo.FullName,
"commit", commit.ShortID,
"commit", pull.HeadCommit,
"state", state.String(),
)

Expand Down
152 changes: 126 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 @@ -28,6 +28,7 @@
const updateStatusTargetUrl = "https://google.com"
const updateStatusSrc = "src"
const updateStatusHeadBranch = "test"
const updateStatusHeadBranchDuplicate = "test_duplicate"

Check failure on line 31 in server/events/vcs/gitlab_client_test.go

View workflow job for this annotation

GitHub Actions / Linting 

const `updateStatusHeadBranchDuplicate` is unused (unused)

/* UpdateStatus request JSON body object */
type UpdateStatusJsonBody struct {
Expand All @@ -39,15 +40,13 @@
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 +345,19 @@
_, 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?per_page=1&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 +468,25 @@
_, 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?per_page=1&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 +605,19 @@
_, 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?per_page=1&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 +671,104 @@
}
}

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?per_page=1&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