fix(gitlab): update /ok-to-test status to success for GitLab#2642
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extends the /ok-to-test approval status update logic to include GitLab, ensuring that the CI status is updated to success upon approval. It introduces a new E2E test for this behavior and refactors GitLab test utilities to support project visibility and commit status polling. A critical bug was identified in the new WaitForGitLabCommitStatusSuccessCount helper where the success counter is not reset within the polling loop, which will lead to incorrect counts and false positives in tests.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2642 +/- ##
=======================================
Coverage 58.81% 58.81%
=======================================
Files 206 206
Lines 20308 20308
=======================================
Hits 11944 11944
Misses 7591 7591
Partials 773 773 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
4 times, most recently
from
54ab8e3 to
ebe7a3a
Compare
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
from
ebe7a3a to
0f030eb
Compare
pkg/pipelineascode/match.go
Outdated
| // When /ok-to-test is approved, update the parent "Pipelines as Code CI" status to success | ||
| // to indicate the approval was successful before pipelines start running. | ||
| if p.event.EventType == opscomments.OkToTestCommentEventType.String() && p.vcx.GetConfig().Name == "gitea" { | ||
| if p.event.EventType == opscomments.OkToTestCommentEventType.String() && (p.vcx.GetConfig().Name == "gitea" || p.vcx.GetConfig().Name == "gitlab") { |
There was a problem hiding this comment.
this would be the same for any webhook based provider (ie not github app)
can you do some enum + function that would be abler to compare all of the webhook based one?
There was a problem hiding this comment.
this not true for github webhook as well, it also provides way to update the status
There was a problem hiding this comment.
so I think we can do !strings.Contains(p.vcx.GetConfig().Name, "github") to cover all the provider except github webhook & app.
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
4 times, most recently
from
b5e9115 to
0107f52
Compare
|
can you hold on relaunching the tests/failures, i want to use it to test why the Re-Run button fix is not working |
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
from
0107f52 to
de2f4b2
Compare
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
from
de2f4b2 to
540dd9d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the logic for handling /ok-to-test comments to include non-GitHub providers and introduces a new E2E test for GitLab access control. It also refactors GitLab project creation and forking to support visibility settings and adds a shared helper for waiting on commit status counts. Feedback focuses on improving the reliability and maintainability of the new test helpers, specifically addressing incorrect counting logic in the shared GitLab status waiter, removing redundant local helper functions that violate naming conventions, and softening strict assertions in E2E tests to prevent flakiness.
test/pkg/gitlab/wait.go
Outdated
| func WaitForGitLabCommitStatusCount(ctx context.Context, client *clientGitlab.Client, logger *zap.SugaredLogger, projectID int, sha, targetStatus string, minSuccessCount int) (int, error) { | ||
| if targetStatus != "success" && targetStatus != "failed" && targetStatus != "pending" && targetStatus != "running" { | ||
| return 0, fmt.Errorf("invalid target status: %s", targetStatus) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(ctx, twait.DefaultTimeout) | ||
| defer cancel() | ||
|
|
||
| var successCount int | ||
| err := kubeinteraction.PollImmediateWithContext(ctx, twait.DefaultTimeout, func() (bool, error) { | ||
| statuses, _, err := client.Commits.GetCommitStatuses(projectID, sha, &clientGitlab.GetCommitStatusesOptions{All: clientGitlab.Ptr(true)}) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| logger.Infof("Current GitLab commit status count: %d (waiting for at least %d)", len(statuses), minSuccessCount) | ||
| currentSuccessCount := 0 | ||
| for _, status := range statuses { | ||
| if status.Status == targetStatus { | ||
| currentSuccessCount++ | ||
| } | ||
|
|
||
| if currentSuccessCount >= minSuccessCount { | ||
| successCount = currentSuccessCount | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| }) | ||
| return successCount, err | ||
| } |
There was a problem hiding this comment.
The WaitForGitLabCommitStatusCount helper has several issues that can lead to flaky or incorrect test results:
- Incorrect Counting Logic: It uses
All: truewhen fetching statuses, which returns the entire history. The loop then counts every historical entry matchingtargetStatus. If a status was retried (e.g.,pending->success->success), it will be counted twice, even though only one unique context is successful. RemovingAll: trueensures GitLab only returns the latest status for each unique context. - Misleading Naming: The parameter
minSuccessCountis used for any status (includingfailedorpending). It should be renamed tominCountfor clarity. - Incomplete Validation: The
targetStatuscheck is missing thecanceledstate. - Generic Usage: Allowing an empty
targetStatuswould make this helper more versatile, enabling it to replace local implementations that simply count all unique statuses.
func WaitForGitLabCommitStatusCount(ctx context.Context, client *clientGitlab.Client, logger *zap.SugaredLogger, projectID int, sha, targetStatus string, minCount int) (int, error) {
if targetStatus != "" && targetStatus != "success" && targetStatus != "failed" && targetStatus != "pending" && targetStatus != "running" && targetStatus != "canceled" {
return 0, fmt.Errorf("invalid target status: %s", targetStatus)
}
ctx, cancel := context.WithTimeout(ctx, twait.DefaultTimeout)
defer cancel()
var count int
err := kubeinteraction.PollImmediateWithContext(ctx, twait.DefaultTimeout, func() (bool, error) {
// Do not use All: true so we only get the latest status for each unique context
statuses, _, err := client.Commits.GetCommitStatuses(projectID, sha, &clientGitlab.GetCommitStatusesOptions{})
if err != nil {
return false, err
}
logger.Infof("Current GitLab commit status count: %d (waiting for at least %d)", len(statuses), minCount)
currentCount := 0
for _, status := range statuses {
if targetStatus == "" || status.Status == targetStatus {
currentCount++
}
}
if currentCount >= minCount {
count = currentCount
return true, nil
}
return false, nil
})
return count, err
}| func waitForGitLabCommitStatusCount(ctx context.Context, client *clientGitlab.Client, logger *zap.SugaredLogger, projectID int, sha string, minCount int) (int, error) { | ||
| var count int | ||
| err := kubeinteraction.PollImmediateWithContext(ctx, twait.DefaultTimeout, func() (bool, error) { | ||
| currentCount, err := gitlabCommitStatusCount(client, projectID, sha) | ||
| logger.Infof("Current GitLab commit status count: %d (waiting for at least %d)", currentCount, minCount) | ||
| statuses, _, err := client.Commits.GetCommitStatuses(projectID, sha, &clientGitlab.GetCommitStatusesOptions{}) | ||
| logger.Infof("Current GitLab commit status count: %d (waiting for at least %d)", len(statuses), minCount) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| count = currentCount | ||
| count = len(statuses) | ||
| return count >= minCount, nil | ||
| }) | ||
| return count, err | ||
| } |
There was a problem hiding this comment.
This local helper waitForGitLabCommitStatusCount is redundant now that a shared implementation WaitForGitLabCommitStatusCount has been added to test/pkg/gitlab/wait.go. To improve maintainability and avoid logic drift, please remove this local version and update the callers to use the shared helper (passing an empty string for targetStatus if all statuses should be counted). Additionally, the current function name violates the repository rule that test-related functions must use PascalCase and not contain underscores.
References
- Test function names must use PascalCase and must not contain any underscores.
test/gitlab_access_control_test.go
Outdated
| topts.ParamsRun.Clients.Log.Infof("Waiting for pending status on fork MR from unauthorized user") | ||
| sourceStatusCount, err := tgitlab.WaitForGitLabCommitStatusCount(ctx, topts.SecondGLProvider.Client(), topts.ParamsRun.Clients.Log, int(forkProject.ID), mr.SHA, "success", 2) | ||
| assert.NilError(t, err) | ||
| assert.Assert(t, sourceStatusCount == 2, "expected 2 success commit status on fork, got %d", sourceStatusCount) |
There was a problem hiding this comment.
Using a strict equality check (== 2) for commit status counts can make E2E tests flaky if there are unexpected statuses (e.g., from GitLab's built-in CI or other integrations). It is generally safer to assert that the count is at least the expected value.
| assert.Assert(t, sourceStatusCount == 2, "expected 2 success commit status on fork, got %d", sourceStatusCount) | |
| assert.Assert(t, sourceStatusCount >= 2, "expected at least 2 success commit status on fork, got %d", sourceStatusCount) |
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
from
540dd9d to
5362ec0
Compare
|
/lgtm |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congrats @zakisk your PR Has been approved 🎉
✅ Pull Request Approved
Approval Status:
- Required Approvals: 1
- Current Approvals: 1
👥 Reviewers Who Approved:
| Reviewer | Permission Level | Approval Status |
|---|---|---|
| @chmouel | admin |
✅ |
📝 Next Steps
- Ensure all required checks pass
- Comply with branch protection rules
- Request a maintainer to merge using the
/mergecommand (or merge it
directly if you have repository permission).
Automated by the PAC Boussole 🧭
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
from
5362ec0 to
90492a5
Compare
Similar to Forgejo, GitLab also leaves the "pending approval" commit status in a pending state after /ok-to-test is posted on an unauthorized user's PR. This extends the existing fix to also update that status to success for GitLab, indicating the approval was successful before pipelines start running. Also adds an e2e test for this flow and refactors GitLab test helpers to support private/public project visibility and shared commit status utilities. https://redhat.atlassian.net/browse/SRVKP-11156 Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
zakisk
force-pushed
the
SRVKP-11156-success-status-ok-to-test-in-gitlab
branch
from
90492a5 to
4114b32
Compare
4 participants
Similar to Forgejo, GitLab also leaves the "pending approval" commit status in a pending state after /ok-to-test is posted on an unauthorized user's PR. This extends the existing fix to also update that status to success for GitLab, indicating the approval was successful before pipelines start running.
Also adds an e2e test for this flow and refactors GitLab test helpers to support private/public project visibility and shared commit status utilities.
https://redhat.atlassian.net/browse/SRVKP-11156
📝 Description of the Change
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.