Repository: openshift-pipelines/pipelines-as-code
Pull request #248 (2021-09-30 06:30:01)

Fix max-keep-run
We were getting the oldest one first and skipping when that should have
been the other way around.

Add test for it.

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>



Patch:
diff --git a/.tekton/pull-request.yaml b/.tekton/pull-request.yaml
index ac20953d..e4be40e9 100644
--- a/.tekton/pull-request.yaml
+++ b/.tekton/pull-request.yaml
@@ -7,6 +7,7 @@ metadata:
     pipelinesascode.tekton.dev/on-event: "[pull_request]"
     pipelinesascode.tekton.dev/on-target-branch: "[main]"
     pipelinesascode.tekton.dev/task: "[git-clone]"
+    pipelinesascode.tekton.dev/max-keep-runs: "2"
 spec:
   params:
     - name: repo_url
diff --git a/pkg/kubeinteraction/cleanups.go b/pkg/kubeinteraction/cleanups.go
index 0089c66e..e35f0e65 100644
--- a/pkg/kubeinteraction/cleanups.go
+++ b/pkg/kubeinteraction/cleanups.go
@@ -32,7 +32,7 @@ func (k Interaction) CleanupPipelines(ctx context.Context, namespace string, rep
 		LabelSelector: labelSelector,
 	})
 
-	sort.Sort(prByCompletionTime(pruns.Items))
+	sort.Sort(sort.Reverse(prByCompletionTime(pruns.Items)))
 
 	for c, v := range pruns.Items {
 		if v.GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Running" {


Test patch:
diff --git a/pkg/kubeinteraction/cleanups_test.go b/pkg/kubeinteraction/cleanups_test.go
new file mode 100644
index 00000000..536ecdb5
--- /dev/null
+++ b/pkg/kubeinteraction/cleanups_test.go
@@ -0,0 +1,210 @@
+package kubeinteraction
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/jonboulle/clockwork"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"go.uber.org/zap"
+	zapobserver "go.uber.org/zap/zaptest/observer"
+	"gotest.tools/v3/assert"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	knativeapi "knative.dev/pkg/apis"
+	duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
+	rtesting "knative.dev/pkg/reconciler/testing"
+)
+
+func TestInteraction_CleanupPipelines(t *testing.T) {
+	ns := "namespace"
+	clock := clockwork.NewFakeClock()
+
+	type args struct {
+		namespace      string
+		repositoryName string
+		maxKeep        int
+		pruns          []*v1beta1.PipelineRun
+		kept           int
+	}
+
+	const cleanupRepoName = "clean-me-up-before-you-go-go"
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "cleanup",
+			args: args{
+				namespace:      ns,
+				repositoryName: cleanupRepoName,
+				maxKeep:        1,
+				kept:           1,
+				pruns: []*v1beta1.PipelineRun{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pipeline-newest",
+							Namespace: ns,
+							Labels: map[string]string{
+								"pipelinesascode.tekton.dev/repository": cleanupRepoName,
+							},
+						},
+						Status: v1beta1.PipelineRunStatus{
+							PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+								StartTime:      &metav1.Time{Time: clock.Now().Add(-5 * time.Minute)},
+								CompletionTime: &metav1.Time{Time: clock.Now().Add(10 * time.Minute)},
+							},
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pipeline-middest",
+							Namespace: ns,
+							Labels: map[string]string{
+								"pipelinesascode.tekton.dev/repository": cleanupRepoName,
+							},
+						},
+						Status: v1beta1.PipelineRunStatus{
+							PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+								StartTime:      &metav1.Time{Time: clock.Now().Add(-15 * time.Minute)},
+								CompletionTime: &metav1.Time{Time: clock.Now().Add(20 * time.Minute)},
+							},
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pipeline-oldest",
+							Namespace: ns,
+							Labels: map[string]string{
+								"pipelinesascode.tekton.dev/repository": cleanupRepoName,
+							},
+						},
+						Status: v1beta1.PipelineRunStatus{
+							PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+								StartTime:      &metav1.Time{Time: clock.Now().Add(-25 * time.Minute)},
+								CompletionTime: &metav1.Time{Time: clock.Now().Add(30 * time.Minute)},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "cleanup-skip-running",
+			args: args{
+				namespace:      ns,
+				repositoryName: cleanupRepoName,
+				maxKeep:        1,
+				kept:           2,
+				pruns: []*v1beta1.PipelineRun{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pipeline-newest",
+							Namespace: ns,
+							Labels: map[string]string{
+								"pipelinesascode.tekton.dev/repository": cleanupRepoName,
+							},
+						},
+						Status: v1beta1.PipelineRunStatus{
+
+							PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+								// pipeline run started 5 minutes ago
+								StartTime: &metav1.Time{Time: clock.Now().Add(-5 * time.Minute)},
+								// takes 10 minutes to complete
+								CompletionTime: &metav1.Time{Time: clock.Now().Add(10 * time.Minute)},
+							},
+						},
+					},
+
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pipeline-oldest",
+							Namespace: ns,
+							Labels: map[string]string{
+								"pipelinesascode.tekton.dev/repository": cleanupRepoName,
+							},
+						},
+						Status: v1beta1.PipelineRunStatus{
+							PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+								// pipeline run started 5 minutes ago
+								StartTime: &metav1.Time{Time: clock.Now().Add(-25 * time.Minute)},
+								// takes 10 minutes to complete
+								CompletionTime: &metav1.Time{Time: clock.Now().Add(30 * time.Minute)},
+							},
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "pipeline-midest",
+							Namespace: ns,
+							Labels: map[string]string{
+								"pipelinesascode.tekton.dev/repository": cleanupRepoName,
+							},
+						},
+						Status: v1beta1.PipelineRunStatus{
+							Status: duckv1beta1.Status{
+								Conditions: duckv1beta1.Conditions{
+									{
+										Type:   knativeapi.ConditionSucceeded,
+										Status: corev1.ConditionTrue,
+										Reason: v1beta1.PipelineRunReasonRunning.String(),
+									},
+								},
+							},
+							PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+								// pipeline run started 5 minutes ago
+								StartTime: &metav1.Time{Time: clock.Now().Add(-15 * time.Minute)},
+								// takes 10 minutes to complete
+								CompletionTime: &metav1.Time{Time: clock.Now().Add(20 * time.Minute)},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, _ := rtesting.SetupFakeContext(t)
+			tdata := testclient.Data{
+				PipelineRuns: tt.args.pruns,
+				Namespaces: []*corev1.Namespace{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: ns,
+						},
+					},
+				},
+			}
+			stdata, _ := testclient.SeedTestData(t, ctx, tdata)
+			observer, _ := zapobserver.New(zap.InfoLevel)
+			fakelogger := zap.New(observer).Sugar()
+			kint := Interaction{
+				Run: &params.Run{
+					Clients: clients.Clients{
+						Kube:   stdata.Kube,
+						Log:    fakelogger,
+						Tekton: stdata.Pipeline,
+					},
+				},
+			}
+
+			if err := kint.CleanupPipelines(ctx, tt.args.namespace, tt.args.repositoryName,
+				tt.args.maxKeep); (err != nil) != tt.wantErr {
+				t.Errorf("CleanupPipelines() error = %v, wantErr %v", err, tt.wantErr)
+			}
+
+			plist, err := kint.Run.Clients.Tekton.TektonV1beta1().PipelineRuns(tt.args.namespace).List(ctx,
+				metav1.ListOptions{})
+			assert.NilError(t, err)
+			assert.Equal(t, len(plist.Items), tt.args.kept)
+			assert.Equal(t, plist.Items[len(plist.Items)-1].Name, tt.args.pruns[0].Name,
+				fmt.Sprintf("%s != %s", plist.Items[0].Name, tt.args.pruns[0].Name))
+		})
+	}
+}
diff --git a/pkg/kubeinteraction/namespace_test.go b/pkg/kubeinteraction/namespace_test.go
new file mode 100644
index 00000000..7179001f
--- /dev/null
+++ b/pkg/kubeinteraction/namespace_test.go
@@ -0,0 +1,61 @@
+package kubeinteraction
+
+import (
+	"testing"
+
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	"go.uber.org/zap"
+	zapobserver "go.uber.org/zap/zaptest/observer"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	rtesting "knative.dev/pkg/reconciler/testing"
+)
+
+func TestInteraction_GetNamespace(t *testing.T) {
+	tests := []struct {
+		name     string
+		targetNS string
+		logMsg   string
+		wantErr  bool
+	}{
+		{
+			name:     "error when namespace is not there",
+			targetNS: "not existing",
+			logMsg:   "namespace is",
+			wantErr:  true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, _ := rtesting.SetupFakeContext(t)
+			tdata := testclient.Data{
+				Namespaces: []*corev1.Namespace{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: tt.targetNS,
+							// TODO: it's not working ? go figure out why! It doesn't actually create a NS here
+						},
+					},
+				},
+			}
+
+			stdata, _ := testclient.SeedTestData(t, ctx, tdata)
+			observer, _ := zapobserver.New(zap.InfoLevel)
+			fakelogger := zap.New(observer).Sugar()
+			kint := Interaction{
+				Run: &params.Run{
+					Clients: clients.Clients{
+						Kube: stdata.Kube,
+						Log:  fakelogger,
+					},
+				},
+			}
+
+			if err := kint.GetNamespace(ctx, tt.targetNS); (err != nil) != tt.wantErr {
+				t.Errorf("GetNamespace() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}