Skip to content

Commit 77374f3

Browse files
zakiskzakisk
committed
fix: Fixed PipelineRun triggering on PR closed
issue: when a PipelineRun is running on PR and it doesn't have cancel-in-progress annotation specified, on the PR close PAC is cancelling the PipelinRun. solution: in code on PR close, it wasn't checked before that a PipelineRun has cancel-in-progress annotation or not and PAC just cancel PipelineRuns regardless of feature enabled. Now cancel-in-progress annotation is added to PipelineRun labels so that it can be filtered using labelSelector. https://issues.redhat.com/browse/SRVKP-7456 Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
1 parent 81ba2fa commit 77374f3

File tree

5 files changed

+153
-12
lines changed

5 files changed

+153
-12
lines changed

‎pkg/kubeinteraction/labels.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,10 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu
8383
annotations[keys.TargetProjectID] = strconv.Itoa(event.TargetProjectID)
8484
}
8585

86+
if value, ok := pipelineRun.GetObjectMeta().GetAnnotations()[keys.CancelInProgress]; ok {
87+
labels[keys.CancelInProgress] = value
88+
}
89+
8690
for k, v := range labels {
8791
pipelineRun.Labels[k] = v
8892
}

‎pkg/kubeinteraction/labels_test.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,10 @@ func TestAddLabelsAndAnnotations(t *testing.T) {
4141
event: event,
4242
pipelineRun: &tektonv1.PipelineRun{
4343
ObjectMeta: metav1.ObjectMeta{
44-
Labels: map[string]string{},
45-
Annotations: map[string]string{},
44+
Labels: map[string]string{},
45+
Annotations: map[string]string{
46+
keys.CancelInProgress: "true",
47+
},
4648
},
4749
},
4850
repo: &apipac.Repository{
@@ -70,6 +72,8 @@ func TestAddLabelsAndAnnotations(t *testing.T) {
7072
assert.NilError(t, err)
7173
assert.Equal(t, tt.args.pipelineRun.Labels[keys.URLOrg], tt.args.event.Organization, "'%s' != %s",
7274
tt.args.pipelineRun.Labels[keys.URLOrg], tt.args.event.Organization)
75+
assert.Equal(t, tt.args.pipelineRun.Labels[keys.CancelInProgress], tt.args.pipelineRun.Annotations[keys.CancelInProgress], "'%s' != %s",
76+
tt.args.pipelineRun.Labels[keys.CancelInProgress], tt.args.pipelineRun.Annotations[keys.CancelInProgress])
7377
assert.Equal(t, tt.args.pipelineRun.Annotations[keys.URLOrg], tt.args.event.Organization, "'%s' != %s",
7478
tt.args.pipelineRun.Annotations[keys.URLOrg], tt.args.event.Organization)
7579
assert.Equal(t, tt.args.pipelineRun.Annotations[keys.ShaURL], tt.args.event.SHAURL)

‎pkg/pipelineascode/cancel_pipelineruns.go‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,9 @@ var cancelMergePatch = map[string]any{
3232
// that belong to a specific pull request in the given repository.
3333
func (p *PacRun) cancelAllInProgressBelongingToClosedPullRequest(ctx context.Context, repo *v1alpha1.Repository) error {
3434
labelSelector := getLabelSelector(map[string]string{
35-
keys.URLRepository: formatting.CleanValueKubernetes(p.event.Repository),
36-
keys.PullRequest: strconv.Itoa(int(p.event.PullRequestNumber)),
35+
keys.URLRepository: formatting.CleanValueKubernetes(p.event.Repository),
36+
keys.PullRequest: strconv.Itoa(p.event.PullRequestNumber),
37+
keys.CancelInProgress: "true",
3738
})
3839
prs, err := p.run.Clients.Tekton.TektonV1().PipelineRuns(repo.Namespace).List(ctx, metav1.ListOptions{
3940
LabelSelector: labelSelector,
@@ -176,10 +177,6 @@ func (p *PacRun) cancelPipelineRuns(ctx context.Context, prs *tektonv1.PipelineR
176177
continue
177178
}
178179

179-
if pr.IsPending() {
180-
p.logger.Infof("cancel-in-progress: skipping cancelling pipelinerun %v/%v in pending state", pr.GetNamespace(), pr.GetName())
181-
}
182-
183180
p.logger.Infof("cancel-in-progress: cancelling pipelinerun %v/%v", pr.GetNamespace(), pr.GetName())
184181
wg.Add(1)
185182
go func(ctx context.Context, pr tektonv1.PipelineRun) {

‎pkg/pipelineascode/cancel_pipelineruns_test.go‎

Lines changed: 100 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,7 @@ func TestCancelInProgressMatchingPR(t *testing.T) {
867867
}
868868
}
869869

870-
func TestCancelAllInProgressBelongingToPullRequest(t *testing.T) {
870+
func TestCancelAllInProgressBelongingToClosedPullRequest(t *testing.T) {
871871
observer, _ := zapobserver.New(zap.InfoLevel)
872872
logger := zap.New(observer).Sugar()
873873

@@ -879,7 +879,7 @@ func TestCancelAllInProgressBelongingToPullRequest(t *testing.T) {
879879
cancelledPipelineRuns map[string]bool
880880
}{
881881
{
882-
name: "cancel all in progress PipelineRuns",
882+
name: "cancel all in progress PipelineRuns with annotation set to true",
883883
event: &info.Event{
884884
Repository: "foo",
885885
TriggerTarget: "pull_request",
@@ -891,15 +891,29 @@ func TestCancelAllInProgressBelongingToPullRequest(t *testing.T) {
891891
ObjectMeta: metav1.ObjectMeta{
892892
Name: "pr-foo-1",
893893
Namespace: "foo",
894-
Labels: fooRepoLabels,
894+
Labels: map[string]string{
895+
keys.OriginalPRName: "pr-foo",
896+
keys.URLRepository: formatting.CleanValueKubernetes("foo"),
897+
keys.SHA: formatting.CleanValueKubernetes("foosha"),
898+
keys.PullRequest: strconv.Itoa(pullReqNumber),
899+
keys.EventType: string(triggertype.PullRequest),
900+
keys.CancelInProgress: "true",
901+
},
895902
},
896903
Spec: pipelinev1.PipelineRunSpec{},
897904
},
898905
{
899906
ObjectMeta: metav1.ObjectMeta{
900907
Name: "pr-foo-2",
901908
Namespace: "foo",
902-
Labels: fooRepoLabels,
909+
Labels: map[string]string{
910+
keys.OriginalPRName: "pr-foo",
911+
keys.URLRepository: formatting.CleanValueKubernetes("foo"),
912+
keys.SHA: formatting.CleanValueKubernetes("foosha"),
913+
keys.PullRequest: strconv.Itoa(pullReqNumber),
914+
keys.EventType: string(triggertype.PullRequest),
915+
keys.CancelInProgress: "true",
916+
},
903917
},
904918
Spec: pipelinev1.PipelineRunSpec{},
905919
},
@@ -909,6 +923,88 @@ func TestCancelAllInProgressBelongingToPullRequest(t *testing.T) {
909923
"pr-foo-2": true,
910924
},
911925
},
926+
{
927+
name: "cancel all in progress PipelineRuns with annotation set to false",
928+
event: &info.Event{
929+
Repository: "foo",
930+
TriggerTarget: "pull_request",
931+
PullRequestNumber: pullReqNumber,
932+
},
933+
repo: fooRepo,
934+
pipelineRuns: []*pipelinev1.PipelineRun{
935+
{
936+
ObjectMeta: metav1.ObjectMeta{
937+
Name: "pr-foo-1",
938+
Namespace: "foo",
939+
Labels: map[string]string{
940+
keys.OriginalPRName: "pr-foo",
941+
keys.URLRepository: formatting.CleanValueKubernetes("foo"),
942+
keys.SHA: formatting.CleanValueKubernetes("foosha"),
943+
keys.PullRequest: strconv.Itoa(pullReqNumber),
944+
keys.EventType: string(triggertype.PullRequest),
945+
keys.CancelInProgress: "false",
946+
},
947+
},
948+
Spec: pipelinev1.PipelineRunSpec{},
949+
},
950+
{
951+
ObjectMeta: metav1.ObjectMeta{
952+
Name: "pr-foo-2",
953+
Namespace: "foo",
954+
Labels: map[string]string{
955+
keys.OriginalPRName: "pr-foo",
956+
keys.URLRepository: formatting.CleanValueKubernetes("foo"),
957+
keys.SHA: formatting.CleanValueKubernetes("foosha"),
958+
keys.PullRequest: strconv.Itoa(pullReqNumber),
959+
keys.EventType: string(triggertype.PullRequest),
960+
keys.CancelInProgress: "false",
961+
},
962+
},
963+
Spec: pipelinev1.PipelineRunSpec{},
964+
},
965+
},
966+
cancelledPipelineRuns: map[string]bool{},
967+
},
968+
{
969+
name: "cancel all in progress PipelineRuns with no annotation",
970+
event: &info.Event{
971+
Repository: "foo",
972+
TriggerTarget: "pull_request",
973+
PullRequestNumber: pullReqNumber,
974+
},
975+
repo: fooRepo,
976+
pipelineRuns: []*pipelinev1.PipelineRun{
977+
{
978+
ObjectMeta: metav1.ObjectMeta{
979+
Name: "pr-foo-1",
980+
Namespace: "foo",
981+
Labels: map[string]string{
982+
keys.OriginalPRName: "pr-foo",
983+
keys.URLRepository: formatting.CleanValueKubernetes("foo"),
984+
keys.SHA: formatting.CleanValueKubernetes("foosha"),
985+
keys.PullRequest: strconv.Itoa(pullReqNumber),
986+
keys.EventType: string(triggertype.PullRequest),
987+
},
988+
},
989+
Spec: pipelinev1.PipelineRunSpec{},
990+
},
991+
{
992+
ObjectMeta: metav1.ObjectMeta{
993+
Name: "pr-foo-2",
994+
Namespace: "foo",
995+
Labels: map[string]string{
996+
keys.OriginalPRName: "pr-foo",
997+
keys.URLRepository: formatting.CleanValueKubernetes("foo"),
998+
keys.SHA: formatting.CleanValueKubernetes("foosha"),
999+
keys.PullRequest: strconv.Itoa(pullReqNumber),
1000+
keys.EventType: string(triggertype.PullRequest),
1001+
},
1002+
},
1003+
Spec: pipelinev1.PipelineRunSpec{},
1004+
},
1005+
},
1006+
cancelledPipelineRuns: map[string]bool{},
1007+
},
9121008
{
9131009
name: "no PipelineRuns to cancel",
9141010
event: &info.Event{

‎test/github_pullrequest_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"time"
1414

1515
"github.com/google/go-github/v70/github"
16+
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
1617
"gotest.tools/v3/assert"
1718
"gotest.tools/v3/assert/cmp"
1819
"gotest.tools/v3/golden"
@@ -421,6 +422,45 @@ func TestGithubPullRequestNoOnLabelAnnotation(t *testing.T) {
421422
}
422423
}
423424

425+
func TestGithubPullRequestNoPipelineRunCancelledOnPRClosed(t *testing.T) {
426+
ctx := context.Background()
427+
g := &tgithub.PRTest{
428+
Label: "Github PullRequest",
429+
YamlFiles: []string{"testdata/pipelinerun-gitops.yaml"},
430+
NoStatusCheck: true,
431+
}
432+
g.RunPullRequest(ctx, t)
433+
defer g.TearDown(ctx, t)
434+
435+
g.Cnx.Clients.Log.Infof("Waiting for the two pipelinerun to be created")
436+
waitOpts := twait.Opts{
437+
RepoName: g.TargetNamespace,
438+
Namespace: g.TargetNamespace,
439+
MinNumberStatus: 1,
440+
PollTimeout: twait.DefaultTimeout,
441+
TargetSHA: g.SHA,
442+
}
443+
err := twait.UntilPipelineRunCreated(ctx, g.Cnx.Clients, waitOpts)
444+
assert.NilError(t, err)
445+
446+
g.Cnx.Clients.Log.Infof("Closing the PullRequest")
447+
_, _, err = g.Provider.Client.PullRequests.Edit(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, &github.PullRequest{
448+
State: github.Ptr("closed"),
449+
})
450+
assert.NilError(t, err)
451+
452+
for i := 0; i < 10; i++ {
453+
prs, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{})
454+
assert.NilError(t, err)
455+
// after adding a label on the PR we need to make sure that it doesn't trigger another PipelineRun.
456+
assert.Equal(t, len(prs.Items), 1)
457+
prReason := prs.Items[0].Status.GetConditions()[0].Reason
458+
assert.Equal(t, prReason, string(tektonv1.PipelineRunReasonRunning), "Wanted PipelineRun to be running but it is ", prReason)
459+
g.Cnx.Clients.Log.Infof("pipelinerun has desired reason %s till now", prReason)
460+
time.Sleep(1 * time.Second)
461+
}
462+
}
463+
424464
// Local Variables:
425465
// compile-command: "go test -tags=e2e -v -info TestGithubPullRequest$ ."
426466
// End:

0 commit comments

Comments
 (0)
⚡