feat: Honor gitops in GitLab threads replies

Previously, the GitLab provider only checked the top-level comment of a
discussion for the `/ok-to-test` command. This limited where the command
could be effectively used. The `/ok-to-test` command is now recognized
when posted as a reply within a merge request discussion thread.
An E2E test was added to verify this behavior, and the `README.md` was
updated to reflect the new capability.

Jira: https://issues.redhat.com/browse/SRVKP-8324

Co-authored-by: Gemini <gemini@google.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

Author: chmouel

pkg/provider/gitlab/README.md

@@ -32,7 +32,6 @@
 
 ### Until there is a ask for it
 
-- /ok-to-test in threads comments (only top level comment is supported atm).
 - caching API calls for permissions.
 
 ## NOTES

pkg/provider/gitlab/acl.go

@@ -47,19 +47,22 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e
 		nextPage = resp.NextPage
 	}
 
-	for _, comment := range discussions {
-		// TODO: maybe we do threads in the future but for now we just check the top thread for ops related comments
-		topthread := comment.Notes[0]
-		if acl.MatchRegexp(acl.OKToTestCommentRegexp, topthread.Body) {
-			commenterEvent := info.NewEvent()
-			commenterEvent.Event = event.Event
-			commenterEvent.Sender = topthread.Author.Username
-			commenterEvent.BaseBranch = event.BaseBranch
-			commenterEvent.HeadBranch = event.HeadBranch
-			commenterEvent.DefaultBranch = event.DefaultBranch
-			// TODO: we could probably do with caching when checking all issues?
-			if v.checkMembership(ctx, commenterEvent, topthread.Author.ID) {
-				return true, nil
+	for _, discussion := range discussions {
+		// Iterate through every note in the discussion thread and evaluate them.
+		// If a note contains an OK-to-test command, verify the commenter's permission
+		// (either project membership or presence in OWNERS/OWNERS_ALIASES).
+		for _, note := range discussion.Notes {
+			if acl.MatchRegexp(acl.OKToTestCommentRegexp, note.Body) {
+				commenterEvent := info.NewEvent()
+				commenterEvent.Event = event.Event
+				commenterEvent.Sender = note.Author.Username
+				commenterEvent.BaseBranch = event.BaseBranch
+				commenterEvent.HeadBranch = event.HeadBranch
+				commenterEvent.DefaultBranch = event.DefaultBranch
+				// We could add caching for membership checks in the future.
+				if v.checkMembership(ctx, commenterEvent, note.Author.ID) {
+					return true, nil
+				}
 			}
 		}
 	}

pkg/provider/gitlab/acl_test.go

@@ -29,6 +29,7 @@ func TestIsAllowed(t *testing.T) {
 		commentContent  string
 		commentAuthor   string
 		commentAuthorID int
+		threadFirstNote string
 	}{
 		{
 			name:    "check client has been set",
@@ -76,6 +77,23 @@ func TestIsAllowed(t *testing.T) {
 			commentAuthor:   "admin",
 			commentAuthorID: 1111,
 		},
+		{
+			name:       "allowed when /ok-to-test is in a reply note",
+			allowed:    true,
+			wantClient: true,
+			fields: fields{
+				userID:          6666,
+				targetProjectID: 2525,
+			},
+			args: args{
+				event: &info.Event{Sender: "noowner", PullRequestNumber: 2},
+			},
+			allowMemberID:   1111,
+			threadFirstNote: "random comment",
+			commentContent:  "/ok-to-test",
+			commentAuthor:   "admin",
+			commentAuthorID: 1111,
+		},
 		{
 			name:       "disallowed from non authorized note",
 			wantClient: true,
@@ -111,8 +129,15 @@ func TestIsAllowed(t *testing.T) {
 					thelp.MuxGetFile(mux, tt.fields.targetProjectID, "OWNERS", tt.ownerFile, false)
 				}
 				if tt.commentContent != "" {
-					thelp.MuxDiscussionsNote(mux, tt.fields.targetProjectID,
-						tt.args.event.PullRequestNumber, tt.commentAuthor, tt.commentAuthorID, tt.commentContent)
+					if tt.threadFirstNote != "" {
+						thelp.MuxDiscussionsNoteWithReply(mux, tt.fields.targetProjectID,
+							tt.args.event.PullRequestNumber,
+							"someuser", 2222, tt.threadFirstNote,
+							tt.commentAuthor, tt.commentAuthorID, tt.commentContent)
+					} else {
+						thelp.MuxDiscussionsNote(mux, tt.fields.targetProjectID,
+							tt.args.event.PullRequestNumber, tt.commentAuthor, tt.commentAuthorID, tt.commentContent)
+					}
 				} else {
 					thelp.MuxDiscussionsNoteEmpty(mux, tt.fields.targetProjectID, tt.args.event.PullRequestNumber)
 				}

pkg/provider/gitlab/test/test.go

@@ -124,6 +124,29 @@ func MuxDiscussionsNote(mux *http.ServeMux, pid, mrID int, author string, author
 	})
 }
 
+// MuxDiscussionsNoteWithReply returns a single discussion where the first note
+// does not contain the trigger but a subsequent reply does. This simulates
+// /ok-to-test being posted in a thread reply.
+func MuxDiscussionsNoteWithReply(mux *http.ServeMux, pid, mrID int, firstAuthor string, firstAuthorID int, firstContent, okAuthor string, okAuthorID int, okContent string) {
+	path := fmt.Sprintf("/projects/%d/merge_requests/%d/discussions", pid, mrID)
+	mux.HandleFunc(path, func(rw http.ResponseWriter, _ *http.Request) {
+		fmt.Fprintf(rw, `[
+            {
+                "notes": [
+                    {
+                        "body": %q,
+                        "author": {"username": %q, "id": %d}
+                    },
+                    {
+                        "body": %q,
+                        "author": {"username": %q, "id": %d}
+                    }
+                ]
+            }
+        ]`, firstContent, firstAuthor, firstAuthorID, okContent, okAuthor, okAuthorID)
+	})
+}
+
 func MuxGetFile(mux *http.ServeMux, pid int, fname, content string, wantErr bool) {
 	mux.HandleFunc(fmt.Sprintf("/projects/%d/repository/files/%s/raw", pid, fname), func(rw http.ResponseWriter, _ *http.Request) {
 		if wantErr {

test/gitlab_oktotest_thread_reply_test.go

@@ -0,0 +1,95 @@
+//go:build e2e
+// +build e2e
+
+package test
+
+import (
+	"context"
+	"net/http"
+	"testing"
+
+	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx"
+	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"
+	twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
+	"github.com/tektoncd/pipeline/pkg/names"
+	clientGitlab "gitlab.com/gitlab-org/api/client-go"
+	"gotest.tools/v3/assert"
+)
+
+// TestGitlabOpsCommentInThreadReply verifies that a /test command placed
+// in a reply within a discussion thread on a Merge Request is honored.
+func TestGitlabOpsCommentInThreadReply(t *testing.T) {
+	targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
+	ctx := context.Background()
+	runcnx, opts, glprovider, err := tgitlab.Setup(ctx)
+	assert.NilError(t, err)
+	ctx, err = cctx.GetControllerCtxInfo(ctx, runcnx)
+	assert.NilError(t, err)
+	runcnx.Clients.Log.Info("Testing GitLab /test <PipelineRunName> in thread replies")
+
+	projectinfo, resp, err := glprovider.Client().Projects.GetProject(opts.ProjectID, nil)
+	assert.NilError(t, err)
+	if resp != nil && resp.StatusCode == http.StatusNotFound {
+		t.Errorf("Repository %s not found in %s", opts.Organization, opts.Repo)
+	}
+
+	// Create Repository CRD
+	err = tgitlab.CreateCRD(ctx, projectinfo, runcnx, opts, targetNS, nil)
+	assert.NilError(t, err)
+
+	// Create a basic PipelineRun
+	entries, err := payload.GetEntries(map[string]string{
+		".tekton/pipelinerun.yaml": "testdata/pipelinerun.yaml",
+	}, targetNS, projectinfo.DefaultBranch, "pull_request", map[string]string{})
+	assert.NilError(t, err)
+
+	// Create a branch with files and open a merge request
+	targetRefName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test")
+	gitCloneURL, err := scm.MakeGitCloneURL(projectinfo.WebURL, opts.UserName, opts.Password)
+	assert.NilError(t, err)
+	commitTitle := "Committing files from test on " + targetRefName
+	scmOpts := &scm.Opts{
+		GitURL:        gitCloneURL,
+		CommitTitle:   commitTitle,
+		Log:           runcnx.Clients.Log,
+		WebURL:        projectinfo.WebURL,
+		TargetRefName: targetRefName,
+		BaseRefName:   projectinfo.DefaultBranch,
+	}
+	_ = scm.PushFilesToRefGit(t, scmOpts, entries)
+	mrTitle := "TestMergeRequest - " + targetRefName
+	mrID, err := tgitlab.CreateMR(glprovider.Client(), opts.ProjectID, targetRefName, projectinfo.DefaultBranch, mrTitle)
+	assert.NilError(t, err)
+	defer tgitlab.TearDown(ctx, t, runcnx, glprovider, mrID, targetRefName, targetNS, opts.ProjectID)
+
+	// Create a discussion thread with an initial note
+	disc, _, err := glprovider.Client().Discussions.CreateMergeRequestDiscussion(opts.ProjectID, mrID, &clientGitlab.CreateMergeRequestDiscussionOptions{
+		Body: clientGitlab.Ptr("random initial note"),
+	})
+	assert.NilError(t, err)
+
+	// Wait for repository status to reflect a successful run triggered by the comment
+	waitOpts := twait.Opts{
+		RepoName:        targetNS,
+		Namespace:       targetNS,
+		MinNumberStatus: 1,
+		PollTimeout:     twait.DefaultTimeout,
+		TargetSHA:       "",
+	}
+	_, err = twait.UntilRepositoryUpdated(ctx, runcnx.Clients, waitOpts)
+	assert.NilError(t, err)
+
+	runcnx.Clients.Log.Info("Updating discussion with /test comment in a reply thread")
+	// Add a reply to the discussion containing /test
+	_, _, err = glprovider.Client().Discussions.AddMergeRequestDiscussionNote(opts.ProjectID, mrID, disc.ID, &clientGitlab.AddMergeRequestDiscussionNoteOptions{
+		Body: clientGitlab.Ptr("/test"),
+	})
+	assert.NilError(t, err)
+	waitOpts.MinNumberStatus = 2
+	_, err = twait.UntilRepositoryUpdated(ctx, runcnx.Clients, waitOpts)
+	assert.NilError(t, err)
+
+	runcnx.Clients.Log.Info("Repository status updated after /test comment")
+}