diff --git a/docs/content/docs/guide/running.md b/docs/content/docs/guide/running.md index 950b1e6c4..5649a852c 100644 --- 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 index de46d1e2d..9da6d67e7 100644 --- 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 index db00e0500..4c69e65f9 100644 --- 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{ diff --git a/test/gitea_test.go b/test/gitea_test.go index 5d4dd2ecc..97f7fd170 100644 --- a/test/gitea_test.go +++ b/test/gitea_test.go @@ -378,29 +378,69 @@ func TestGiteaConfigMaxKeepRun(t *testing.T) { assert.Equal(t, len(prs.Items), 1, "should have only one pipelinerun, but we have: %d", len(prs.Items)) } +// TestGiteaConfigCancelInProgress will test the pipelinerun annotation +// `pipelinesascode.tekton.dev/cancel-in-progress: "true", it will first start +// one Pull Request which will run a PipelineRun and then send a /retest which +// should cancel the in progress one. +// It will create a new branch and push a new Pull Request with a PipelineRun of +// the same name of the first PR and make sure PipelineRun of the same name only +// acts on the same Pull Request and not on the one of the others. func TestGiteaConfigCancelInProgress(t *testing.T) { + prmap := map[string]string{".tekton/pr.yaml": "testdata/pipelinerun-cancel-in-progress.yaml"} topts := &tgitea.TestOpts{ - TargetEvent: triggertype.PullRequest.String(), - YAMLFiles: map[string]string{ - ".tekton/pr.yaml": "testdata/pipelinerun-cancel-in-progress.yaml", - }, + TargetEvent: triggertype.PullRequest.String(), + YAMLFiles: prmap, CheckForStatus: "", ExpectEvents: false, Regexp: nil, } _, f := tgitea.TestPR(t, topts) defer f() + + time.Sleep(3 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + // wait a bit that the pipelinerun had created tgitea.PostCommentOnPullRequest(t, topts, "/retest") - time.Sleep(5 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + time.Sleep(2 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + + targetRef := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("cancel-in-progress") + entries, err := payload.GetEntries(prmap, topts.TargetNS, topts.DefaultBranch, topts.TargetEvent, map[string]string{}) + assert.NilError(t, err) + topts.TargetRefName = topts.DefaultBranch + scmOpts := &scm.Opts{ + GitURL: topts.GitCloneURL, + Log: topts.ParamsRun.Clients.Log, + WebURL: topts.GitHTMLURL, + TargetRefName: targetRef, + BaseRefName: topts.DefaultBranch, + NoCheckOutFromBase: true, + } + scm.PushFilesToRefGit(t, scmOpts, entries) + + pr, _, err := topts.GiteaCNX.Client.CreatePullRequest(topts.Opts.Organization, topts.Opts.Repo, gitea.CreatePullRequestOption{ + Title: "Test Pull Request - " + targetRef, + Head: targetRef, + Base: topts.DefaultBranch, + }) + assert.NilError(t, err) + topts.PullRequest = pr + topts.ParamsRun.Clients.Log.Infof("PullRequest %s has been created", pr.HTMLURL) + topts.CheckForStatus = "success" + tgitea.WaitForStatus(t, topts, "heads/"+targetRef, "", false) prs, err := topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{}) assert.NilError(t, err) sort.PipelineRunSortByStartTime(prs.Items) - assert.Equal(t, len(prs.Items), 2, "should have 2 pipelineruns, but we have: %d", len(prs.Items)) - assert.Equal(t, prs.Items[1].GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason(), "Cancelled", "oldest pr should have been cancelled") + assert.Equal(t, len(prs.Items), 3, "should have 2 pipelineruns, but we have: %d", len(prs.Items)) + cancelledPr := 0 + for _, pr := range prs.Items { + if pr.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Cancelled" { + cancelledPr++ + } + } + assert.Equal(t, cancelledPr, 1, "only one pr should have been canceled") } func TestGiteaPush(t *testing.T) { @@ -781,7 +821,7 @@ func TestGiteaProvenanceForDefaultBranch(t *testing.T) { Settings: &v1alpha1.Settings{PipelineRunProvenance: "default_branch"}, NoPullRequestCreation: true, } - verifyProvinance(t, topts, "HELLOMOTO", "step-task", false) + verifyProvenance(t, topts, "HELLOMOTO", "step-task", false) } // TestGiteaProvenanceForSource tests the provenance feature of the PipelineRun. @@ -793,7 +833,7 @@ func TestGiteaProvenanceForSource(t *testing.T) { Settings: &v1alpha1.Settings{PipelineRunProvenance: "source"}, NoPullRequestCreation: true, } - verifyProvinance(t, topts, "testing provenance for source", "step-source-provenance-test", false) + verifyProvenance(t, topts, "testing provenance for source", "step-source-provenance-test", false) } // TestGiteaGlobalRepoProvenanceForDefaultBranch tests the provenance feature of the PipelineRun. @@ -808,7 +848,7 @@ func TestGiteaGlobalRepoProvenanceForDefaultBranch(t *testing.T) { Settings: &v1alpha1.Settings{}, } - verifyProvinance(t, topts, "HELLOMOTO", "step-task", true) + verifyProvenance(t, topts, "HELLOMOTO", "step-task", true) } // TestGiteaGlobalAndLocalRepoProvenance verifies the provenance feature of the PipelineRun, @@ -824,10 +864,10 @@ func TestGiteaGlobalAndLocalRepoProvenance(t *testing.T) { }, } - verifyProvinance(t, topts, "testing provenance for source", "step-source-provenance-test", true) + verifyProvenance(t, topts, "testing provenance for source", "step-source-provenance-test", true) } -func verifyProvinance(t *testing.T, topts *tgitea.TestOpts, expectedOutput, cName string, isGlobal bool) { +func verifyProvenance(t *testing.T, topts *tgitea.TestOpts, expectedOutput, cName string, isGlobal bool) { if isGlobal { ctx := context.Background() topts.ParamsRun, topts.Opts, topts.GiteaCNX, _ = tgitea.Setup(ctx)