Skip to content

Commit

Permalink
Fix --ignore-running not working with -p/-t in delete cmd
Browse files Browse the repository at this point in the history 
This will add the fix to --ignore-running=true is not working with
-p/-t/--clustertask in delete cmd resulting in deleting all runs.

A bit of refactoring and adds unit tests

Fix #1571
  • Loading branch information
@piyush-garg @tekton-robot
piyush-garg authored and tekton-robot committed Jun 6, 2022
1 parent 9030e0c commit 1ce5288
Show file tree
Hide file tree
Showing 4 changed files with 529 additions and 42 deletions.
20 changes: 5 additions & 15 deletions pkg/cmd/pipelinerun/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ func deletePipelineRuns(s *cli.Stream, p cli.Params, prNames []string, opts *opt
return err
}
numberOfDeletedPr = len(prtodelete)
numberOfKeptPr = len(prtokeep)

// Delete the PipelineRuns associated with a Pipeline
d.WithRelated("PipelineRun", pipelineRunLister(cs, opts.Keep, opts.KeepSince, p.Namespace(), opts.IgnoreRunning), func(pipelineRunName string) error {
Expand Down Expand Up @@ -238,23 +239,12 @@ func deletePipelineRuns(s *cli.Stream, p cli.Params, prNames []string, opts *opt

func pipelineRunLister(cs *cli.Clients, keep, since int, ns string, ignoreRunning bool) func(string) ([]string, error) {
return func(pipelineName string) ([]string, error) {
lOpts := metav1.ListOptions{
LabelSelector: fmt.Sprintf("tekton.dev/pipeline=%s", pipelineName),
}
pipelineRuns, err := pr.List(cs, lOpts, ns)
labelSelector := fmt.Sprintf("tekton.dev/pipeline=%s", pipelineName)
prtodelete, _, err := allPipelineRunNames(cs, keep, since, ignoreRunning, labelSelector, ns)
if err != nil {
return nil, err
}
var todelete []string
switch {
case since > 0 && keep > 0:
todelete, _ = keepPipelineRunsByAgeAndNumber(pipelineRuns, since, keep, ignoreRunning)
case since > 0:
todelete, _ = keepPipelineRunsByAge(pipelineRuns, since, ignoreRunning)
default:
todelete, _ = keepPipelineRunsByNumber(pipelineRuns, keep)
}
return todelete, nil
return prtodelete, nil
}
}

Expand All @@ -277,7 +267,7 @@ func allPipelineRunNames(cs *cli.Clients, keep, since int, ignoreRunning bool, l
continue
}
for _, v2 := range v.Status.Conditions {
if v2.Reason == "Running" || v2.Reason == "Pending" {
if v2.Reason == "Running" || v2.Reason == "Pending" || v2.Reason == "PipelineRunPending" || v2.Reason == "Started" {
continue
}
pipelineRunTmps = append(pipelineRunTmps, v)
Expand Down
165 changes: 162 additions & 3 deletions pkg/cmd/pipelinerun/delete_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,56 @@ func TestPipelineRunDelete(t *testing.T) {
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "pipeline-run-4",
Namespace: "ns",
CreationTimestamp: metav1.Time{Time: clock.Now()},
Labels: map[string]string{"tekton.dev/pipeline": "pipeline"},
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: &v1alpha1.PipelineRef{
Name: "pipeline",
},
},
Status: v1beta1.PipelineRunStatus{
Status: duckv1beta1.Status{
Conditions: duckv1beta1.Conditions{
{
Status: corev1.ConditionTrue,
Reason: v1beta1.PipelineRunReasonPending.String(),
},
},
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "pipeline-run-5",
Namespace: "ns",
CreationTimestamp: metav1.Time{Time: clock.Now()},
Labels: map[string]string{"tekton.dev/pipeline": "pipeline"},
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: &v1alpha1.PipelineRef{
Name: "pipeline",
},
},
Status: v1beta1.PipelineRunStatus{
Status: duckv1beta1.Status{
Conditions: duckv1beta1.Conditions{
{
Status: corev1.ConditionTrue,
Reason: v1beta1.PipelineRunReasonRunning.String(),
},
},
},
PipelineRunStatusFields: v1alpha1.PipelineRunStatusFields{
// pipeline run starts now
StartTime: &metav1.Time{Time: time.Now()},
},
},
},
}

type clients struct {
Expand All @@ -182,7 +232,7 @@ func TestPipelineRunDelete(t *testing.T) {
}

seeds := make([]clients, 0)
for i := 0; i < 10; i++ {
for i := 0; i < 13; i++ {
cs, _ := test.SeedTestData(t, pipelinetest.Data{
Pipelines: pdata,
PipelineRuns: prdata,
Expand All @@ -195,6 +245,8 @@ func TestPipelineRunDelete(t *testing.T) {
cb.UnstructuredPR(prdata[1], version),
cb.UnstructuredPR(prdata[2], version),
cb.UnstructuredPR(prdata[3], version),
cb.UnstructuredPR(prdata[4], version),
cb.UnstructuredPR(prdata[5], version),
)
if err != nil {
t.Errorf("unable to create dynamic client: %v", err)
Expand Down Expand Up @@ -463,6 +515,33 @@ func TestPipelineRunDelete(t *testing.T) {
wantError: true,
want: "pipelineruns.tekton.dev \"nonexistent\" not found",
},
{
name: "Delete all of a pipeline by default ignore-running",
command: []string{"rm", "-p", "pipeline", "-n", "ns"},
dynamic: seeds[10].dynamicClient,
input: seeds[10].pipelineClient,
inputStream: nil,
wantError: false,
want: "Are you sure you want to delete all PipelineRuns related to Pipeline \"pipeline\" (y/n): All PipelineRuns(Completed) associated with Pipeline \"pipeline\" deleted in namespace \"ns\"\n",
},
{
name: "Delete all of a pipeline by explicit ignore-running true",
command: []string{"rm", "-p", "pipeline", "-n", "ns", "--ignore-running=true"},
dynamic: seeds[11].dynamicClient,
input: seeds[11].pipelineClient,
inputStream: nil,
wantError: false,
want: "Are you sure you want to delete all PipelineRuns related to Pipeline \"pipeline\" (y/n): All PipelineRuns(Completed) associated with Pipeline \"pipeline\" deleted in namespace \"ns\"\n",
},
{
name: "Delete all of a pipeline by ignore-running false",
command: []string{"rm", "-p", "pipeline", "-n", "ns", "--ignore-running=false"},
dynamic: seeds[12].dynamicClient,
input: seeds[12].pipelineClient,
inputStream: nil,
wantError: false,
want: "Are you sure you want to delete all PipelineRuns related to Pipeline \"pipeline\" (y/n): All PipelineRuns associated with Pipeline \"pipeline\" deleted in namespace \"ns\"\n",
},
}

for _, tp := range testParams {
Expand Down Expand Up @@ -616,6 +695,57 @@ func TestPipelineRunDelete_v1beta1(t *testing.T) {
},
Status: v1beta1.PipelineRunStatus{},
},
{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns",
Name: "pipeline-run-5",
Labels: map[string]string{"tekton.dev/pipeline": "pipeline"},
CreationTimestamp: metav1.Time{Time: clock.Now()},
},
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{
Name: "pipeline",
},
},
Status: v1beta1.PipelineRunStatus{
Status: duckv1beta1.Status{
Conditions: duckv1beta1.Conditions{
{
Status: corev1.ConditionTrue,
Reason: v1beta1.PipelineRunReasonPending.String(),
},
},
},
PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns",
Name: "pipeline-run-6",
Labels: map[string]string{"tekton.dev/pipeline": "pipeline"},
CreationTimestamp: metav1.Time{Time: clock.Now()},
},
Spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{
Name: "pipeline",
},
},
Status: v1beta1.PipelineRunStatus{
Status: duckv1beta1.Status{
Conditions: duckv1beta1.Conditions{
{
Status: corev1.ConditionTrue,
Reason: v1beta1.PipelineRunReasonRunning.String(),
},
},
},
PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
// pipeline run starts now
StartTime: &metav1.Time{Time: clock.Now()},
},
},
},
}

type clients struct {
Expand All @@ -624,7 +754,7 @@ func TestPipelineRunDelete_v1beta1(t *testing.T) {
}

seeds := make([]clients, 0)
for i := 0; i < 12; i++ {
for i := 0; i < 15; i++ {
cs, _ := test.SeedV1beta1TestData(t, pipelinev1beta1test.Data{
Pipelines: pdata,
PipelineRuns: prdata,
Expand All @@ -637,6 +767,8 @@ func TestPipelineRunDelete_v1beta1(t *testing.T) {
cb.UnstructuredV1beta1PR(prdata[1], version),
cb.UnstructuredV1beta1PR(prdata[2], version),
cb.UnstructuredV1beta1PR(prdata[3], version),
cb.UnstructuredV1beta1PR(prdata[4], version),
cb.UnstructuredV1beta1PR(prdata[5], version),
)
if err != nil {
t.Errorf("unable to create dynamic client: %v", err)
Expand Down Expand Up @@ -931,7 +1063,7 @@ func TestPipelineRunDelete_v1beta1(t *testing.T) {
input: seeds[9].pipelineClient,
inputStream: nil,
wantError: false,
want: "3 expired PipelineRuns has been deleted in namespace \"ns\", kept 0\n",
want: "5 expired PipelineRuns has been deleted in namespace \"ns\", kept 0\n",
},
{
name: "Delete all the pipelineruns including one without status",
Expand Down Expand Up @@ -960,6 +1092,33 @@ func TestPipelineRunDelete_v1beta1(t *testing.T) {
wantError: false,
want: "There is/are only 2 PipelineRun(s) associated for Pipeline: pipeline \n",
},
{
name: "Delete all of a pipeline by default ignore-running",
command: []string{"rm", "-p", "pipeline", "-n", "ns"},
dynamic: seeds[12].dynamicClient,
input: seeds[12].pipelineClient,
inputStream: nil,
wantError: false,
want: "Are you sure you want to delete all PipelineRuns related to Pipeline \"pipeline\" (y/n): All PipelineRuns(Completed) associated with Pipeline \"pipeline\" deleted in namespace \"ns\"\n",
},
{
name: "Delete all of a pipeline by explicit ignore-running true",
command: []string{"rm", "-p", "pipeline", "-n", "ns", "--ignore-running=true"},
dynamic: seeds[13].dynamicClient,
input: seeds[13].pipelineClient,
inputStream: nil,
wantError: false,
want: "Are you sure you want to delete all PipelineRuns related to Pipeline \"pipeline\" (y/n): All PipelineRuns(Completed) associated with Pipeline \"pipeline\" deleted in namespace \"ns\"\n",
},
{
name: "Delete all of a pipeline by ignore-running false",
command: []string{"rm", "-p", "pipeline", "-n", "ns", "--ignore-running=false"},
dynamic: seeds[14].dynamicClient,
input: seeds[14].pipelineClient,
inputStream: nil,
wantError: false,
want: "Are you sure you want to delete all PipelineRuns related to Pipeline \"pipeline\" (y/n): All PipelineRuns associated with Pipeline \"pipeline\" deleted in namespace \"ns\"\n",
},
}

for _, tp := range testParams {
Expand Down
38 changes: 16 additions & 22 deletions pkg/cmd/taskrun/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func deleteTaskRuns(s *cli.Stream, p cli.Params, trNames []string, opts *options
d = deleter.New("TaskRun", func(taskRunName string) error {
return actions.Delete(trGroupResource, cs.Dynamic, cs.Tekton.Discovery(), taskRunName, p.Namespace(), metav1.DeleteOptions{})
})
trToDelete, trToKeep, err := allTaskRunNames(cs, opts.Keep, opts.KeepSince, opts.IgnoreRunning, opts.LabelSelector, p.Namespace())
trToDelete, trToKeep, err := allTaskRunNames(cs, opts.Keep, opts.KeepSince, opts.IgnoreRunning, opts.LabelSelector, p.Namespace(), "")
if err != nil {
return err
}
Expand All @@ -192,18 +192,19 @@ func deleteTaskRuns(s *cli.Stream, p cli.Params, trNames []string, opts *options
labelSelector := fmt.Sprintf("tekton.dev/%s=%s", resourceType, opts.ParentResourceName)

// Compute the total no of TaskRuns which we need to delete
trToDelete, trToKeep, err := allTaskRunNames(cs, opts.Keep, opts.KeepSince, opts.IgnoreRunning, labelSelector, p.Namespace())
trToDelete, trToKeep, err := allTaskRunNames(cs, opts.Keep, opts.KeepSince, opts.IgnoreRunning, labelSelector, p.Namespace(), opts.ParentResource)
if err != nil {
return err
}
numberOfDeletedTr = len(trToDelete)
numberOfKeptTr = len(trToKeep)

// Delete the TaskRuns associated with a Task or ClusterTask
d.WithRelated("TaskRun", taskRunLister(p, opts.Keep, opts.KeepSince, opts.ParentResource, cs, opts.IgnoreRunning), func(taskRunName string) error {
return actions.Delete(trGroupResource, cs.Dynamic, cs.Tekton.Discovery(), taskRunName, p.Namespace(), metav1.DeleteOptions{})
})
if len(trToDelete) == 0 && opts.Keep > len(trToKeep) {
fmt.Fprintf(s.Out, "There is/are only %d %s(s) associated for Task: %s \n", len(trToKeep), opts.Resource, opts.ParentResourceName)
fmt.Fprintf(s.Out, "There is/are only %d %s(s) associated for %s: %s \n", len(trToKeep), opts.Resource, opts.ParentResource, opts.ParentResourceName)
return nil
}
d.DeleteRelated(s, []string{opts.ParentResourceName})
Expand Down Expand Up @@ -266,30 +267,16 @@ func taskRunLister(p cli.Params, keep, since int, kind string, cs *cli.Clients,
label = "clusterTask"
}

lOpts := metav1.ListOptions{
LabelSelector: fmt.Sprintf("tekton.dev/%s=%s", label, taskName),
}
trs, err := trlist.TaskRuns(cs, lOpts, p.Namespace())
labelSelector := fmt.Sprintf("tekton.dev/%s=%s", label, taskName)
trToDelete, _, err := allTaskRunNames(cs, keep, since, ignoreRunning, labelSelector, p.Namespace(), kind)
if err != nil {
return nil, err
}
if kind == "Task" {
trs.Items = taskpkg.FilterByRef(trs.Items, string(v1beta1.NamespacedTaskKind))
}
var todelete []string
switch {
case since > 0 && keep > 0:
todelete, _ = keepTaskRunsByAgeAndNumber(trs, since, keep, ignoreRunning)
case since > 0:
todelete, _ = keepTaskRunsByAge(trs, since, ignoreRunning)
default:
todelete, _ = keepTaskRunsByNumber(trs, keep)
}
return todelete, nil
return trToDelete, nil
}
}

func allTaskRunNames(cs *cli.Clients, keep, since int, ignoreRunning bool, labelSelector, ns string) ([]string, []string, error) {
func allTaskRunNames(cs *cli.Clients, keep, since int, ignoreRunning bool, labelSelector, ns, kind string) ([]string, []string, error) {
var todelete, tokeep []string

taskRuns, err := trlist.TaskRuns(cs, metav1.ListOptions{
Expand All @@ -299,11 +286,18 @@ func allTaskRunNames(cs *cli.Clients, keep, since int, ignoreRunning bool, label
return todelete, tokeep, err
}

if kind == "Task" {
taskRuns.Items = taskpkg.FilterByRef(taskRuns.Items, string(v1beta1.NamespacedTaskKind))
}

if ignoreRunning {
var taskRunTmp = []v1beta1.TaskRun{}
for _, v := range taskRuns.Items {
if v.Status.Conditions == nil {
continue
}
for _, v2 := range v.Status.Conditions {
if v2.Reason == "Running" || v2.Reason == "Pending" {
if v2.Reason == "Running" || v2.Reason == "Pending" || v2.Reason == "Started" {
continue
}
taskRunTmp = append(taskRunTmp, v)
Expand Down
Loading

0 comments on commit 1ce5288

Please sign in to comment.