fix: Fix cancel in progress to act same pr/branch

We need to ensure that we only cancel the pipeline run that is running on the same branch as the current PR or push event. This ensures that we don't cancel the wrong pipeline run. On PullRequest events, we only cancel the pipeline run that is running on the same Pull Request number. On Push events, we only cancel the pipeline run that is running on the same SourceBranch (which is the same as HeadBranch on push) This avoids the issue when you have multiple pull request running on a repo and we don't cancel the wrong ones that are running for a different PR. Signed-off-by: Chmouel Boudjnah <[email protected]>
openshift-pipelines · Nov 21, 2024 · 6e8c278 · 6e8c278
1 parent eb50fac
commit 6e8c278
Show file tree

Hide file tree

Showing 4 changed files with 278 additions and 47 deletions.
diff --git a/docs/content/docs/guide/running.md b/docs/content/docs/guide/running.md
@@ -119,9 +119,16 @@ has already completed or has been cancelled, it will be skipped. (For persistent
 settings, refer to the [max-keep-run annotation]({{< relref
 "/docs/guide/cleanups.md" >}}) instead.)
 
-The cancellation occurs after the latest PipelineRun has been successfully
-created and started. This annotation cannot be used to ensure that only one
-PipelineRun is active at any time.
+The cancellation selection is filtered by the current `PullRequest` or a
+by the targeted branch in the Push event. This means that if for example there
+are two `PullRequests`, each associated with a `PipelineRun` of the same name
+and marked with the `cancel-in-progress` annotation, the cancellation will
+cancel only the `PipelineRun` currently in progress for the specific
+`PullRequest`. This ensures no interference between the two `PullRequests`.
+
+The cancellation is not immediate and only occurs after the latest PipelineRun
+has been successfully created and started. This annotation cannot be relied to
+ensure that only one PipelineRun is active at any time.
 
 Currently, `cancel-in-progress` cannot be used in conjunction with [concurrency
 limit]({{< relref "/docs/guide/repositorycrd.md#concurrency" >}}).

diff --git a/pkg/pipelineascode/cancel_pipelineruns.go b/pkg/pipelineascode/cancel_pipelineruns.go
@@ -6,16 +6,17 @@ import (
 	"strconv"
 	"sync"
 
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/action"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/action"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 )
 
 var cancelMergePatch = map[string]interface{}{
@@ -46,13 +47,19 @@ func (p *PacRun) cancelInProgress(ctx context.Context, matchPR *tektonv1.Pipelin
 	if !ok {
 		return nil
 	}
-	labelSelector := getLabelSelector(map[string]string{
+
+	labelMap := map[string]string{
 		keys.URLRepository:  formatting.CleanValueKubernetes(p.event.Repository),
 		keys.OriginalPRName: prName,
-	})
-
+		keys.EventType:      p.event.TriggerTarget.String(),
+	}
+	if p.event.TriggerTarget == triggertype.PullRequest {
+		labelMap[keys.PullRequest] = strconv.Itoa(p.event.PullRequestNumber)
+	}
+	labelMaps := getLabelSelector(labelMap)
+	p.run.Clients.Log.Infof("cancel-in-progress: selecting pipelineRuns to cancel with labels: %v", labelMaps)
 	prs, err := p.run.Clients.Tekton.TektonV1().PipelineRuns(matchPR.GetNamespace()).List(ctx, metav1.ListOptions{
-		LabelSelector: labelSelector,
+		LabelSelector: labelMaps,
 	})
 	if err != nil {
 		return fmt.Errorf("failed to list pipelineRuns : %w", err)
@@ -62,6 +69,17 @@ func (p *PacRun) cancelInProgress(ctx context.Context, matchPR *tektonv1.Pipelin
 		if pr.GetName() == matchPR.GetName() {
 			continue
 		}
+		if sourceBranch, ok := pr.GetAnnotations()[keys.SourceBranch]; ok {
+			// NOTE(chmouel): Every PR has their own branch and so is every push to different branch
+			// it means we only cancel pipelinerun of the same name that runs to
+			// the unique branch. Note: HeadBranch is the branch from where the PR
+			// comes from in git jargon.
+			if sourceBranch != p.event.HeadBranch {
+				p.logger.Infof("cancel-in-progress: skipping pipelinerun %v/%v as it is not from the same branch, annotation source-branch: %s event headbranch: %s", pr.GetNamespace(), pr.GetName(), sourceBranch, p.event.HeadBranch)
+				continue
+			}
+		}
+
 		if pr.IsDone() {
 			continue
 		}

diff --git a/pkg/pipelineascode/cancel_pipelineruns_test.go b/pkg/pipelineascode/cancel_pipelineruns_test.go
@@ -6,13 +6,6 @@ import (
 	"testing"
 
 	"github.com/google/go-github/v64/github"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
-	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
 	pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
@@ -22,10 +15,20 @@ import (
 	"knative.dev/pkg/apis"
 	knativeduckv1 "knative.dev/pkg/apis/duck/v1"
 	rtesting "knative.dev/pkg/reconciler/testing"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
 )
 
 var (
-	fooRepo = &v1alpha1.Repository{
+	pullReqNumber = 11
+	fooRepo       = &v1alpha1.Repository{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "foo",
 			Name:      "foo",
@@ -42,25 +45,26 @@ var (
 		keys.OriginalPRName: "pr-foo",
 		keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
 		keys.SHA:            formatting.CleanValueKubernetes("foosha"),
-		keys.PullRequest:    strconv.Itoa(11),
+		keys.PullRequest:    strconv.Itoa(pullReqNumber),
+		keys.EventType:      string(triggertype.PullRequest),
 	}
 	fooRepoAnnotations = map[string]string{
 		keys.URLRepository: "foo",
 		keys.SHA:           "foosha",
-		keys.PullRequest:   strconv.Itoa(11),
+		keys.PullRequest:   strconv.Itoa(pullReqNumber),
 		keys.Repository:    "foo",
 	}
 	fooRepoLabelsPrFooAbc = map[string]string{
 		keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
 		keys.SHA:            formatting.CleanValueKubernetes("foosha"),
-		keys.PullRequest:    strconv.Itoa(11),
+		keys.PullRequest:    strconv.Itoa(pullReqNumber),
 		keys.OriginalPRName: "pr-foo-abc",
 		keys.Repository:     "foo",
 	}
 	fooRepoAnnotationsPrFooAbc = map[string]string{
 		keys.URLRepository:  "foo",
 		keys.SHA:            "foosha",
-		keys.PullRequest:    strconv.Itoa(11),
+		keys.PullRequest:    strconv.Itoa(pullReqNumber),
 		keys.OriginalPRName: "pr-foo-abc",
 		keys.Repository:     "foo",
 	}
@@ -82,7 +86,7 @@ func TestCancelPipelinerun(t *testing.T) {
 				Repository:        "foo",
 				SHA:               "foosha",
 				TriggerTarget:     "pull_request",
-				PullRequestNumber: 11,
+				PullRequestNumber: pullReqNumber,
 				State: info.State{
 					CancelPipelineRuns: true,
 				},
@@ -108,7 +112,7 @@ func TestCancelPipelinerun(t *testing.T) {
 				Repository:        "foo",
 				SHA:               "foosha",
 				TriggerTarget:     "pull_request",
-				PullRequestNumber: 11,
+				PullRequestNumber: pullReqNumber,
 				State: info.State{
 					CancelPipelineRuns: true,
 				},
@@ -123,7 +127,7 @@ func TestCancelPipelinerun(t *testing.T) {
 				Repository:        "foo",
 				SHA:               "foosha",
 				TriggerTarget:     "pull_request",
-				PullRequestNumber: 11,
+				PullRequestNumber: pullReqNumber,
 				State: info.State{
 					CancelPipelineRuns:      true,
 					TargetCancelPipelineRun: "pr-foo-abc",
@@ -169,7 +173,7 @@ func TestCancelPipelinerun(t *testing.T) {
 				Repository:        "foo",
 				SHA:               "foosha",
 				TriggerTarget:     "pull_request",
-				PullRequestNumber: 11,
+				PullRequestNumber: pullReqNumber,
 				State: info.State{
 					CancelPipelineRuns: true,
 				},
@@ -328,16 +332,25 @@ func TestCancelInProgress(t *testing.T) {
 		{
 			name: "cancel in progress",
 			event: &info.Event{
-				Repository: "foo",
-				SHA:        "foosha",
+				Repository:        "foo",
+				SHA:               "foosha",
+				HeadBranch:        "head",
+				EventType:         string(triggertype.PullRequest),
+				TriggerTarget:     triggertype.PullRequest,
+				PullRequestNumber: pullReqNumber,
 			},
 			pipelineRuns: []*pipelinev1.PipelineRun{
 				{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:        "pr-foo",
-						Namespace:   "foo",
-						Labels:      fooRepoLabels,
-						Annotations: map[string]string{keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", keys.Repository: "foo"},
+						Name:      "pr-foo",
+						Namespace: "foo",
+						Labels:    fooRepoLabels,
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true",
+							keys.OriginalPRName:   "pr-foo",
+							keys.Repository:       "foo",
+							keys.SourceBranch:     "head",
+						},
 					},
 					Spec: pipelinev1.PipelineRunSpec{},
 				},
@@ -346,7 +359,11 @@ func TestCancelInProgress(t *testing.T) {
 						GenerateName: "pr-foo-",
 						Namespace:    "foo",
 						Labels:       fooRepoLabels,
-						Annotations:  map[string]string{keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo", keys.Repository: "foo"},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo",
+							keys.Repository:   "foo",
+							keys.SourceBranch: "head",
+						},
 					},
 					Spec: pipelinev1.PipelineRunSpec{},
 				},
@@ -357,6 +374,155 @@ func TestCancelInProgress(t *testing.T) {
 			},
 			wantLog: "cancel-in-progress: cancelling pipelinerun foo/",
 		},
+		{
+			name: "cancel in progress exclude not belonging to same push branch",
+			event: &info.Event{
+				Repository:    "foo",
+				SHA:           "foosha",
+				HeadBranch:    "head",
+				EventType:     string(triggertype.Push),
+				TriggerTarget: triggertype.Push,
+			},
+			pipelineRuns: []*pipelinev1.PipelineRun{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "pr-foo",
+						Namespace: "foo",
+						Labels: map[string]string{
+							keys.OriginalPRName: "pr-foo",
+							keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
+							keys.SHA:            formatting.CleanValueKubernetes("foosha"),
+							keys.EventType:      string(triggertype.Push),
+						},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true",
+							keys.OriginalPRName:   "pr-foo",
+							keys.Repository:       "foo",
+							keys.SourceBranch:     "head",
+						},
+					},
+					Spec: pipelinev1.PipelineRunSpec{},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "pr-foo-1",
+						Namespace: "foo",
+						Labels: map[string]string{
+							keys.OriginalPRName: "pr-foo",
+							keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
+							keys.SHA:            formatting.CleanValueKubernetes("foosha"),
+							keys.EventType:      string(triggertype.Push),
+						},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo",
+							keys.Repository:   "foo",
+							keys.SourceBranch: "anotherhead",
+						},
+					},
+					Spec: pipelinev1.PipelineRunSpec{},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "pr-foo-2",
+						Namespace: "foo",
+						Labels: map[string]string{
+							keys.OriginalPRName: "pr-foo",
+							keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
+							keys.SHA:            formatting.CleanValueKubernetes("foosha"),
+							keys.EventType:      string(triggertype.Push),
+						},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo",
+							keys.Repository:   "foo",
+							keys.SourceBranch: "head",
+						},
+					},
+					Spec: pipelinev1.PipelineRunSpec{},
+				},
+			},
+			repo: fooRepo,
+			cancelledPipelineRuns: map[string]bool{
+				"pr-foo-2": true,
+			},
+			wantLog: "skipping pipelinerun foo/pr-foo-1 as it is not from the same branch",
+		},
+		{
+			name: "cancel in progress exclude not belonging to same pr",
+			event: &info.Event{
+				Repository:        "foo",
+				SHA:               "foosha",
+				HeadBranch:        "head",
+				EventType:         string(triggertype.PullRequest),
+				TriggerTarget:     triggertype.PullRequest,
+				PullRequestNumber: pullReqNumber,
+			},
+			pipelineRuns: []*pipelinev1.PipelineRun{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "pr-foo",
+						Namespace: "foo",
+						Labels: map[string]string{
+							keys.OriginalPRName: "pr-foo",
+							keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
+							keys.SHA:            formatting.CleanValueKubernetes("foosha"),
+							keys.PullRequest:    strconv.Itoa(pullReqNumber),
+							keys.EventType:      string(triggertype.PullRequest),
+						},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true",
+							keys.OriginalPRName:   "pr-foo",
+							keys.Repository:       "foo",
+							keys.SourceBranch:     "head",
+						},
+					},
+					Spec: pipelinev1.PipelineRunSpec{},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "pr-foo-1",
+						Namespace: "foo",
+						Labels: map[string]string{
+							keys.OriginalPRName: "pr-foo",
+							keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
+							keys.SHA:            formatting.CleanValueKubernetes("foosha"),
+							keys.PullRequest:    strconv.Itoa(10),
+							keys.EventType:      string(triggertype.PullRequest),
+						},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo",
+							keys.Repository:   "foo",
+							keys.SourceBranch: "head",
+						},
+					},
+					Spec: pipelinev1.PipelineRunSpec{},
+				},
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "pr-foo-2",
+						Namespace: "foo",
+						Labels: map[string]string{
+							keys.OriginalPRName: "pr-foo",
+							keys.URLRepository:  formatting.CleanValueKubernetes("foo"),
+							keys.SHA:            formatting.CleanValueKubernetes("foosha"),
+							keys.PullRequest:    strconv.Itoa(pullReqNumber),
+							keys.EventType:      string(triggertype.PullRequest),
+						},
+						Annotations: map[string]string{
+							keys.CancelInProgress: "true", keys.OriginalPRName: "pr-foo",
+							keys.Repository:   "foo",
+							keys.SourceBranch: "head",
+						},
+					},
+					Spec: pipelinev1.PipelineRunSpec{},
+				},
+			},
+			repo: fooRepo,
+			cancelledPipelineRuns: map[string]bool{
+				"pr-foo-2": true,
+			},
+			wantLog: "cancel-in-progress: cancelling pipelinerun foo/",
+		},
+
 		{
 			name: "cancel in progress with concurrency limit",
 			event: &info.Event{