Skip to content

Commit a3e227e

Browse files
waveywaveswaveywaves
committed
fix: /ok-to-test /retest pipelineruns should not be created if last sha successful
when executing /ok-to-test or /retest gitops commands, check whether the last pipelinerun created for the same SHA was successful. If the run was successful, skip new pipelinerun creation
1 parent 33a66a0 commit a3e227e

5 files changed

+218
-13
lines changed

‎pkg/matcher/annotation_matcher.go

+36
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,16 @@ import (
1212
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
1313
apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
1414
"github.com/openshift-pipelines/pipelines-as-code/pkg/events"
15+
"github.com/openshift-pipelines/pipelines-as-code/pkg/formatting"
1516
"github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments"
1617
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
1718
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
1819
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
1920
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
2021
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
2122
"go.uber.org/zap"
23+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
"knative.dev/pkg/apis"
2225
)
2326

2427
const (
@@ -353,12 +356,45 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger
353356
}
354357

355358
if len(matchedPRs) > 0 {
359+
if existingPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo); existingPR != nil {
360+
return []Match{{PipelineRun: existingPR, Repo: repo}}, nil
361+
}
356362
return matchedPRs, nil
357363
}
358364

359365
return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns))
360366
}
361367

368+
// checkForExistingSuccessfulPipelineRun checks if there's an existing successful PipelineRun for the same SHA
369+
// when executing /ok-to-test or /retest gitops commands
370+
func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository) *tektonv1.PipelineRun {
371+
if (event.EventType == opscomments.RetestAllCommentEventType.String() ||
372+
event.EventType == opscomments.OkToTestCommentEventType.String()) &&
373+
event.SHA != "" {
374+
labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA))
375+
existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{
376+
LabelSelector: labelSelector,
377+
})
378+
if err == nil && len(existingPRs.Items) > 0 {
379+
var lastRun tektonv1.PipelineRun
380+
lastRun = existingPRs.Items[0]
381+
382+
for _, pr := range existingPRs.Items {
383+
if pr.CreationTimestamp.After(lastRun.CreationTimestamp.Time) {
384+
lastRun = pr
385+
}
386+
}
387+
388+
if lastRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
389+
logger.Infof("skipping creation of new pipelinerun for sha %s as the last pipelinerun '%s' has already succeeded",
390+
event.SHA, lastRun.Name)
391+
return &lastRun
392+
}
393+
}
394+
}
395+
return nil
396+
}
397+
362398
func buildAvailableMatchingAnnotationErr(event *info.Event, pruns []*tektonv1.PipelineRun) string {
363399
errmsg := "available annotations of the PipelineRuns annotations in .tekton/ dir:"
364400
for _, prun := range pruns {

‎pkg/matcher/annotation_matcher_test.go

+130
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,10 @@ import (
3030
zapobserver "go.uber.org/zap/zaptest/observer"
3131
"gotest.tools/v3/assert"
3232
"gotest.tools/v3/golden"
33+
corev1 "k8s.io/api/core/v1"
3334
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
35+
"knative.dev/pkg/apis"
36+
knativeduckv1 "knative.dev/pkg/apis/duck/v1"
3437
rtesting "knative.dev/pkg/reconciler/testing"
3538
)
3639

@@ -2570,3 +2573,130 @@ func TestGetName(t *testing.T) {
25702573
})
25712574
}
25722575
}
2576+
2577+
func TestCheckForExistingSuccessfulPipelineRun(t *testing.T) {
2578+
ctx, _ := rtesting.SetupFakeContext(t)
2579+
logger := zap.NewExample().Sugar()
2580+
2581+
repo := &v1alpha1.Repository{
2582+
ObjectMeta: metav1.ObjectMeta{
2583+
Name: "test-repo",
2584+
Namespace: "test-ns",
2585+
},
2586+
}
2587+
2588+
// Create a successful PipelineRun
2589+
pr := &tektonv1.PipelineRun{
2590+
ObjectMeta: metav1.ObjectMeta{
2591+
Name: "test-pr",
2592+
Namespace: "test-ns",
2593+
Labels: map[string]string{
2594+
keys.SHA: "test-sha",
2595+
},
2596+
CreationTimestamp: metav1.Now(),
2597+
},
2598+
Status: tektonv1.PipelineRunStatus{
2599+
Status: knativeduckv1.Status{
2600+
Conditions: knativeduckv1.Conditions{
2601+
apis.Condition{
2602+
Type: apis.ConditionSucceeded,
2603+
Status: corev1.ConditionTrue,
2604+
},
2605+
},
2606+
},
2607+
},
2608+
}
2609+
2610+
// Create a failed PipelineRun with the same SHA but older
2611+
earlierTime := metav1.NewTime(time.Now().Add(-1 * time.Hour))
2612+
failedPR := &tektonv1.PipelineRun{
2613+
ObjectMeta: metav1.ObjectMeta{
2614+
Name: "failed-pr",
2615+
Namespace: "test-ns",
2616+
Labels: map[string]string{
2617+
keys.SHA: "test-sha",
2618+
},
2619+
CreationTimestamp: earlierTime,
2620+
},
2621+
Status: tektonv1.PipelineRunStatus{
2622+
Status: knativeduckv1.Status{
2623+
Conditions: knativeduckv1.Conditions{
2624+
apis.Condition{
2625+
Type: apis.ConditionSucceeded,
2626+
Status: corev1.ConditionFalse,
2627+
},
2628+
},
2629+
},
2630+
},
2631+
}
2632+
2633+
// Setup test clients
2634+
tdata := testclient.Data{
2635+
PipelineRuns: []*tektonv1.PipelineRun{pr, failedPR},
2636+
Repositories: []*v1alpha1.Repository{repo},
2637+
}
2638+
stdata, _ := testclient.SeedTestData(t, ctx, tdata)
2639+
2640+
cs := &params.Run{
2641+
Clients: clients.Clients{
2642+
Log: logger,
2643+
Tekton: stdata.Pipeline,
2644+
Kube: stdata.Kube,
2645+
},
2646+
}
2647+
2648+
tests := []struct {
2649+
name string
2650+
eventType string
2651+
sha string
2652+
wantPR bool
2653+
}{
2654+
{
2655+
name: "Retest command with matching SHA should find successful PR",
2656+
eventType: opscomments.RetestAllCommentEventType.String(),
2657+
sha: "test-sha",
2658+
wantPR: true,
2659+
},
2660+
{
2661+
name: "Ok-to-test command with matching SHA should find successful PR",
2662+
eventType: opscomments.OkToTestCommentEventType.String(),
2663+
sha: "test-sha",
2664+
wantPR: true,
2665+
},
2666+
{
2667+
name: "Retest command with non-matching SHA should not find PR",
2668+
eventType: opscomments.RetestAllCommentEventType.String(),
2669+
sha: "other-sha",
2670+
wantPR: false,
2671+
},
2672+
{
2673+
name: "Different event type should not find PR",
2674+
eventType: opscomments.TestAllCommentEventType.String(),
2675+
sha: "test-sha",
2676+
wantPR: false,
2677+
},
2678+
}
2679+
2680+
for _, tt := range tests {
2681+
t.Run(tt.name, func(t *testing.T) {
2682+
event := &info.Event{
2683+
EventType: tt.eventType,
2684+
SHA: tt.sha,
2685+
}
2686+
2687+
foundPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo)
2688+
2689+
if tt.wantPR && foundPR == nil {
2690+
t.Errorf("Expected to find a successful PipelineRun, but got nil")
2691+
}
2692+
2693+
if !tt.wantPR && foundPR != nil {
2694+
t.Errorf("Expected not to find a PipelineRun, but found %s", foundPR.Name)
2695+
}
2696+
2697+
if tt.wantPR && foundPR != nil {
2698+
assert.Equal(t, "test-pr", foundPR.Name)
2699+
}
2700+
})
2701+
}
2702+
}

‎test/gitea_test.go

+11-7
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
3232
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
3333
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
34-
"github.com/openshift-pipelines/pipelines-as-code/pkg/sort"
3534
"github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx"
3635
tknpactest "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cli"
3736
tgitea "github.com/openshift-pipelines/pipelines-as-code/test/pkg/gitea"
@@ -394,7 +393,7 @@ func TestGiteaConfigMaxKeepRun(t *testing.T) {
394393
_, err := twait.UntilRepositoryUpdated(context.Background(), topts.ParamsRun.Clients, waitOpts)
395394
assert.NilError(t, err)
396395

397-
time.Sleep(15 * time.Second) // “Evil does not sleep. It waits.” - Galadriel
396+
time.Sleep(15 * time.Second) // "Evil does not sleep. It waits." - Galadriel
398397

399398
prs, err := topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{})
400399
assert.NilError(t, err)
@@ -421,12 +420,12 @@ func TestGiteaConfigCancelInProgress(t *testing.T) {
421420
_, f := tgitea.TestPR(t, topts)
422421
defer f()
423422

424-
time.Sleep(3 * time.Second) // “Evil does not sleep. It waits.” - Galadriel
423+
time.Sleep(3 * time.Second) // "Evil does not sleep. It waits." - Galadriel
425424

426425
// wait a bit that the pipelinerun had created
427426
tgitea.PostCommentOnPullRequest(t, topts, "/retest")
428427

429-
time.Sleep(2 * time.Second) // “Evil does not sleep. It waits.” - Galadriel
428+
time.Sleep(2 * time.Second) // "Evil does not sleep. It waits." - Galadriel
430429

431430
targetRef := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("cancel-in-progress")
432431
entries, err := payload.GetEntries(prmap, topts.TargetNS, topts.DefaultBranch, topts.TargetEvent, map[string]string{})
@@ -456,8 +455,7 @@ func TestGiteaConfigCancelInProgress(t *testing.T) {
456455
prs, err := topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{})
457456
assert.NilError(t, err)
458457

459-
sort.PipelineRunSortByStartTime(prs.Items)
460-
assert.Equal(t, len(prs.Items), 3, "should have 2 pipelineruns, but we have: %d", len(prs.Items))
458+
// Reset counter and count cancelled PipelineRuns from the updated list
461459
cancelledPr := 0
462460
for _, pr := range prs.Items {
463461
if pr.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Cancelled" {
@@ -473,12 +471,18 @@ func TestGiteaConfigCancelInProgress(t *testing.T) {
473471
tgitea.PostCommentOnPullRequest(t, topts, "/retest")
474472
tgitea.WaitForStatus(t, topts, "heads/"+targetRef, "", false)
475473

474+
// Get a fresh list of PipelineRuns after all tests
475+
prs, err = topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{})
476+
assert.NilError(t, err)
477+
478+
// Reset counter and count cancelled PipelineRuns from the updated list
479+
cancelledPr = 0
476480
for _, pr := range prs.Items {
477481
if pr.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Cancelled" {
478482
cancelledPr++
479483
}
480484
}
481-
assert.Equal(t, cancelledPr, 2, "tweo pr should have been canceled")
485+
assert.Equal(t, cancelledPr, 2, "two prs should have been canceled")
482486
}
483487

484488
func TestGiteaConfigCancelInProgressAfterPRClosed(t *testing.T) {

‎test/github_config_maxkeepruns_test.go

+21-3
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,27 @@ func TestGithubMaxKeepRuns(t *testing.T) {
2626
g.RunPullRequest(ctx, t)
2727
defer g.TearDown(ctx, t)
2828

29-
g.Cnx.Clients.Log.Infof("Creating /retest in PullRequest")
30-
_, _, err := g.Provider.Client().Issues.CreateComment(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber,
31-
&ghlib.IssueComment{Body: ghlib.Ptr("/retest")})
29+
// Wait for the first pipeline run to be created
30+
time.Sleep(5 * time.Second)
31+
32+
// Get the name of the PipelineRun to retest specifically
33+
prList, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{})
34+
assert.NilError(t, err)
35+
assert.Assert(t, len(prList.Items) > 0, "Expected at least one PipelineRun to be created")
36+
37+
pipelineRunName := ""
38+
for _, pr := range prList.Items {
39+
if originalName, ok := pr.GetAnnotations()[keys.OriginalPRName]; ok {
40+
pipelineRunName = originalName
41+
break
42+
}
43+
}
44+
assert.Assert(t, pipelineRunName != "", "Could not find the original PipelineRun name")
45+
46+
// Use retest with specific PipelineRun name
47+
g.Cnx.Clients.Log.Infof("Creating /retest %s in PullRequest", pipelineRunName)
48+
_, _, err = g.Provider.Client().Issues.CreateComment(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber,
49+
&ghlib.IssueComment{Body: ghlib.Ptr("/retest " + pipelineRunName)})
3250
assert.NilError(t, err)
3351

3452
g.Cnx.Clients.Log.Infof("Wait for the second repository update to be updated")

‎test/gitlab_merge_request_test.go

+20-3
Original file line numberDiff line numberDiff line change
@@ -101,15 +101,32 @@ func TestGitlabMergeRequest(t *testing.T) {
101101
assert.Equal(t, "Merge Request", prsNew.Items[i].Annotations[keys.EventType])
102102
}
103103

104-
runcnx.Clients.Log.Infof("Sending /retest comment on MergeRequest %s/-/merge_requests/%d", projectinfo.WebURL, mrID)
104+
// Wait a moment to ensure all PipelineRuns are fully created
105+
time.Sleep(5 * time.Second)
106+
107+
// Query for an existing PipelineRun to retest specifically
108+
prList, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{})
109+
assert.NilError(t, err)
110+
assert.Assert(t, len(prList.Items) > 0, "Expected at least one PipelineRun to be created")
111+
112+
pipelineRunName := ""
113+
for _, pr := range prList.Items {
114+
if originalName, ok := pr.GetAnnotations()[keys.OriginalPRName]; ok {
115+
pipelineRunName = originalName
116+
break
117+
}
118+
}
119+
assert.Assert(t, pipelineRunName != "", "Could not find the original PipelineRun name")
120+
121+
runcnx.Clients.Log.Infof("Sending /retest %s comment on MergeRequest %s/-/merge_requests/%d", pipelineRunName, projectinfo.WebURL, mrID)
105122
_, _, err = glprovider.Client().Notes.CreateMergeRequestNote(opts.ProjectID, mrID, &clientGitlab.CreateMergeRequestNoteOptions{
106-
Body: clientGitlab.Ptr("/retest"),
123+
Body: clientGitlab.Ptr("/retest " + pipelineRunName),
107124
})
108125
assert.NilError(t, err)
109126

110127
sopt = twait.SuccessOpt{
111128
Title: commitTitle,
112-
OnEvent: opscomments.RetestAllCommentEventType.String(),
129+
OnEvent: opscomments.RetestSingleCommentEventType.String(),
113130
TargetNS: targetNS,
114131
NumberofPRMatch: 5, // this is the max we get in repos status
115132
SHA: "",

0 commit comments

Comments
 (0)