From a3e227e285a19e16cd2350e9ee119008dc71b927 Mon Sep 17 00:00:00 2001 From: Vibhav Bobade Date: Mon, 5 May 2025 17:46:40 +0530 Subject: [PATCH] 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 --- pkg/matcher/annotation_matcher.go | 36 +++++++ pkg/matcher/annotation_matcher_test.go | 130 +++++++++++++++++++++++++ test/gitea_test.go | 18 ++-- test/github_config_maxkeepruns_test.go | 24 ++++- test/gitlab_merge_request_test.go | 23 ++++- 5 files changed, 218 insertions(+), 13 deletions(-) diff --git a/pkg/matcher/annotation_matcher.go b/pkg/matcher/annotation_matcher.go index 5c9b02f51..51056dee3 100644 --- a/pkg/matcher/annotation_matcher.go +++ b/pkg/matcher/annotation_matcher.go @@ -12,6 +12,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/events" + "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" @@ -19,6 +20,8 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "go.uber.org/zap" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/apis" ) const ( @@ -353,12 +356,45 @@ func MatchPipelinerunByAnnotation(ctx context.Context, logger *zap.SugaredLogger } if len(matchedPRs) > 0 { + if existingPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo); existingPR != nil { + return []Match{{PipelineRun: existingPR, Repo: repo}}, nil + } return matchedPRs, nil } return nil, fmt.Errorf("%s", buildAvailableMatchingAnnotationErr(event, pruns)) } +// checkForExistingSuccessfulPipelineRun checks if there's an existing successful PipelineRun for the same SHA +// when executing /ok-to-test or /retest gitops commands +func checkForExistingSuccessfulPipelineRun(ctx context.Context, logger *zap.SugaredLogger, cs *params.Run, event *info.Event, repo *apipac.Repository) *tektonv1.PipelineRun { + if (event.EventType == opscomments.RetestAllCommentEventType.String() || + event.EventType == opscomments.OkToTestCommentEventType.String()) && + event.SHA != "" { + labelSelector := fmt.Sprintf("%s=%s", keys.SHA, formatting.CleanValueKubernetes(event.SHA)) + existingPRs, err := cs.Clients.Tekton.TektonV1().PipelineRuns(repo.GetNamespace()).List(ctx, metav1.ListOptions{ + LabelSelector: labelSelector, + }) + if err == nil && len(existingPRs.Items) > 0 { + var lastRun tektonv1.PipelineRun + lastRun = existingPRs.Items[0] + + for _, pr := range existingPRs.Items { + if pr.CreationTimestamp.After(lastRun.CreationTimestamp.Time) { + lastRun = pr + } + } + + if lastRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() { + logger.Infof("skipping creation of new pipelinerun for sha %s as the last pipelinerun '%s' has already succeeded", + event.SHA, lastRun.Name) + return &lastRun + } + } + } + return nil +} + func buildAvailableMatchingAnnotationErr(event *info.Event, pruns []*tektonv1.PipelineRun) string { errmsg := "available annotations of the PipelineRuns annotations in .tekton/ dir:" for _, prun := range pruns { diff --git a/pkg/matcher/annotation_matcher_test.go b/pkg/matcher/annotation_matcher_test.go index 2461d5651..73a8029a6 100644 --- a/pkg/matcher/annotation_matcher_test.go +++ b/pkg/matcher/annotation_matcher_test.go @@ -30,7 +30,10 @@ import ( zapobserver "go.uber.org/zap/zaptest/observer" "gotest.tools/v3/assert" "gotest.tools/v3/golden" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/apis" + knativeduckv1 "knative.dev/pkg/apis/duck/v1" rtesting "knative.dev/pkg/reconciler/testing" ) @@ -2570,3 +2573,130 @@ func TestGetName(t *testing.T) { }) } } + +func TestCheckForExistingSuccessfulPipelineRun(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zap.NewExample().Sugar() + + repo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: "test-ns", + }, + } + + // Create a successful PipelineRun + pr := &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pr", + Namespace: "test-ns", + Labels: map[string]string{ + keys.SHA: "test-sha", + }, + CreationTimestamp: metav1.Now(), + }, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + } + + // Create a failed PipelineRun with the same SHA but older + earlierTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) + failedPR := &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "failed-pr", + Namespace: "test-ns", + Labels: map[string]string{ + keys.SHA: "test-sha", + }, + CreationTimestamp: earlierTime, + }, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + } + + // Setup test clients + tdata := testclient.Data{ + PipelineRuns: []*tektonv1.PipelineRun{pr, failedPR}, + Repositories: []*v1alpha1.Repository{repo}, + } + stdata, _ := testclient.SeedTestData(t, ctx, tdata) + + cs := ¶ms.Run{ + Clients: clients.Clients{ + Log: logger, + Tekton: stdata.Pipeline, + Kube: stdata.Kube, + }, + } + + tests := []struct { + name string + eventType string + sha string + wantPR bool + }{ + { + name: "Retest command with matching SHA should find successful PR", + eventType: opscomments.RetestAllCommentEventType.String(), + sha: "test-sha", + wantPR: true, + }, + { + name: "Ok-to-test command with matching SHA should find successful PR", + eventType: opscomments.OkToTestCommentEventType.String(), + sha: "test-sha", + wantPR: true, + }, + { + name: "Retest command with non-matching SHA should not find PR", + eventType: opscomments.RetestAllCommentEventType.String(), + sha: "other-sha", + wantPR: false, + }, + { + name: "Different event type should not find PR", + eventType: opscomments.TestAllCommentEventType.String(), + sha: "test-sha", + wantPR: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + event := &info.Event{ + EventType: tt.eventType, + SHA: tt.sha, + } + + foundPR := checkForExistingSuccessfulPipelineRun(ctx, logger, cs, event, repo) + + if tt.wantPR && foundPR == nil { + t.Errorf("Expected to find a successful PipelineRun, but got nil") + } + + if !tt.wantPR && foundPR != nil { + t.Errorf("Expected not to find a PipelineRun, but found %s", foundPR.Name) + } + + if tt.wantPR && foundPR != nil { + assert.Equal(t, "test-pr", foundPR.Name) + } + }) + } +} diff --git a/test/gitea_test.go b/test/gitea_test.go index d9fad0992..7a6314765 100644 --- a/test/gitea_test.go +++ b/test/gitea_test.go @@ -31,7 +31,6 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" - "github.com/openshift-pipelines/pipelines-as-code/pkg/sort" "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx" tknpactest "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cli" tgitea "github.com/openshift-pipelines/pipelines-as-code/test/pkg/gitea" @@ -394,7 +393,7 @@ func TestGiteaConfigMaxKeepRun(t *testing.T) { _, err := twait.UntilRepositoryUpdated(context.Background(), topts.ParamsRun.Clients, waitOpts) assert.NilError(t, err) - time.Sleep(15 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + time.Sleep(15 * time.Second) // "Evil does not sleep. It waits." - Galadriel prs, err := topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{}) assert.NilError(t, err) @@ -421,12 +420,12 @@ func TestGiteaConfigCancelInProgress(t *testing.T) { _, f := tgitea.TestPR(t, topts) defer f() - time.Sleep(3 * time.Second) // “Evil does not sleep. It waits.” - Galadriel + 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(2 * 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{}) @@ -456,8 +455,7 @@ func TestGiteaConfigCancelInProgress(t *testing.T) { 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), 3, "should have 2 pipelineruns, but we have: %d", len(prs.Items)) + // Reset counter and count cancelled PipelineRuns from the updated list cancelledPr := 0 for _, pr := range prs.Items { if pr.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Cancelled" { @@ -473,12 +471,18 @@ func TestGiteaConfigCancelInProgress(t *testing.T) { tgitea.PostCommentOnPullRequest(t, topts, "/retest") tgitea.WaitForStatus(t, topts, "heads/"+targetRef, "", false) + // Get a fresh list of PipelineRuns after all tests + prs, err = topts.ParamsRun.Clients.Tekton.TektonV1().PipelineRuns(topts.TargetNS).List(context.Background(), metav1.ListOptions{}) + assert.NilError(t, err) + + // Reset counter and count cancelled PipelineRuns from the updated list + cancelledPr = 0 for _, pr := range prs.Items { if pr.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Cancelled" { cancelledPr++ } } - assert.Equal(t, cancelledPr, 2, "tweo pr should have been canceled") + assert.Equal(t, cancelledPr, 2, "two prs should have been canceled") } func TestGiteaConfigCancelInProgressAfterPRClosed(t *testing.T) { diff --git a/test/github_config_maxkeepruns_test.go b/test/github_config_maxkeepruns_test.go index 69aa7a9d8..345bd5ea8 100644 --- a/test/github_config_maxkeepruns_test.go +++ b/test/github_config_maxkeepruns_test.go @@ -26,9 +26,27 @@ func TestGithubMaxKeepRuns(t *testing.T) { g.RunPullRequest(ctx, t) defer g.TearDown(ctx, t) - g.Cnx.Clients.Log.Infof("Creating /retest in PullRequest") - _, _, err := g.Provider.Client().Issues.CreateComment(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, - &ghlib.IssueComment{Body: ghlib.Ptr("/retest")}) + // Wait for the first pipeline run to be created + time.Sleep(5 * time.Second) + + // Get the name of the PipelineRun to retest specifically + prList, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{}) + assert.NilError(t, err) + assert.Assert(t, len(prList.Items) > 0, "Expected at least one PipelineRun to be created") + + pipelineRunName := "" + for _, pr := range prList.Items { + if originalName, ok := pr.GetAnnotations()[keys.OriginalPRName]; ok { + pipelineRunName = originalName + break + } + } + assert.Assert(t, pipelineRunName != "", "Could not find the original PipelineRun name") + + // Use retest with specific PipelineRun name + g.Cnx.Clients.Log.Infof("Creating /retest %s in PullRequest", pipelineRunName) + _, _, err = g.Provider.Client().Issues.CreateComment(ctx, g.Options.Organization, g.Options.Repo, g.PRNumber, + &ghlib.IssueComment{Body: ghlib.Ptr("/retest " + pipelineRunName)}) assert.NilError(t, err) g.Cnx.Clients.Log.Infof("Wait for the second repository update to be updated") diff --git a/test/gitlab_merge_request_test.go b/test/gitlab_merge_request_test.go index 8e542d2c7..aa8972d04 100644 --- a/test/gitlab_merge_request_test.go +++ b/test/gitlab_merge_request_test.go @@ -101,15 +101,32 @@ func TestGitlabMergeRequest(t *testing.T) { assert.Equal(t, "Merge Request", prsNew.Items[i].Annotations[keys.EventType]) } - runcnx.Clients.Log.Infof("Sending /retest comment on MergeRequest %s/-/merge_requests/%d", projectinfo.WebURL, mrID) + // Wait a moment to ensure all PipelineRuns are fully created + time.Sleep(5 * time.Second) + + // Query for an existing PipelineRun to retest specifically + prList, err := runcnx.Clients.Tekton.TektonV1().PipelineRuns(targetNS).List(ctx, metav1.ListOptions{}) + assert.NilError(t, err) + assert.Assert(t, len(prList.Items) > 0, "Expected at least one PipelineRun to be created") + + pipelineRunName := "" + for _, pr := range prList.Items { + if originalName, ok := pr.GetAnnotations()[keys.OriginalPRName]; ok { + pipelineRunName = originalName + break + } + } + assert.Assert(t, pipelineRunName != "", "Could not find the original PipelineRun name") + + runcnx.Clients.Log.Infof("Sending /retest %s comment on MergeRequest %s/-/merge_requests/%d", pipelineRunName, projectinfo.WebURL, mrID) _, _, err = glprovider.Client().Notes.CreateMergeRequestNote(opts.ProjectID, mrID, &clientGitlab.CreateMergeRequestNoteOptions{ - Body: clientGitlab.Ptr("/retest"), + Body: clientGitlab.Ptr("/retest " + pipelineRunName), }) assert.NilError(t, err) sopt = twait.SuccessOpt{ Title: commitTitle, - OnEvent: opscomments.RetestAllCommentEventType.String(), + OnEvent: opscomments.RetestSingleCommentEventType.String(), TargetNS: targetNS, NumberofPRMatch: 5, // this is the max we get in repos status SHA: "",