Skip to content

Commit 4114b32

Browse files
committed
fix(gitlab): update /ok-to-test status to success for GitLab
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>
1 parent 14a8e6b commit 4114b32

File tree

6 files changed

+209
-32
lines changed

6 files changed

+209
-32
lines changed

pkg/pipelineascode/match.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e
123123
}
124124
// When /ok-to-test is approved, update the parent "Pipelines as Code CI" status to success
125125
// to indicate the approval was successful before pipelines start running.
126-
if p.event.EventType == opscomments.OkToTestCommentEventType.String() && p.vcx.GetConfig().Name == "gitea" {
126+
if p.event.EventType == opscomments.OkToTestCommentEventType.String() && !strings.Contains(p.vcx.GetConfig().Name, "github") {
127127
approvalStatus := providerstatus.StatusOpts{
128128
Status: CompletedStatus,
129129
Title: "Approved",

test/gitlab_access_control_test.go

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
//go:build e2e
2+
3+
package test
4+
5+
import (
6+
"context"
7+
"fmt"
8+
"os"
9+
"testing"
10+
"time"
11+
12+
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
13+
tgitlab "github.com/openshift-pipelines/pipelines-as-code/test/pkg/gitlab"
14+
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
15+
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/scm"
16+
"github.com/tektoncd/pipeline/pkg/names"
17+
clientGitlab "gitlab.com/gitlab-org/api/client-go"
18+
"gotest.tools/v3/assert"
19+
)
20+
21+
// TestGitlabSuccessStatusAfterOkToTest tests that when an unauthorized user
22+
// creates a fork MR, the CI status starts as pending/skipped, and after an
23+
// authorized user posts /ok-to-test the status transitions to success.
24+
func TestGitlabSuccessStatusAfterOkToTest(t *testing.T) {
25+
ctx := context.Background()
26+
if !tgitlab.HasSecondIdentity() {
27+
t.Skip("Skipping: TEST_GITLAB_SECOND_TOKEN is not configured")
28+
}
29+
30+
topts := &tgitlab.TestOpts{
31+
NoMRCreation: true,
32+
TargetEvent: triggertype.PullRequest.String(),
33+
}
34+
35+
runcnx, opts, glprovider, err := tgitlab.Setup(ctx)
36+
assert.NilError(t, err, fmt.Errorf("cannot do gitlab setup: %w", err))
37+
topts.GLProvider = glprovider
38+
topts.ParamsRun = runcnx
39+
topts.Opts = opts
40+
topts.TargetRefName = names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test")
41+
topts.TargetNS = names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
42+
43+
// Create a fresh GitLab project
44+
groupPath := os.Getenv("TEST_GITLAB_GROUP")
45+
hookURL := os.Getenv("TEST_GITLAB_SMEEURL")
46+
webhookSecret := os.Getenv("TEST_EL_WEBHOOK_SECRET")
47+
project, err := tgitlab.CreateGitLabProject(topts.GLProvider.Client(), groupPath, topts.TargetRefName, hookURL, webhookSecret, false, topts.ParamsRun.Clients.Log)
48+
assert.NilError(t, err)
49+
topts.ProjectID = int(project.ID)
50+
topts.ProjectInfo = project
51+
topts.GitHTMLURL = project.WebURL
52+
topts.DefaultBranch = project.DefaultBranch
53+
54+
defer func() {
55+
if os.Getenv("TEST_NOCLEANUP") != "true" {
56+
tgitlab.TearDown(ctx, t, topts)
57+
}
58+
}()
59+
60+
assert.NilError(t, tgitlab.SetupSecondIdentity(ctx, topts))
61+
62+
err = tgitlab.CreateCRD(ctx, topts)
63+
assert.NilError(t, err)
64+
65+
// Fork project as second user
66+
forkProject, err := tgitlab.ForkGitLabProject(
67+
topts.SecondGLProvider.Client(),
68+
topts.ProjectID,
69+
os.Getenv("TEST_GITLAB_SECOND_GROUP"),
70+
false,
71+
topts.ParamsRun.Clients.Log,
72+
)
73+
assert.NilError(t, err)
74+
defer func() {
75+
topts.ParamsRun.Clients.Log.Infof("Deleting fork project %d", forkProject.ID)
76+
_, err := topts.SecondGLProvider.Client().Projects.DeleteProject(forkProject.ID, nil)
77+
if err != nil {
78+
t.Logf("Error deleting fork project %d: %v", forkProject.ID, err)
79+
}
80+
}()
81+
82+
// Grant first user access to fork so controller can read .tekton
83+
firstUser, _, err := topts.GLProvider.Client().Users.CurrentUser()
84+
assert.NilError(t, err)
85+
assert.NilError(t, tgitlab.AddGitLabProjectMember(
86+
topts.SecondGLProvider.Client(),
87+
int(forkProject.ID),
88+
firstUser.ID,
89+
clientGitlab.DeveloperPermissions,
90+
topts.ParamsRun.Clients.Log,
91+
))
92+
93+
time.Sleep(5 * time.Second)
94+
95+
entries, err := payload.GetEntries(map[string]string{
96+
".tekton/pr.yaml": "testdata/pipelinerun.yaml",
97+
}, topts.TargetNS, topts.DefaultBranch, triggertype.PullRequest.String(), map[string]string{})
98+
assert.NilError(t, err)
99+
100+
targetRefName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-fork-oktotest")
101+
forkCloneURL, err := scm.MakeGitCloneURL(forkProject.WebURL, topts.SecondOpts.UserName, topts.SecondOpts.Password)
102+
assert.NilError(t, err)
103+
104+
_ = scm.PushFilesToRefGit(t, &scm.Opts{
105+
GitURL: forkCloneURL,
106+
CommitTitle: "Add fork ok-to-test fixtures - " + targetRefName,
107+
Log: topts.ParamsRun.Clients.Log,
108+
WebURL: forkProject.WebURL,
109+
TargetRefName: targetRefName,
110+
BaseRefName: topts.DefaultBranch,
111+
}, entries)
112+
113+
// Create MR from fork to original project
114+
mrTitle := "TestGitlabSuccessStatusAfterOkToTest - " + targetRefName
115+
mr, _, err := topts.SecondGLProvider.Client().MergeRequests.CreateMergeRequest(forkProject.ID, &clientGitlab.CreateMergeRequestOptions{
116+
Title: &mrTitle,
117+
SourceBranch: &targetRefName,
118+
TargetBranch: &topts.ProjectInfo.DefaultBranch,
119+
TargetProjectID: &topts.ProjectInfo.ID,
120+
})
121+
assert.NilError(t, err)
122+
defer func() {
123+
_, _, err := topts.GLProvider.Client().MergeRequests.UpdateMergeRequest(topts.ProjectID, mr.IID,
124+
&clientGitlab.UpdateMergeRequestOptions{StateEvent: clientGitlab.Ptr("close")})
125+
if err != nil {
126+
t.Logf("Error closing MR %d: %v", mr.IID, err)
127+
}
128+
}()
129+
130+
mr, _, err = topts.GLProvider.Client().MergeRequests.GetMergeRequest(topts.ProjectID, mr.IID, nil)
131+
assert.NilError(t, err)
132+
topts.ParamsRun.Clients.Log.Infof("Created fork MR %q with SHA %s", mr.WebURL, mr.SHA)
133+
134+
// Post /ok-to-test as authorized user (first user / admin)
135+
topts.ParamsRun.Clients.Log.Infof("Posting /ok-to-test comment as authorized user on MR %d", mr.IID)
136+
_, _, err = topts.GLProvider.Client().Notes.CreateMergeRequestNote(topts.ProjectID, mr.IID,
137+
&clientGitlab.CreateMergeRequestNoteOptions{Body: clientGitlab.Ptr("/ok-to-test")})
138+
assert.NilError(t, err)
139+
140+
// Wait for the pending/skipped status from the unauthorized fork MR
141+
topts.ParamsRun.Clients.Log.Infof("Waiting for pending status on fork MR from unauthorized user")
142+
sourceStatusCount, err := tgitlab.WaitForGitLabCommitStatusCount(ctx, topts.SecondGLProvider.Client(), topts.ParamsRun.Clients.Log, int(forkProject.ID), mr.SHA, "success", 2)
143+
assert.NilError(t, err)
144+
assert.Assert(t, sourceStatusCount >= 2, "expected 2 success commit status on fork, got %d", sourceStatusCount)
145+
}

test/gitlab_merge_request_retest_pruned_test.go

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
package test
44

55
import (
6-
"context"
76
"fmt"
87
"os"
98
"testing"
@@ -19,7 +18,6 @@ import (
1918
twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
2019
"github.com/tektoncd/pipeline/pkg/names"
2120
clientGitlab "gitlab.com/gitlab-org/api/client-go"
22-
"go.uber.org/zap"
2321
"gotest.tools/v3/assert"
2422
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2523
)
@@ -206,6 +204,7 @@ func TestGitlabRetestAfterPipelineRunPruningFromFork(t *testing.T) {
206204
topts.SecondGLProvider.Client(),
207205
topts.ProjectID,
208206
os.Getenv("TEST_GITLAB_SECOND_GROUP"),
207+
true,
209208
topts.ParamsRun.Clients.Log,
210209
)
211210
assert.NilError(t, err)
@@ -292,14 +291,14 @@ func TestGitlabRetestAfterPipelineRunPruningFromFork(t *testing.T) {
292291
assert.NilError(t, err)
293292

294293
topts.ParamsRun.Clients.Log.Infof("Verifying commit statuses on fork (source) project for fork MR")
295-
sourceStatusCount, err := waitForGitLabCommitStatusCount(ctx, topts.SecondGLProvider.Client(), topts.ParamsRun.Clients.Log, int(forkProject.ID), mr.SHA, 2)
294+
sourceStatusCount, err := tgitlab.WaitForGitLabCommitStatusCount(ctx, topts.SecondGLProvider.Client(), topts.ParamsRun.Clients.Log, int(forkProject.ID), mr.SHA, "", 2)
296295
assert.NilError(t, err)
297296
assert.Assert(t, sourceStatusCount >= 2, "expected at least 2 commit statuses on fork (source) project, got %d", sourceStatusCount)
298297

299298
topts.ParamsRun.Clients.Log.Infof("Verifying no commit statuses on target project for fork MR")
300-
targetStatusCount, err := gitlabCommitStatusCount(topts.GLProvider.Client(), topts.ProjectID, mr.SHA)
299+
targetStatuses, _, err := topts.GLProvider.Client().Commits.GetCommitStatuses(topts.ProjectID, mr.SHA, &clientGitlab.GetCommitStatusesOptions{})
301300
assert.NilError(t, err)
302-
assert.Equal(t, targetStatusCount, 0,
301+
assert.Equal(t, len(targetStatuses), 0,
303302
"expected no commit statuses on target project; with Developer access on fork, statuses are written directly to the source project")
304303

305304
topts.ParamsRun.Clients.Log.Infof("Verifying initial PipelineRuns for fork MR")
@@ -373,25 +372,3 @@ func TestGitlabRetestAfterPipelineRunPruningFromFork(t *testing.T) {
373372
assert.Equal(t, newCount, 1,
374373
"expected only 1 new PipelineRun after /retest from a fork MR, but got %d", newCount)
375374
}
376-
377-
func waitForGitLabCommitStatusCount(ctx context.Context, client *clientGitlab.Client, logger *zap.SugaredLogger, projectID int, sha string, minCount int) (int, error) {
378-
var count int
379-
err := kubeinteraction.PollImmediateWithContext(ctx, twait.DefaultTimeout, func() (bool, error) {
380-
currentCount, err := gitlabCommitStatusCount(client, projectID, sha)
381-
logger.Infof("Current GitLab commit status count: %d (waiting for at least %d)", currentCount, minCount)
382-
if err != nil {
383-
return false, err
384-
}
385-
count = currentCount
386-
return count >= minCount, nil
387-
})
388-
return count, err
389-
}
390-
391-
func gitlabCommitStatusCount(client *clientGitlab.Client, projectID int, sha string) (int, error) {
392-
statuses, _, err := client.Commits.GetCommitStatuses(projectID, sha, &clientGitlab.GetCommitStatusesOptions{})
393-
if err != nil {
394-
return 0, err
395-
}
396-
return len(statuses), nil
397-
}

test/pkg/gitlab/scm.go

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,19 +13,25 @@ import (
1313
// a webhook pointing to the given hookURL (for example, a smee channel URL
1414
// used to forward events to the controller). The project is initialised with
1515
// a README on the "main" branch.
16-
func CreateGitLabProject(client *gitlab.Client, groupPath, projectName, hookURL, webhookSecret string, logger *zap.SugaredLogger) (*gitlab.Project, error) {
16+
func CreateGitLabProject(client *gitlab.Client, groupPath, projectName, hookURL, webhookSecret string, isPrivate bool, logger *zap.SugaredLogger) (*gitlab.Project, error) {
1717
logger.Infof("Looking up GitLab group %s", groupPath)
1818
group, _, err := client.Groups.GetGroup(groupPath, nil)
1919
if err != nil {
2020
return nil, fmt.Errorf("failed to look up group %s: %w", groupPath, err)
2121
}
2222

23+
visibility := gitlab.PublicVisibility
24+
if isPrivate {
25+
visibility = gitlab.PrivateVisibility
26+
}
27+
2328
logger.Infof("Creating GitLab project %s in group %s (ID %d)", projectName, groupPath, group.ID)
2429
project, _, err := client.Projects.CreateProject(&gitlab.CreateProjectOptions{
2530
Name: gitlab.Ptr(projectName),
2631
NamespaceID: gitlab.Ptr(group.ID),
2732
InitializeWithReadme: gitlab.Ptr(true),
2833
DefaultBranch: gitlab.Ptr("main"),
34+
Visibility: gitlab.Ptr(visibility),
2935
})
3036
if err != nil {
3137
return nil, fmt.Errorf("failed to create project %s: %w", projectName, err)
@@ -49,7 +55,7 @@ func CreateGitLabProject(client *gitlab.Client, groupPath, projectName, hookURL,
4955
return project, nil
5056
}
5157

52-
func ForkGitLabProject(client *gitlab.Client, projectID int, namespacePath string, logger *zap.SugaredLogger) (*gitlab.Project, error) {
58+
func ForkGitLabProject(client *gitlab.Client, projectID int, namespacePath string, isPrivate bool, logger *zap.SugaredLogger) (*gitlab.Project, error) {
5359
currentUser, _, err := client.Users.CurrentUser()
5460
if err != nil {
5561
logger.Warnf("could not fetch current GitLab user: %v", err)
@@ -61,9 +67,15 @@ func ForkGitLabProject(client *gitlab.Client, projectID int, namespacePath strin
6167

6268
var project *gitlab.Project
6369
var lastErr error
70+
visibility := gitlab.PublicVisibility
71+
if isPrivate {
72+
visibility = gitlab.PrivateVisibility
73+
}
6474
deadline := time.Now().Add(30 * time.Second)
6575
for attempt := 1; time.Now().Before(deadline); attempt++ {
66-
options := &gitlab.ForkProjectOptions{}
76+
options := &gitlab.ForkProjectOptions{
77+
Visibility: gitlab.Ptr(visibility),
78+
}
6779
if namespacePath != "" {
6880
options.NamespacePath = gitlab.Ptr(namespacePath)
6981
}

test/pkg/gitlab/test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func TestMR(t *testing.T, topts *TestOpts) (context.Context, func()) {
8989
groupPath := os.Getenv("TEST_GITLAB_GROUP")
9090
hookURL := os.Getenv("TEST_GITLAB_SMEEURL")
9191
webhookSecret := os.Getenv("TEST_EL_WEBHOOK_SECRET")
92-
project, err := CreateGitLabProject(topts.GLProvider.Client(), groupPath, topts.TargetRefName, hookURL, webhookSecret, topts.ParamsRun.Clients.Log)
92+
project, err := CreateGitLabProject(topts.GLProvider.Client(), groupPath, topts.TargetRefName, hookURL, webhookSecret, true, topts.ParamsRun.Clients.Log)
9393
assert.NilError(t, err)
9494
topts.ProjectID = int(project.ID)
9595
topts.ProjectInfo = project

test/pkg/gitlab/wait.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package gitlab
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
"github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction"
8+
twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
9+
clientGitlab "gitlab.com/gitlab-org/api/client-go"
10+
"go.uber.org/zap"
11+
)
12+
13+
func WaitForGitLabCommitStatusCount(ctx context.Context, client *clientGitlab.Client, logger *zap.SugaredLogger, projectID int, sha, targetStatus string, minCount int) (int, error) {
14+
//nolint:misspell
15+
if targetStatus != "" && targetStatus != "success" && targetStatus != "failed" && targetStatus != "pending" && targetStatus != "running" && targetStatus != "canceled" {
16+
return 0, fmt.Errorf("invalid target status: %s", targetStatus)
17+
}
18+
19+
ctx, cancel := context.WithTimeout(ctx, twait.DefaultTimeout)
20+
defer cancel()
21+
22+
var count int
23+
err := kubeinteraction.PollImmediateWithContext(ctx, twait.DefaultTimeout, func() (bool, error) {
24+
statuses, _, err := client.Commits.GetCommitStatuses(projectID, sha, &clientGitlab.GetCommitStatusesOptions{All: clientGitlab.Ptr(true)})
25+
if err != nil {
26+
return false, err
27+
}
28+
logger.Infof("Current GitLab commit status count: %d (waiting for at least %d)", len(statuses), minCount)
29+
currentCount := 0
30+
for _, status := range statuses {
31+
if targetStatus == "" || status.Status == targetStatus {
32+
currentCount++
33+
}
34+
35+
if currentCount >= minCount {
36+
count = currentCount
37+
return true, nil
38+
}
39+
}
40+
return false, nil
41+
})
42+
return count, err
43+
}

0 commit comments

Comments
 (0)