Revert on-path-change on pr merge in bitbucket

This reverts commit f9fd3f3.

Reason for Revert:
The previous fix for SRVKP-7432 introduced an unexpected bug
(SRVKP-8616) where the `{{ revision }}` dynamic variable in Pipelines as
Code no longer fetches the correct, newest merge commit in Bitbucket
Data Center. Instead, it was fetching the last commit of the source
branch.

Customer feedback indicates this change breaks their CI/CD workflow, as
their teams rely on the merge-commit ID and do not wish to alter their
git strategies. This is a critical blocker for them, preventing them
from upgrading to versions 1.19.x and accessing other important fixes,
such as the one for CAP-724.

This revert restores the previous behavior, ensuring the `{{ revision
}}` variable correctly references the merge commit. A long-term solution
to address both issues (SRVKP-7432 and SRVKP-8616) will be developed in
separate, newly-created tickets (SRVKP-8619 and SRVKP-8620).

Fixes: SRVKP-8616
Related: SRVKP-7432, SRVKP-8619, SRVKP-8620

Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: zakisk

pkg/provider/bitbucketdatacenter/parse_payload.go

@@ -88,7 +88,7 @@ func sanitizeOwner(owner string) string {
 }
 
 // ParsePayload parses the payload from the event.
-func (v *Provider) ParsePayload(_ context.Context, run *params.Run, request *http.Request,
+func (v *Provider) ParsePayload(_ context.Context, _ *params.Run, request *http.Request,
 	payload string,
 ) (*info.Event, error) {
 	processedEvent := info.NewEvent()
@@ -176,27 +176,6 @@ func (v *Provider) ParsePayload(_ context.Context, run *params.Run, request *htt
 		}
 
 		processedEvent.SHA = e.Changes[0].ToHash
-
-		// In Bitbucket Data Center, when a pull request is merged, it creates two commits in the repository:
-		// 1. A merge commit, which is represented by `changes[0].ToHash`.
-		// 2. The actual commit containing the changes from the source branch.
-		//
-		// However, the merge commit often does not contain any file changes itself,
-		// which can cause issues when determining whether file modifications should trigger PipelineRuns.
-		//
-		// Typically, a regular (non-merge) commit has a single parent, but a merge commit has two parents:
-		// - The first parent is the previous HEAD of the destination branch (the branch into which the PR was merged).
-		// - The second parent is the HEAD of the source branch (the branch being merged).
-		//
-		// To correctly identify the actual commit that contains the changes (i.e., the source branch's HEAD),
-		// we inspect `e.Commits[0]`, and if it has more than one parent, we take the second parent.
-		// This helps ensure we reference the correct commit.
-		if len(e.Commits) > 1 && len(e.Commits[0].Parents) > 1 {
-			processedEvent.SHA = e.Commits[0].Parents[1].ID
-			run.Clients.Log.Infof("Detected a merge commit as HEAD; "+
-				"using second parent commit SHA %s (source branch HEAD) to target push event", e.Commits[0].Parents[1].ID)
-		}
-
 		processedEvent.URL = e.Repository.Links.Self[0].Href
 		processedEvent.BaseBranch = e.Changes[0].RefID
 		processedEvent.HeadBranch = e.Changes[0].RefID

pkg/provider/bitbucketdatacenter/parse_payload_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	bbv1test "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/test"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/types"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger"
 	"gotest.tools/v3/assert"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
@@ -571,7 +570,6 @@ func TestParsePayload(t *testing.T) {
 		payloadEvent            any
 		expEvent                *info.Event
 		eventType               string
-		wantSHA                 string
 		wantErrSubstr           string
 		rawStr                  string
 		targetPipelinerun       string
@@ -609,14 +607,12 @@ func TestParsePayload(t *testing.T) {
 			eventType:    "pr:opened",
 			payloadEvent: bbv1test.MakePREvent(ev1, ""),
 			expEvent:     ev1,
-			wantSHA:      "abcd",
 		},
 		{
 			name:         "good/push",
 			eventType:    "repo:refs_changed",
 			payloadEvent: bbv1test.MakePushEvent(ev1, []types.PushRequestEventChange{{ToHash: ev1.SHA, RefID: "base"}}, []types.Commit{{ID: ev1.SHA}}),
 			expEvent:     ev1,
-			wantSHA:      "abcd",
 		},
 		{
 			name:          "bad/changes are empty in push",
@@ -632,82 +628,37 @@ func TestParsePayload(t *testing.T) {
 			expEvent:      ev1,
 			wantErrSubstr: "push event contains no commits; cannot proceed",
 		},
-		{
-			name:      "good/changes are empty in push",
-			eventType: "repo:refs_changed",
-			payloadEvent: bbv1test.MakePushEvent(ev1, []types.PushRequestEventChange{
-				{
-					Ref:    types.Ref{ID: "refs/heads/main"},
-					ToHash: "abcd",
-				},
-			}, []types.Commit{
-				{
-					Parents: []struct {
-						ID        string `json:"id"`
-						DisplayID string `json:"displayId"`
-					}{
-						{
-							ID:        "efghabcd",
-							DisplayID: "efgh",
-						},
-						{
-							ID:        "abcdefgh",
-							DisplayID: "abcd",
-						},
-					},
-				},
-				{
-					ID: "abcdefgh",
-					Parents: []struct {
-						ID        string `json:"id"`
-						DisplayID string `json:"displayId"`
-					}{
-						{
-							ID:        "weroiusf",
-							DisplayID: "wero",
-						},
-					},
-				},
-			}),
-			expEvent: ev1,
-			wantSHA:  "abcdefgh",
-		},
 		{
 			name:         "good/comment ok-to-test",
 			eventType:    "pr:comment:added",
 			payloadEvent: bbv1test.MakePREvent(ev1, "/ok-to-test"),
 			expEvent:     ev1,
-			wantSHA:      "abcd",
 		},
 		{
 			name:         "good/comment test",
 			eventType:    "pr:comment:added",
 			payloadEvent: bbv1test.MakePREvent(ev1, "/test"),
 			expEvent:     ev1,
-			wantSHA:      "abcd",
 		},
 		{
 			name:              "good/comment retest a pr",
 			eventType:         "pr:comment:added",
 			payloadEvent:      bbv1test.MakePREvent(ev1, "/retest dummy"),
 			expEvent:          ev1,
 			targetPipelinerun: "dummy",
-			wantSHA:           "abcd",
 		},
 		{
 			name:                    "good/comment cancel a pr",
 			eventType:               "pr:comment:added",
 			payloadEvent:            bbv1test.MakePREvent(ev1, "/cancel dummy"),
 			expEvent:                ev1,
 			canceltargetPipelinerun: "dummy",
-			wantSHA:                 "abcd",
 		},
 		{
 			name:         "good/comment cancel all",
 			eventType:    "pr:comment:added",
 			payloadEvent: bbv1test.MakePREvent(ev1, "/cancel"),
 			expEvent:     ev1,
-			wantSHA:      "abcd",
 		},
 		{
 			name:      "branch/deleted with zero hash",
@@ -735,7 +686,6 @@ func TestParsePayload(t *testing.T) {
 			run := &params.Run{
 				Info: info.Info{},
 			}
-			run.Clients.Log, _ = logger.GetLogger()
 			_b, err := json.Marshal(tt.payloadEvent)
 			assert.NilError(t, err)
 			payload := string(_b)
@@ -757,7 +707,6 @@ func TestParsePayload(t *testing.T) {
 				return
 			}
 			// assert SHA ID
-			assert.Equal(t, tt.wantSHA, got.SHA)
 			assert.Equal(t, got.AccountID, tt.expEvent.AccountID)
 
 			// test that we got slashed

test/bitbucket_datacenter_pull_request_test.go

@@ -9,17 +9,12 @@ import (
 	"os"
 	"testing"
 
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	tbbdc "github.com/openshift-pipelines/pipelines-as-code/test/pkg/bitbucketdatacenter"
-	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/options"
-	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
 	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
 
-	"github.com/jenkins-x/go-scm/scm"
 	"github.com/tektoncd/pipeline/pkg/names"
 	"gotest.tools/v3/assert"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestBitbucketDataCenterPullRequest(t *testing.T) {
@@ -40,11 +35,9 @@ func TestBitbucketDataCenterPullRequest(t *testing.T) {
 		files[fmt.Sprintf(".tekton/pipelinerun-%d.yaml", i)] = "testdata/pipelinerun.yaml"
 	}
 
-	files, err = payload.GetEntries(files, targetNS, options.MainBranch, triggertype.PullRequest.String(), map[string]string{})
-	assert.NilError(t, err)
-
 	pr := tbbdc.CreatePR(ctx, t, client, runcnx, opts, repo, files, bitbucketWSOwner, targetNS)
-	defer tbbdc.TearDown(ctx, t, runcnx, client, pr, bitbucketWSOwner, targetNS)
+	runcnx.Clients.Log.Infof("Pull Request with title '%s' is created", pr.Title)
+	defer tbbdc.TearDown(ctx, t, runcnx, client, pr.Number, bitbucketWSOwner, targetNS)
 
 	successOpts := wait.SuccessOpt{
 		TargetNS:        targetNS,
@@ -71,11 +64,9 @@ func TestBitbucketDataCenterCELPathChangeInPullRequest(t *testing.T) {
 		".tekton/pipelinerun.yaml": "testdata/pipelinerun-cel-path-changed.yaml",
 	}
 
-	files, err = payload.GetEntries(files, targetNS, options.MainBranch, triggertype.PullRequest.String(), map[string]string{})
-	assert.NilError(t, err)
-
 	pr := tbbdc.CreatePR(ctx, t, client, runcnx, opts, repo, files, bitbucketWSOwner, targetNS)
-	defer tbbdc.TearDown(ctx, t, runcnx, client, pr, bitbucketWSOwner, targetNS)
+	runcnx.Clients.Log.Infof("Pull Request with title '%s' is created", pr.Title)
+	defer tbbdc.TearDown(ctx, t, runcnx, client, pr.Number, bitbucketWSOwner, targetNS)
 
 	successOpts := wait.SuccessOpt{
 		TargetNS:        targetNS,
@@ -85,64 +76,3 @@ func TestBitbucketDataCenterCELPathChangeInPullRequest(t *testing.T) {
 	}
 	wait.Succeeded(ctx, t, runcnx, opts, successOpts)
 }
-
-func TestBitbucketDataCenterOnPathChangeAnnotationOnPRMerge(t *testing.T) {
-	targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
-	// this would be a temporary base branch for the pull request we're going to raise
-	// we need this because we're going to merge the pull request so that after test
-	// we can delete the temporary base branch and our main branch should not be affected
-	// by this merge because we run the E2E frequently.
-	tempBaseBranch := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
-
-	ctx := context.Background()
-	bitbucketWSOwner := os.Getenv("TEST_BITBUCKET_SERVER_E2E_REPOSITORY")
-
-	ctx, runcnx, opts, client, err := tbbdc.Setup(ctx)
-	assert.NilError(t, err)
-
-	repo := tbbdc.CreateCRD(ctx, t, client, runcnx, bitbucketWSOwner, targetNS)
-	runcnx.Clients.Log.Infof("Repository %s has been created", repo.Name)
-	defer tbbdc.TearDownNs(ctx, t, runcnx, targetNS)
-
-	branch, resp, err := client.Git.CreateRef(ctx, bitbucketWSOwner, tempBaseBranch, repo.Branch)
-	assert.NilError(t, err, "error creating branch: http status code: %d : %v", resp.Status, err)
-	runcnx.Clients.Log.Infof("Base branch %s has been created", branch.Name)
-
-	opts.BaseBranch = branch.Name
-
-	if os.Getenv("TEST_NOCLEANUP") != "true" {
-		defer func() {
-			_, err := client.Git.DeleteRef(ctx, bitbucketWSOwner, tempBaseBranch)
-			assert.NilError(t, err, "error deleting branch: http status code: %d : %v", resp.Status, err)
-		}()
-	}
-
-	files := map[string]string{
-		".tekton/pr.yaml":       "testdata/pipelinerun-on-path-change.yaml",
-		"doc/foo/bar/README.md": "README.md",
-	}
-
-	files, err = payload.GetEntries(files, targetNS, tempBaseBranch, triggertype.Push.String(), map[string]string{})
-	assert.NilError(t, err)
-
-	pr := tbbdc.CreatePR(ctx, t, client, runcnx, opts, repo, files, bitbucketWSOwner, targetNS)
-	defer tbbdc.TearDown(ctx, t, runcnx, client, nil, bitbucketWSOwner, targetNS)
-
-	// merge the pull request so that we can get push event.
-	_, err = client.PullRequests.Merge(ctx, bitbucketWSOwner, pr.Number, &scm.PullRequestMergeOptions{})
-	assert.NilError(t, err)
-
-	successOpts := wait.SuccessOpt{
-		TargetNS:        targetNS,
-		OnEvent:         triggertype.Push.String(),
-		NumberofPRMatch: 1,
-		MinNumberStatus: 1,
-	}
-	wait.Succeeded(ctx, t, runcnx, opts, successOpts)
-
-	pipelineRuns, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{})
-	assert.NilError(t, err)
-	assert.Equal(t, len(pipelineRuns.Items), 1)
-	// check that pipeline run contains on-path-change annotation.
-	assert.Equal(t, pipelineRuns.Items[0].GetAnnotations()[keys.OnPathChange], "[doc/***.md]")
-}

test/bitbucket_datacenter_push_test.go

@@ -39,7 +39,7 @@ func TestBitbucketDataCenterCELPathChangeOnPush(t *testing.T) {
 	branch, resp, err := client.Git.CreateRef(ctx, bitbucketWSOwner, targetNS, mainBranchRef)
 	assert.NilError(t, err, "error creating branch: http status code: %d : %v", resp.Status, err)
 	runcnx.Clients.Log.Infof("Branch %s has been created", branch.Name)
-	defer tbbs.TearDown(ctx, t, runcnx, client, nil, bitbucketWSOwner, branch.Name)
+	defer tbbs.TearDown(ctx, t, runcnx, client, -1, bitbucketWSOwner, branch.Name)
 
 	files, err = payload.GetEntries(files, targetNS, branch.Name, triggertype.Push.String(), map[string]string{})
 	assert.NilError(t, err)

test/pkg/bitbucketdatacenter/pr.go

@@ -6,32 +6,31 @@ import (
 	"testing"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/options"
+	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
 	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/scm"
 
 	goscm "github.com/jenkins-x/go-scm/scm"
 	"gotest.tools/v3/assert"
 )
 
 func CreatePR(ctx context.Context, t *testing.T, client *goscm.Client, runcnx *params.Run, opts options.E2E, repo *goscm.Repository, files map[string]string, orgAndRepo, targetNS string) *goscm.PullRequest {
-	baseBranchRef := repo.Branch
-	if opts.BaseBranch != "" {
-		baseBranchRef = opts.BaseBranch
-	}
-
-	branch, resp, err := client.Git.CreateRef(ctx, orgAndRepo, targetNS, baseBranchRef)
+	mainBranchRef := "refs/heads/main"
+	branch, resp, err := client.Git.CreateRef(ctx, orgAndRepo, targetNS, mainBranchRef)
 	assert.NilError(t, err, "error creating branch: http status code: %d : %v", resp.Status, err)
 	runcnx.Clients.Log.Infof("Branch %s has been created", branch.Name)
 
+	files, err = payload.GetEntries(files, targetNS, options.MainBranch, triggertype.PullRequest.String(), map[string]string{})
+	assert.NilError(t, err)
 	gitCloneURL, err := scm.MakeGitCloneURL(repo.Clone, opts.UserName, opts.Password)
 	assert.NilError(t, err)
-
 	scmOpts := &scm.Opts{
 		GitURL:        gitCloneURL,
 		Log:           runcnx.Clients.Log,
 		WebURL:        repo.Clone,
 		TargetRefName: targetNS,
-		BaseRefName:   baseBranchRef,
+		BaseRefName:   repo.Branch,
 		CommitTitle:   fmt.Sprintf("commit %s", targetNS),
 	}
 	scm.PushFilesToRefGit(t, scmOpts, files)
@@ -42,10 +41,9 @@ func CreatePR(ctx context.Context, t *testing.T, client *goscm.Client, runcnx *p
 		Title: title,
 		Body:  "Test PAC on bitbucket data center",
 		Head:  targetNS,
-		Base:  baseBranchRef,
+		Base:  "main",
 	}
 	pr, resp, err := client.PullRequests.Create(ctx, orgAndRepo, prOpts)
 	assert.NilError(t, err, "error creating pull request: http status code: %d : %v", resp.Status, err)
-	runcnx.Clients.Log.Infof("Created Pull Request with Title '%s'. Head branch '%s' ⮕ Base Branch '%s'", pr.Title, pr.Head.Ref, pr.Base.Ref)
 	return pr
 }

test/pkg/bitbucketdatacenter/setup.go

@@ -82,16 +82,15 @@ func TearDownNs(ctx context.Context, t *testing.T, runcnx *params.Run, targetNS
 	repository.NSTearDown(ctx, t, runcnx, targetNS)
 }
 
-func TearDown(ctx context.Context, t *testing.T, runcnx *params.Run, client *scm.Client, pr *scm.PullRequest, orgAndRepo, ref string) {
+func TearDown(ctx context.Context, t *testing.T, runcnx *params.Run, client *scm.Client, prID int, orgAndRepo, ref string) {
 	if os.Getenv("TEST_NOCLEANUP") == "true" {
 		runcnx.Clients.Log.Infof("Not cleaning up and closing PR since TEST_NOCLEANUP is set")
 		return
 	}
 
-	// in Bitbucket Data Center, merged pull requests cannot be deleted.
-	if pr != nil && !pr.Merged {
-		runcnx.Clients.Log.Infof("Deleting PR #%d", pr.Number)
-		_, err := client.PullRequests.DeletePullRequest(ctx, orgAndRepo, pr.Number)
+	if prID != -1 {
+		runcnx.Clients.Log.Infof("Deleting PR #%d", prID)
+		_, err := client.PullRequests.DeletePullRequest(ctx, orgAndRepo, prID)
 		assert.NilError(t, err)
 	}
 

test/pkg/options/options.go

@@ -4,7 +4,6 @@ import "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascod
 
 type E2E struct {
 	Repo, Organization string
-	BaseBranch         string
 	DirectWebhook      bool
 	ProjectID          int
 	ControllerURL      string