Skip to content

Commit c6ff19c

Browse files
zakiskchmouel
zakisk
authored and
chmouel
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 <[email protected]>
1 parent 16234d8 commit c6ff19c

File tree

5 files changed

+173
-15
lines changed

5 files changed

+173
-15
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: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
1-
//go:build e2e
2-
// +build e2e
3-
41
package test
52

63
import (
@@ -13,6 +10,7 @@ import (
1310
"time"
1411

1512
"github.com/google/go-github/v70/github"
13+
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
1614
"gotest.tools/v3/assert"
1715
"gotest.tools/v3/assert/cmp"
1816
"gotest.tools/v3/golden"
@@ -421,6 +419,65 @@ func TestGithubPullRequestNoOnLabelAnnotation(t *testing.T) {
421419
}
422420
}
423421

422+
func TestGithubPullRequestNoPipelineRunCancelledOnPRClosed(t *testing.T) {
423+
ctx := context.Background()
424+
g := &tgithub.PRTest{
425+
Label: "Github PullRequest",
426+
YamlFiles: []string{"testdata/pipelinerun-gitops.yaml"},
427+
NoStatusCheck: true,
428+
}
429+
g.RunPullRequest(ctx, t)
430+
defer g.TearDown(ctx, t)
431+
432+
g.Cnx.Clients.Log.Infof("Waiting for the two pipelinerun to be created")
433+
waitOpts := twait.Opts{
434+
RepoName: g.TargetNamespace,
435+
Namespace: g.TargetNamespace,
436+
MinNumberStatus: 1,
437+
PollTimeout: twait.DefaultTimeout,
438+
TargetSHA: g.SHA,
439+
}
440+
err := twait.UntilPipelineRunCreated(ctx, g.Cnx.Clients, waitOpts)
441+
assert.NilError(t, err)
442+
443+
g.Cnx.Clients.Log.Infof("Closing the PullRequest")
444+
_, _, err = g.Provider.Client.PullRequests.Edit(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, &github.PullRequest{
445+
State: github.Ptr("closed"),
446+
})
447+
assert.NilError(t, err)
448+
449+
isCancelled := false
450+
var prReason string
451+
452+
for range 10 {
453+
prs, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{})
454+
if err != nil {
455+
t.Logf("failed to list PipelineRuns: %v", err)
456+
time.Sleep(1 * time.Second)
457+
continue // try again
458+
}
459+
if len(prs.Items) != 1 {
460+
t.Logf("expected 1 PipelineRun, got %d", len(prs.Items))
461+
time.Sleep(1 * time.Second)
462+
continue // try again
463+
}
464+
// Check all conditions, get the right one
465+
conditions := prs.Items[0].Status.GetConditions()
466+
for _, c := range conditions {
467+
if c.Type == apis.ConditionSucceeded {
468+
prReason = c.Reason
469+
if prReason != string(tektonv1.PipelineRunReasonRunning) {
470+
isCancelled = true
471+
t.Logf("expected PipelineRun `Running`, got %s", prReason)
472+
break
473+
}
474+
}
475+
}
476+
time.Sleep(1 * time.Second)
477+
}
478+
assert.Equal(t, false, isCancelled, fmt.Sprintf("PipelineRun got cancelled while we wanted it `Running`, last reason: %v", prReason))
479+
}
480+
424481
// Local Variables:
425482
// compile-command: "go test -tags=e2e -v -info TestGithubPullRequest$ ."
426483
// End:

0 commit comments

Comments
 (0)