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>

Author: zakisk

pkg/pipelineascode/match.go

@@ -123,7 +123,7 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e
 		}
 		// 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() && !strings.Contains(p.vcx.GetConfig().Name, "github") {
 			approvalStatus := providerstatus.StatusOpts{
 				Status:     CompletedStatus,
 				Title:      "Approved",

test/gitlab_access_control_test.go

@@ -0,0 +1,145 @@
+//go:build e2e
+
+package test
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
+	tgitlab "github.com/openshift-pipelines/pipelines-as-code/test/pkg/gitlab"
+	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
+	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/scm"
+	"github.com/tektoncd/pipeline/pkg/names"
+	clientGitlab "gitlab.com/gitlab-org/api/client-go"
+	"gotest.tools/v3/assert"
+)
+
+// TestGitlabSuccessStatusAfterOkToTest tests that when an unauthorized user
+// creates a fork MR, the CI status starts as pending/skipped, and after an
+// authorized user posts /ok-to-test the status transitions to success.
+func TestGitlabSuccessStatusAfterOkToTest(t *testing.T) {
+	ctx := context.Background()
+	if !tgitlab.HasSecondIdentity() {
+		t.Skip("Skipping: TEST_GITLAB_SECOND_TOKEN is not configured")
+	}
+
+	topts := &tgitlab.TestOpts{
+		NoMRCreation: true,
+		TargetEvent:  triggertype.PullRequest.String(),
+	}
+
+	runcnx, opts, glprovider, err := tgitlab.Setup(ctx)
+	assert.NilError(t, err, fmt.Errorf("cannot do gitlab setup: %w", err))
+	topts.GLProvider = glprovider
+	topts.ParamsRun = runcnx
+	topts.Opts = opts
+	topts.TargetRefName = names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test")
+	topts.TargetNS = names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
+
+	// Create a fresh GitLab project
+	groupPath := os.Getenv("TEST_GITLAB_GROUP")
+	hookURL := os.Getenv("TEST_GITLAB_SMEEURL")
+	webhookSecret := os.Getenv("TEST_EL_WEBHOOK_SECRET")
+	project, err := tgitlab.CreateGitLabProject(topts.GLProvider.Client(), groupPath, topts.TargetRefName, hookURL, webhookSecret, false, topts.ParamsRun.Clients.Log)
+	assert.NilError(t, err)
+	topts.ProjectID = int(project.ID)
+	topts.ProjectInfo = project
+	topts.GitHTMLURL = project.WebURL
+	topts.DefaultBranch = project.DefaultBranch
+
+	defer func() {
+		if os.Getenv("TEST_NOCLEANUP") != "true" {
+			tgitlab.TearDown(ctx, t, topts)
+		}
+	}()
+
+	assert.NilError(t, tgitlab.SetupSecondIdentity(ctx, topts))
+
+	err = tgitlab.CreateCRD(ctx, topts)
+	assert.NilError(t, err)
+
+	// Fork project as second user
+	forkProject, err := tgitlab.ForkGitLabProject(
+		topts.SecondGLProvider.Client(),
+		topts.ProjectID,
+		os.Getenv("TEST_GITLAB_SECOND_GROUP"),
+		false,
+		topts.ParamsRun.Clients.Log,
+	)
+	assert.NilError(t, err)
+	defer func() {
+		topts.ParamsRun.Clients.Log.Infof("Deleting fork project %d", forkProject.ID)
+		_, err := topts.SecondGLProvider.Client().Projects.DeleteProject(forkProject.ID, nil)
+		if err != nil {
+			t.Logf("Error deleting fork project %d: %v", forkProject.ID, err)
+		}
+	}()
+
+	// Grant first user access to fork so controller can read .tekton
+	firstUser, _, err := topts.GLProvider.Client().Users.CurrentUser()
+	assert.NilError(t, err)
+	assert.NilError(t, tgitlab.AddGitLabProjectMember(
+		topts.SecondGLProvider.Client(),
+		int(forkProject.ID),
+		firstUser.ID,
+		clientGitlab.DeveloperPermissions,
+		topts.ParamsRun.Clients.Log,
+	))
+
+	time.Sleep(5 * time.Second)
+
+	entries, err := payload.GetEntries(map[string]string{
+		".tekton/pr.yaml": "testdata/pipelinerun.yaml",
+	}, topts.TargetNS, topts.DefaultBranch, triggertype.PullRequest.String(), map[string]string{})
+	assert.NilError(t, err)
+
+	targetRefName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-fork-oktotest")
+	forkCloneURL, err := scm.MakeGitCloneURL(forkProject.WebURL, topts.SecondOpts.UserName, topts.SecondOpts.Password)
+	assert.NilError(t, err)
+
+	_ = scm.PushFilesToRefGit(t, &scm.Opts{
+		GitURL:        forkCloneURL,
+		CommitTitle:   "Add fork ok-to-test fixtures - " + targetRefName,
+		Log:           topts.ParamsRun.Clients.Log,
+		WebURL:        forkProject.WebURL,
+		TargetRefName: targetRefName,
+		BaseRefName:   topts.DefaultBranch,
+	}, entries)
+
+	// Create MR from fork to original project
+	mrTitle := "TestGitlabSuccessStatusAfterOkToTest - " + targetRefName
+	mr, _, err := topts.SecondGLProvider.Client().MergeRequests.CreateMergeRequest(forkProject.ID, &clientGitlab.CreateMergeRequestOptions{
+		Title:           &mrTitle,
+		SourceBranch:    &targetRefName,
+		TargetBranch:    &topts.ProjectInfo.DefaultBranch,
+		TargetProjectID: &topts.ProjectInfo.ID,
+	})
+	assert.NilError(t, err)
+	defer func() {
+		_, _, err := topts.GLProvider.Client().MergeRequests.UpdateMergeRequest(topts.ProjectID, mr.IID,
+			&clientGitlab.UpdateMergeRequestOptions{StateEvent: clientGitlab.Ptr("close")})
+		if err != nil {
+			t.Logf("Error closing MR %d: %v", mr.IID, err)
+		}
+	}()
+
+	mr, _, err = topts.GLProvider.Client().MergeRequests.GetMergeRequest(topts.ProjectID, mr.IID, nil)
+	assert.NilError(t, err)
+	topts.ParamsRun.Clients.Log.Infof("Created fork MR %q with SHA %s", mr.WebURL, mr.SHA)
+
+	// Post /ok-to-test as authorized user (first user / admin)
+	topts.ParamsRun.Clients.Log.Infof("Posting /ok-to-test comment as authorized user on MR %d", mr.IID)
+	_, _, err = topts.GLProvider.Client().Notes.CreateMergeRequestNote(topts.ProjectID, mr.IID,
+		&clientGitlab.CreateMergeRequestNoteOptions{Body: clientGitlab.Ptr("/ok-to-test")})
+	assert.NilError(t, err)
+
+	// Wait for the pending/skipped status from the unauthorized fork MR
+	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)
+}

test/gitlab_merge_request_retest_pruned_test.go

@@ -206,6 +206,7 @@ func TestGitlabRetestAfterPipelineRunPruningFromFork(t *testing.T) {
 		topts.SecondGLProvider.Client(),
 		topts.ProjectID,
 		os.Getenv("TEST_GITLAB_SECOND_GROUP"),
+		true,
 		topts.ParamsRun.Clients.Log,
 	)
 	assert.NilError(t, err)
@@ -297,9 +298,9 @@ func TestGitlabRetestAfterPipelineRunPruningFromFork(t *testing.T) {
 	assert.Assert(t, sourceStatusCount >= 2, "expected at least 2 commit statuses on fork (source) project, got %d", sourceStatusCount)
 
 	topts.ParamsRun.Clients.Log.Infof("Verifying no commit statuses on target project for fork MR")
-	targetStatusCount, err := gitlabCommitStatusCount(topts.GLProvider.Client(), topts.ProjectID, mr.SHA)
+	targetStatusCount, _, err := topts.GLProvider.Client().Commits.GetCommitStatuses(topts.ProjectID, mr.SHA, &clientGitlab.GetCommitStatusesOptions{})
 	assert.NilError(t, err)
-	assert.Equal(t, targetStatusCount, 0,
+	assert.Equal(t, len(targetStatusCount), 0,
 		"expected no commit statuses on target project; with Developer access on fork, statuses are written directly to the source project")
 
 	topts.ParamsRun.Clients.Log.Infof("Verifying initial PipelineRuns for fork MR")
@@ -377,21 +378,13 @@ func TestGitlabRetestAfterPipelineRunPruningFromFork(t *testing.T) {
 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
 }
-
-func gitlabCommitStatusCount(client *clientGitlab.Client, projectID int, sha string) (int, error) {
-	statuses, _, err := client.Commits.GetCommitStatuses(projectID, sha, &clientGitlab.GetCommitStatusesOptions{})
-	if err != nil {
-		return 0, err
-	}
-	return len(statuses), nil
-}

test/pkg/gitlab/scm.go

@@ -13,19 +13,25 @@ import (
 // a webhook pointing to the given hookURL (for example, a smee channel URL
 // used to forward events to the controller). The project is initialised with
 // a README on the "main" branch.
-func CreateGitLabProject(client *gitlab.Client, groupPath, projectName, hookURL, webhookSecret string, logger *zap.SugaredLogger) (*gitlab.Project, error) {
+func CreateGitLabProject(client *gitlab.Client, groupPath, projectName, hookURL, webhookSecret string, isPrivate bool, logger *zap.SugaredLogger) (*gitlab.Project, error) {
 	logger.Infof("Looking up GitLab group %s", groupPath)
 	group, _, err := client.Groups.GetGroup(groupPath, nil)
 	if err != nil {
 		return nil, fmt.Errorf("failed to look up group %s: %w", groupPath, err)
 	}
 
+	visibility := gitlab.PublicVisibility
+	if isPrivate {
+		visibility = gitlab.PrivateVisibility
+	}
+
 	logger.Infof("Creating GitLab project %s in group %s (ID %d)", projectName, groupPath, group.ID)
 	project, _, err := client.Projects.CreateProject(&gitlab.CreateProjectOptions{
 		Name:                 gitlab.Ptr(projectName),
 		NamespaceID:          gitlab.Ptr(group.ID),
 		InitializeWithReadme: gitlab.Ptr(true),
 		DefaultBranch:        gitlab.Ptr("main"),
+		Visibility:           gitlab.Ptr(visibility),
 	})
 	if err != nil {
 		return nil, fmt.Errorf("failed to create project %s: %w", projectName, err)
@@ -49,7 +55,7 @@ func CreateGitLabProject(client *gitlab.Client, groupPath, projectName, hookURL,
 	return project, nil
 }
 
-func ForkGitLabProject(client *gitlab.Client, projectID int, namespacePath string, logger *zap.SugaredLogger) (*gitlab.Project, error) {
+func ForkGitLabProject(client *gitlab.Client, projectID int, namespacePath string, isPrivate bool, logger *zap.SugaredLogger) (*gitlab.Project, error) {
 	currentUser, _, err := client.Users.CurrentUser()
 	if err != nil {
 		logger.Warnf("could not fetch current GitLab user: %v", err)
@@ -61,9 +67,15 @@ func ForkGitLabProject(client *gitlab.Client, projectID int, namespacePath strin
 
 	var project *gitlab.Project
 	var lastErr error
+	visibility := gitlab.PublicVisibility
+	if isPrivate {
+		visibility = gitlab.PrivateVisibility
+	}
 	deadline := time.Now().Add(30 * time.Second)
 	for attempt := 1; time.Now().Before(deadline); attempt++ {
-		options := &gitlab.ForkProjectOptions{}
+		options := &gitlab.ForkProjectOptions{
+			Visibility: gitlab.Ptr(visibility),
+		}
 		if namespacePath != "" {
 			options.NamespacePath = gitlab.Ptr(namespacePath)
 		}

test/pkg/gitlab/test.go

@@ -89,7 +89,7 @@ func TestMR(t *testing.T, topts *TestOpts) (context.Context, func()) {
 	groupPath := os.Getenv("TEST_GITLAB_GROUP")
 	hookURL := os.Getenv("TEST_GITLAB_SMEEURL")
 	webhookSecret := os.Getenv("TEST_EL_WEBHOOK_SECRET")
-	project, err := CreateGitLabProject(topts.GLProvider.Client(), groupPath, topts.TargetRefName, hookURL, webhookSecret, topts.ParamsRun.Clients.Log)
+	project, err := CreateGitLabProject(topts.GLProvider.Client(), groupPath, topts.TargetRefName, hookURL, webhookSecret, true, topts.ParamsRun.Clients.Log)
 	assert.NilError(t, err)
 	topts.ProjectID = int(project.ID)
 	topts.ProjectInfo = project

test/pkg/gitlab/wait.go

@@ -0,0 +1,42 @@
+package gitlab
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction"
+	twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
+	clientGitlab "gitlab.com/gitlab-org/api/client-go"
+	"go.uber.org/zap"
+)
+
+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
+}