New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix --ignore-running not working with -p/-t in delete cmd #1578
Conversation
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 tektoncd#1571
/retest |
{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "tr0-8", | ||
Namespace: "ns", | ||
Labels: map[string]string{"tekton.dev/task": "random"}, | ||
}, | ||
Spec: v1alpha1.TaskRunSpec{ | ||
TaskRef: &v1alpha1.TaskRef{ | ||
Name: "random", | ||
Kind: v1alpha1.NamespacedTaskKind, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can avoid adding tests for the v1alpha1 as they anyhow are converted to v1beta1. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now i added them, but if we want we can create an epic and remove in the whole codebase
{ | ||
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{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
name: "Delete all of task with default --ignore-running", | ||
command: []string{"delete", "-t", "random", "-f", "-n", "ns", "--ignore-running"}, | ||
dynamic: seeds[9].dynamicClient, | ||
input: seeds[9].pipelineClient, | ||
inputStream: nil, | ||
wantError: false, | ||
want: "All TaskRuns(Completed) associated with Task \"random\" deleted in namespace \"ns\"\n", | ||
}, | ||
{ | ||
name: "Delete all of task with explicit --ignore-running true", | ||
command: []string{"delete", "-t", "random", "-f", "-n", "ns", "--ignore-running=true"}, | ||
dynamic: seeds[10].dynamicClient, | ||
input: seeds[10].pipelineClient, | ||
inputStream: nil, | ||
wantError: false, | ||
want: "All TaskRuns(Completed) associated with Task \"random\" deleted in namespace \"ns\"\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to be duplicate as the default value of --ignore-running
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it to check the default behavior, if somehow the default behavior changes in future
name: "Delete all of clustertask with default --ignore-running", | ||
command: []string{"delete", "--clustertask", "random", "-f", "-n", "ns", "--ignore-running"}, | ||
dynamic: seeds[12].dynamicClient, | ||
input: seeds[12].pipelineClient, | ||
inputStream: nil, | ||
wantError: false, | ||
want: "All TaskRuns(Completed) associated with ClusterTask \"random\" deleted in namespace \"ns\"\n", | ||
}, | ||
{ | ||
name: "Delete all of clustertask with explicit --ignore-running true", | ||
command: []string{"delete", "--clustertask", "random", "-f", "-n", "ns", "--ignore-running=true"}, | ||
dynamic: seeds[13].dynamicClient, | ||
input: seeds[13].pipelineClient, | ||
inputStream: nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
name: "Delete all of task with default --ignore-running", | ||
command: []string{"delete", "-t", "random", "-f", "-n", "ns", "--ignore-running"}, | ||
dynamic: seeds[11].dynamicClient, | ||
input: seeds[11].pipelineClient, | ||
inputStream: nil, | ||
wantError: false, | ||
want: "All TaskRuns(Completed) associated with Task \"random\" deleted in namespace \"ns\"\n", | ||
}, | ||
{ | ||
name: "Delete all of task with explicit --ignore-running true", | ||
command: []string{"delete", "-t", "random", "-f", "-n", "ns", "--ignore-running=true"}, | ||
dynamic: seeds[12].dynamicClient, | ||
input: seeds[12].pipelineClient, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
name: "Delete all of clustertask with default --ignore-running", | ||
command: []string{"delete", "--clustertask", "random", "-f", "-n", "ns", "--ignore-running"}, | ||
dynamic: seeds[14].dynamicClient, | ||
input: seeds[14].pipelineClient, | ||
inputStream: nil, | ||
wantError: false, | ||
want: "All TaskRuns(Completed) associated with ClusterTask \"random\" deleted in namespace \"ns\"\n", | ||
}, | ||
{ | ||
name: "Delete all of clustertask with explicit --ignore-running true", | ||
command: []string{"delete", "--clustertask", "random", "-f", "-n", "ns", "--ignore-running=true"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pradeepitm12, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make check
make generated
See the contribution guide
for more details.
Release Notes