fix(github): resolve pull_request_number on retest for pushed commits

- The {{ pull_request_number }} variable was not substituted
  when a /retest command was issued on a pushed commit (e.g.
  a commit resulting from a PR merge) because the commit
  comment handler in ParsePayload did not fetch the
  associated pull requests for the commit SHA.
- Fetch PRs via getPullRequestsWithCommit in
  handleCommitCommentEvent so the PullRequestNumber is set
  on the event before variable substitution runs.
- Add a unit test for commit_comment events to verify the
  PullRequestNumber is populated from the associated PR.
- Add an e2e test that merges a PR, issues /retest on the
  merged commit, and asserts the PipelineRun logs contain
  the correct pull request number.
- Add a new testdata PipelineRun fixture that echoes the
  {{ pull_request_number }} variable for validation.

https://issues.redhat.com/browse/SRVKP-10662

Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>

Author: zakisk

pkg/provider/github/github.go

@@ -61,6 +61,7 @@ type Provider struct {
 	skippedRun
 	triggerEvent       string
 	cachedChangedFiles *changedfiles.ChangedFiles
+	commitInfo         *github.Commit
 }
 
 type skippedRun struct {
@@ -401,11 +402,17 @@ func (v *Provider) GetCommitInfo(ctx context.Context, runevent *info.Event) erro
 		sha = branchinfo.Commit.GetSHA()
 	}
 	var err error
-	commit, _, err = wrapAPI(v, "get_commit", func() (*github.Commit, *github.Response, error) {
-		return v.Client().Git.GetCommit(ctx, runevent.Organization, runevent.Repository, sha)
-	})
-	if err != nil {
-		return err
+
+	// check if the commit info is already cached in provider
+	if v.commitInfo == nil {
+		commit, _, err = wrapAPI(v, "get_commit", func() (*github.Commit, *github.Response, error) {
+			return v.Client().Git.GetCommit(ctx, runevent.Organization, runevent.Repository, sha)
+		})
+		if err != nil {
+			return err
+		}
+	} else {
+		commit = v.commitInfo
 	}
 
 	runevent.SHAURL = commit.GetHTMLURL()

pkg/provider/github/parse_payload.go

@@ -622,15 +622,37 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C
 	runevent.BaseURL = runevent.HeadURL
 	runevent.TriggerTarget = triggertype.Push
 	opscomments.SetEventTypeAndTargetPR(runevent, event.GetComment().GetBody())
+	v.userType = event.GetSender().GetType()
 
 	defaultBranch := event.GetRepo().GetDefaultBranch()
 	// Set Event.Repository.DefaultBranch as default branch to runevent.HeadBranch, runevent.BaseBranch
+
+	commit, _, err := v.Client().Git.GetCommit(ctx, runevent.Organization, runevent.Repository, runevent.SHA)
+	if err != nil {
+		return runevent, fmt.Errorf("error getting commit %s: %w", runevent.SHA, err)
+	}
+
+	// as we're going to make GetCommit API again in GetCommitInfo func, it will be cached in provider
+	// so that we're wasting one API call
+	v.commitInfo = commit
+
+	// when the commit is a merge commit, either email is 'noreply@github.com' or name is 'web-flow'
+	isMergeCommit := commit.GetCommitter().GetEmail() == githubNoreplyEmail ||
+		commit.GetCommitter().GetName() == githubWebFlowUser
+
+	prs, err := v.getPullRequestsWithCommit(ctx, runevent.SHA, runevent.Organization, runevent.Repository, isMergeCommit)
+	if err != nil {
+		v.Logger.Warnf("Error getting pull requests associated with the commit in this commit comment event: %v", err)
+	}
+	if len(prs) > 0 {
+		runevent.PullRequestNumber = prs[0].GetNumber()
+	}
+
 	runevent.HeadBranch, runevent.BaseBranch = defaultBranch, defaultBranch
 	var (
 		branchName string
 		prName     string
 		tagName    string
-		err        error
 	)
 
 	// If it is a /test or /retest comment with pipelinerun name figure out the pipelinerun name

pkg/provider/github/parse_payload_test.go

@@ -407,7 +407,9 @@ func TestParsePayLoad(t *testing.T) {
 		targetCancelPipelinerun    string
 		wantedBranchName           string
 		wantedTagName              string
+		wantedPullRequestNumber    int
 		isCancelPipelineRunEnabled bool
+		isMergeCommit              bool
 		skipPushEventForPRCommits  bool
 		objectType                 string
 	}{
@@ -936,6 +938,28 @@ func TestParsePayLoad(t *testing.T) {
 			wantedBranchName:           "test1",
 			isCancelPipelineRunEnabled: true,
 		},
+		{
+			name:          "good/commit comment want pull request number",
+			eventType:     "commit_comment",
+			triggerTarget: "push",
+			githubClient:  true,
+			payloadEventStruct: github.CommitCommentEvent{
+				Repo: sampleRepo,
+				Comment: &github.RepositoryComment{
+					CommitID: github.Ptr("samplePRsha"),
+					HTMLURL:  github.Ptr("/888"),
+					Body:     github.Ptr("/retest dummy"),
+				},
+			},
+			muxReplies: map[string]any{
+				"/repos/owner/reponame/pulls/8881":                samplePR,
+				"/repos/owner/reponame/commits/samplePRsha/pulls": []*github.PullRequest{&samplePR},
+			},
+			shaRet:                  "samplePRsha",
+			targetPipelinerun:       "dummy",
+			wantedBranchName:        "main",
+			wantedPullRequestNumber: 54321,
+		},
 		{
 			name:          "bad/commit comment for cancel a pr with invalid branch name",
 			eventType:     "commit_comment",
@@ -975,6 +999,25 @@ func TestParsePayLoad(t *testing.T) {
 			wantedBranchName:  "main",
 			wantErrString:     "provided SHA samplePRshanew is not the HEAD commit of the branch main",
 		},
+		{
+			name:          "commit comment to retest a pr with a merge commit",
+			eventType:     "commit_comment",
+			triggerTarget: "push",
+			githubClient:  true,
+			payloadEventStruct: github.CommitCommentEvent{
+				Repo: sampleRepo,
+				Comment: &github.RepositoryComment{
+					CommitID: github.Ptr("samplePRsha"),
+					HTMLURL:  github.Ptr("/777"),
+					Body:     github.Ptr("/retest dummy"),
+				},
+			},
+			muxReplies:        map[string]any{"/repos/owner/reponame/pulls/777": samplePR},
+			shaRet:            "samplePRsha",
+			targetPipelinerun: "dummy",
+			wantedBranchName:  "main",
+			isMergeCommit:     true,
+		},
 		{
 			name:          "good/skip push event for skip-pr-commits setting",
 			eventType:     "push",
@@ -1076,6 +1119,24 @@ func TestParsePayLoad(t *testing.T) {
 					bjeez, _ := json.Marshal(tag)
 					fmt.Fprint(rw, string(bjeez))
 				})
+
+				mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/git/commits/%s", "owner", "reponame", tt.shaRet), func(rw http.ResponseWriter, _ *http.Request) {
+					name := "test"
+					email := "test@test.com"
+					if tt.isMergeCommit {
+						name = "web-flow"
+						email = "noreply@github.com"
+					}
+					commit := &github.Commit{
+						SHA: github.Ptr(tt.shaRet),
+						Committer: &github.CommitAuthor{
+							Email: github.Ptr(email),
+							Name:  github.Ptr(name),
+						},
+					}
+					bjeez, _ := json.Marshal(commit)
+					fmt.Fprint(rw, string(bjeez))
+				})
 			}
 
 			logger, _ := logger.GetLogger()
@@ -1117,6 +1178,9 @@ func TestParsePayLoad(t *testing.T) {
 				assert.Equal(t, tt.wantedBranchName, ret.BaseBranch)
 				assert.Equal(t, tt.isCancelPipelineRunEnabled, ret.CancelPipelineRuns)
 			}
+			if tt.wantedPullRequestNumber != 0 {
+				assert.Equal(t, tt.wantedPullRequestNumber, ret.PullRequestNumber)
+			}
 			if tt.targetPipelinerun != "" {
 				assert.Equal(t, tt.targetPipelinerun, ret.TargetTestPipelineRun)
 			}

test/github_push_retest_test.go

@@ -5,17 +5,23 @@ package test
 import (
 	"context"
 	"fmt"
+	"net/http"
+	"os"
 	"regexp"
 	"testing"
 	"time"
 
 	"github.com/google/go-github/v81/github"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx"
 	tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
 	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/options"
+	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
 	twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
+	"github.com/tektoncd/pipeline/pkg/names"
 	"gotest.tools/v3/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -217,3 +223,112 @@ func TestGithubSecondPushRequestGitOpsCommentCancel(t *testing.T) {
 	}
 	assert.Assert(t, cancelled, "No cancelled pipeline run found")
 }
+
+func TestGithubPullRequestRetestPullRequestNumberSubstitution(t *testing.T) {
+	targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
+	ctx := context.Background()
+	g := &tgithub.PRTest{}
+
+	ctx, runcnx, opts, ghcnx, err := tgithub.Setup(ctx, g.SecondController, g.Webhook)
+	assert.NilError(t, err)
+	g.Logger = runcnx.Clients.Log
+
+	g.CommitTitle = fmt.Sprintf("Testing %s with Github APPS integration on %s", g.Label, targetNS)
+	g.Logger.Info(g.CommitTitle)
+
+	repoinfo, resp, err := ghcnx.Client().Repositories.Get(ctx, opts.Organization, opts.Repo)
+	assert.NilError(t, err)
+	if resp != nil && resp.StatusCode == http.StatusNotFound {
+		t.Errorf("Repository %s not found in %s", opts.Organization, opts.Repo)
+	}
+
+	if g.Options.Settings.Github != nil {
+		opts.Settings = g.Options.Settings
+	}
+	err = tgithub.CreateCRD(ctx, t, repoinfo, runcnx, opts, targetNS)
+	assert.NilError(t, err)
+
+	tempBaseBranch := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("temp-base-branch")
+	tempBaseBranchRef := fmt.Sprintf("refs/heads/%s", tempBaseBranch)
+
+	// Get the SHA of the default branch
+	defaultBranchRef, _, err := ghcnx.Client().Git.GetRef(ctx, opts.Organization, opts.Repo, fmt.Sprintf("refs/heads/%s", repoinfo.GetDefaultBranch()))
+	assert.NilError(t, err)
+
+	// Create the temporary base branch
+	_, _, err = ghcnx.Client().Git.CreateRef(ctx, opts.Organization, opts.Repo, github.CreateRef{
+		Ref: tempBaseBranchRef,
+		SHA: *defaultBranchRef.Object.SHA,
+	})
+	assert.NilError(t, err)
+
+	yamlEntries := map[string]string{".tekton/pipelinerun-pr-number-variable.yaml": "testdata/pipelinerun-pr-number-variable.yaml"}
+
+	entries, err := payload.GetEntries(yamlEntries, targetNS, tempBaseBranchRef, triggertype.Push.String(), map[string]string{})
+	assert.NilError(t, err)
+
+	targetRefName := fmt.Sprintf("refs/heads/%s",
+		names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-test"))
+
+	sha, vref, err := tgithub.PushFilesToRef(ctx, ghcnx.Client(), g.CommitTitle, tempBaseBranchRef, targetRefName,
+		opts.Organization, opts.Repo, entries)
+	assert.NilError(t, err)
+
+	g.Logger.Infof("Commit %s has been created and pushed to %s", sha, vref.GetURL())
+	number, err := tgithub.PRCreate(ctx, runcnx, ghcnx, opts.Organization,
+		opts.Repo, targetRefName, tempBaseBranchRef, g.CommitTitle)
+	assert.NilError(t, err)
+
+	g.Cnx = runcnx
+	g.Options = opts
+	g.Provider = ghcnx
+	g.TargetNamespace = targetNS
+	g.TargetRefName = targetRefName
+	g.PRNumber = number
+	g.SHA = sha
+
+	defer g.TearDown(ctx, t)
+	defer func() {
+		if os.Getenv("TEST_NOCLEANUP") != "true" {
+			g.Logger.Infof("Deleting Ref %s", tempBaseBranchRef)
+			_, err := ghcnx.Client().Git.DeleteRef(ctx, opts.Organization, opts.Repo, tempBaseBranchRef)
+			assert.NilError(t, err)
+		}
+	}()
+
+	mergeResult, _, err := g.Provider.Client().PullRequests.Merge(ctx, opts.Organization, opts.Repo, g.PRNumber, g.CommitTitle, &github.PullRequestOptions{
+		MergeMethod: "rebase",
+		SHA:         sha,
+	})
+	assert.NilError(t, err)
+	g.Logger.Infof("Pull request %d has been merged", g.PRNumber)
+
+	// wait for API to reflect this PR in response
+	time.Sleep(10 * time.Second)
+
+	mergedSHA := mergeResult.GetSHA()
+
+	_, _, err = g.Provider.Client().Repositories.CreateComment(ctx, opts.Organization, opts.Repo, mergedSHA, &github.RepositoryComment{Body: github.Ptr("/retest pipelinerun-pr-number-variable branch:" + tempBaseBranch)})
+	assert.NilError(t, err)
+	g.Logger.Infof("Comment %s has been created", mergedSHA)
+
+	waitOpts := twait.Opts{
+		RepoName:        g.TargetNamespace,
+		Namespace:       g.TargetNamespace,
+		MinNumberStatus: 2,
+		PollTimeout:     twait.DefaultTimeout,
+		TargetSHA:       mergedSHA,
+	}
+
+	err = twait.UntilPipelineRunHasReason(ctx, g.Cnx.Clients, tektonv1.PipelineRunReasonSuccessful, waitOpts)
+	assert.NilError(t, err)
+
+	regex := regexp.MustCompile(fmt.Sprintf("I don't know my PR number is %d", g.PRNumber))
+
+	// because there are two pipeline runs were created due to pull request merge, we need the one which is triggered on retest comment.
+	err = twait.RegexpMatchingInPodLog(ctx, g.Cnx, g.TargetNamespace,
+		fmt.Sprintf("pipelinesascode.tekton.dev/event-type=%s",
+			opscomments.RetestSingleCommentEventType.String()),
+		"step-task", *regex, "", 2)
+	assert.NilError(t, err)
+}

test/testdata/pipelinerun-pr-number-variable.yaml

@@ -0,0 +1,19 @@
+---
+apiVersion: tekton.dev/v1beta1
+kind: PipelineRun
+metadata:
+  name: "pipelinerun-pr-number-variable"
+  annotations:
+    pipelinesascode.tekton.dev/target-namespace: "\\ .TargetNamespace //"
+    pipelinesascode.tekton.dev/on-target-branch: "[\\ .TargetBranch //]"
+    pipelinesascode.tekton.dev/on-event: "[push]"
+spec:
+  pipelineSpec:
+    tasks:
+      - name: task
+        displayName: "The Task name is Task"
+        taskSpec:
+          steps:
+            - name: task
+              image: registry.access.redhat.com/ubi10/ubi-micro
+              command: ["/bin/echo", "I don't know my PR number is {{ pull_request_number }}"]