Add flag to task and pipeline to only delete related resources #608

ghost · 2020-01-15T16:14:02Z

Changes

Fixes #589

This commit adds a flag to delete resources related to a parent - a
taskrun's parent is a task, a pipelinerun's parent is a pipeline -
without deleting the task or pipeline.

The flags are:
tkn taskrun delete --task task-name
tkn pipelinerun delete --pipeline pipeline-name

TODO:

Add unit tests for the rewritten flags --task and --pipeline

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Regenerate the manpages and docs with make docs and make man if needed.
Run the code checkers with make check
Commit messages follow commit message best practices

Release Notes

tkn delete pipeline --delete-pipelineruns flag has been added to allow deletion of only the pipelineruns related to a pipeline (not the pipeline itself)
tkn delete task --delete-taskruns flag has been added to allow deletion of only the taskruns related to a task (not the task itself)

ghost · 2020-01-15T16:15:28Z

I wonder if the flags should be --only-pipelineruns and --only-taskruns instead of --delete-pipelineruns and --delete-taskruns. Anyone got an opinion on that?

tekton-robot · 2020-01-15T16:29:12Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/clustertask/delete.go	94.4%	95.0%	0.6
pkg/cmd/condition/delete.go	95.0%	95.5%	0.5
pkg/cmd/eventlistener/delete.go	95.0%	95.5%	0.5
pkg/cmd/pipeline/delete.go	93.9%	94.9%	0.9
pkg/cmd/pipelinerun/delete.go	95.0%	95.5%	0.5
pkg/cmd/task/delete.go	93.9%	94.9%	0.9
pkg/cmd/taskrun/delete.go	95.0%	95.5%	0.5
pkg/cmd/triggerbinding/delete.go	95.0%	95.5%	0.5
pkg/cmd/triggertemplate/delete.go	95.0%	95.5%	0.5
pkg/helper/deleter/deleter.go	50.0%	100.0%	50.0
pkg/helper/options/delete.go	93.3%	87.5%	-5.8

chmouel · 2020-01-15T16:43:44Z

tkn pipeline delete --only-runs ? but that may be too cryptic,

I rather have the --only-* prefix which makes more sense imho

danielhelfand · 2020-01-15T16:47:37Z

tkn pipeline delete --only-runs ? but that may be too cryptic,

I rather have the --only-* prefix which makes more sense imho

--only is more clear to me that you aren't deleting anything else. I do prefer --only-pipelineruns/--only-taskruns to be more explicit.

ghost · 2020-01-15T18:46:36Z

ok great, I've gone with --only-taskruns and --only-pipelineruns, thanks folks

tekton-robot · 2020-01-15T18:57:44Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/clustertask/delete.go	94.4%	95.0%	0.6
pkg/cmd/condition/delete.go	95.0%	95.5%	0.5
pkg/cmd/eventlistener/delete.go	95.0%	95.5%	0.5
pkg/cmd/pipeline/delete.go	93.9%	94.9%	0.9
pkg/cmd/pipelinerun/delete.go	95.0%	95.5%	0.5
pkg/cmd/task/delete.go	93.9%	94.9%	0.9
pkg/cmd/taskrun/delete.go	95.0%	95.5%	0.5
pkg/cmd/triggerbinding/delete.go	95.0%	95.5%	0.5
pkg/cmd/triggertemplate/delete.go	95.0%	95.5%	0.5
pkg/helper/deleter/deleter.go	50.0%	100.0%	50.0
pkg/helper/options/delete.go	93.3%	87.5%	-5.8

tekton-robot · 2020-01-15T19:45:02Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/clustertask/delete.go	94.4%	95.0%	0.6
pkg/cmd/condition/delete.go	95.0%	95.5%	0.5
pkg/cmd/eventlistener/delete.go	95.0%	95.5%	0.5
pkg/cmd/pipeline/delete.go	93.9%	94.9%	0.9
pkg/cmd/pipelineresource/delete.go	95.0%	95.5%	0.5
pkg/cmd/pipelinerun/delete.go	95.0%	95.5%	0.5
pkg/cmd/task/delete.go	93.9%	94.9%	0.9
pkg/cmd/taskrun/delete.go	95.0%	95.5%	0.5
pkg/cmd/triggerbinding/delete.go	95.0%	95.5%	0.5
pkg/cmd/triggertemplate/delete.go	95.0%	95.5%	0.5
pkg/helper/deleter/deleter.go	50.0%	100.0%	50.0
pkg/helper/options/delete.go	93.3%	93.8%	0.4

vdemeester · 2020-01-16T09:47:45Z

So… I think we need to think about this a little bit more 🤔 We have something a little bit like that when listing TaskRun and PipelineRun.

# List all taskruns of task 'foo' in namespace 'bar'
tkn taskrun list foo -n bar

This is in taskrun subcommand. You can filter the TaskRun of a given Task.

To be consistent, I would think we could have this on delete too, as tkn taskrun delete --task=foo or something ?
To be even more consitent, tkn taskrun list foo should be deprecated (but still supported for a while), and use a flag instead (the same as for delete, so tkn taskrun list --task=foo) ?

This would also allow to delete all TaskRun for multiple Tasks, with tkn taskrun delete --task=foo --task=bar.

@chmouel @piyush-garg @danielhelfand @sbwsg wdyt ?

chmouel · 2020-01-16T10:16:03Z

To be consistent, I would think we could have this on delete too, as tkn taskrun delete --task=foo or something ?

IMHO I think that's less than intuitive... I like how @sbwsg implemented it, where I can tell to myself :

"I want to delete all the auto created taskruns attached to my task named foo"

and map it into a command that looks as what i am thinking :

tkn delete task foo --delete-taskruns

your suggested command :

tkn taskrun delete --task=foo

looks not intuitive to me.. (and we loose completion)

To be even more consitent, tkn taskrun list foo should be deprecated (but still supported for a while), and use a flag instead (the same as for delete, so tkn taskrun list --task=foo) ?

👎 on this, even tho this may look more logical, this is not going to be intuitive for our end users

vdemeester · 2020-01-16T10:46:02Z

🤔 so tkn taskrun list foo and tkn task delete --delete-taskruns are intuitive but tkn taskrun delete --task=foo is not ?

It might be just me, but as a user, if I want to delete all TaskRuns from a Task I would naively go to tkn taskrun subcommands than tkn task subcommands (aka I would look for a filtering option on the TaskRun delete command). This is what would feel intuitive for me 😛

chmouel · 2020-01-16T10:56:13Z

Because you are deep neck into the project and you know what a run is 🤣

We don't necessary expose the *Runs with CLI, we just say here is a task/pipeline and start it(tkn task start)....

we give a taskrun objects so we can do stuff on it... but it really start from the task for the user...

vdemeester · 2020-01-16T13:53:07Z

Because you are deep neck into the project and you know what a run is rofl

We don't necessary expose the *Runs with CLI, we just say here is a task/pipeline and start it(tkn task start)....

we give a taskrun objects so we can do stuff on it... but it really start from the task for the user...

right, that might make sense 👼 but then should the flag be just about runs or even a subtask 😈 ? (tkn task delete runs)

chmouel · 2020-01-16T14:07:25Z

? (tkn task delete runs)

I am okay with that! but i'll let others weight in!

ghost · 2020-01-16T15:12:11Z

I gotta be honest I don't feel strongly either way - do we want both?

tkn taskrun delete --task my-task
tkn task delete runs my-task

My eye likes the syntax of the first one better. But I agree that users are task-centric, not so much taskrun-centric.

@danielhelfand do you have any thoughts here from when you initially created the issue?

ghost · 2020-01-16T15:13:02Z

Maybe we get even more fancy:

tkn taskrun delete --filter "task=my-task"?

chmouel · 2020-01-16T20:29:16Z

Good idea @sbwsg !

I am +1 on the filter thing... and we can add it to tkn * list too... (ie: #617)

I think people are already used to the label selection from kubectl so this may transpose well..

vdemeester · 2020-01-17T08:52:51Z

As @chmouel, I like the fancy solution 😛

siamaksade · 2020-01-17T13:40:16Z

Filtering with explicit task and pipelinerun flags IMO is more intuitive. The key=value syntax incorrectly implies that it's similar to k8s labels:
tkn taskrun delete --task tname
tkn taskrun delete --pipelinerun prname

Same would go for the list:
tkn taskrun list --task tname
tkn taskrun list --pipelinerun prname

Could make sense to support the same syntax for the pipelinerun too in addition to the existing syntax:
tkn pipelinerun list pname
tkn pipelinerun list --pipeline pname

I'm opposed to indirect delete commands like:
tkn delete task tname --delete-taskruns

The command reads delete tasks except that it doesn't do that, it deletes taskruns.

ghost · 2020-01-17T16:57:43Z

Great feedback, thanks @siamaksade, you make some excellent points. I think my next step here is to bring this up at the next CLI WG.

ghost · 2020-01-21T13:08:40Z

OK cool, after quick chat in the working group, we're going to go with tkn taskrun delete --task tname style syntax

tekton-robot · 2020-01-24T21:02:57Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/delete.go	93.9%	94.4%	0.5
pkg/cmd/task/delete.go	93.9%	94.4%	0.5
pkg/helper/deleter/deleter.go	50.0%	100.0%	50.0
pkg/helper/options/delete.go	93.3%	93.8%	0.4

tekton-robot · 2020-01-27T16:30:26Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/delete.go	93.9%	94.4%	0.5
pkg/cmd/task/delete.go	93.9%	94.4%	0.5
pkg/helper/deleter/deleter.go	50.0%	100.0%	50.0
pkg/helper/options/delete.go	93.3%	88.9%	-4.4

danielhelfand · 2020-01-28T02:17:40Z

Failures are -- FATAL: The documentation or manpages didn't seem to be generated :

Just need to update docs/manpages.

TaskRun and PipelineRun delete commands provide a mechanism to delete both the taskruns or pipelineruns related to a task or pipeline. This commit adds a flag to delete resources related to a parent - a taskrun's parent is a task, a pipelinerun's parent is a pipeline - without deleting the task or pipeline. The flags are: `tkn taskrun delete --task task-name` `tkn pipelinerun delete --pipeline pipeline-name`

tekton-robot · 2020-01-28T13:46:57Z

The following is the coverage report on pkg/.
Say /test pull-tekton-cli-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/cmd/pipeline/delete.go	93.9%	94.4%	0.5
pkg/cmd/task/delete.go	93.9%	94.4%	0.5
pkg/helper/deleter/deleter.go	50.0%	100.0%	50.0
pkg/helper/options/delete.go	93.3%	88.9%	-4.4

vdemeester · 2020-01-28T14:25:31Z

It is green 🎉
/meow

tekton-robot · 2020-01-28T14:25:33Z

@vdemeester:

In response to this:

It is green 🎉
/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester

LGTM 🎉

tekton-robot · 2020-01-28T14:41:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chmouel · 2020-01-28T14:47:14Z

/lgtm

🔥

ghost · 2020-01-28T14:48:51Z

🙏 woo, cheers!

googlebot added the cla: yes label Jan 15, 2020

tekton-robot requested review from chmouel and hrishin January 15, 2020 16:14

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2020

vdemeester approved these changes Jan 28, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2020

tekton-robot assigned chmouel Jan 28, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2020

tekton-robot merged commit 204b8f7 into tektoncd:master Jan 28, 2020

chmouel mentioned this pull request Feb 5, 2020

Objects filtering #684

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to task and pipeline to only delete related resources #608

Add flag to task and pipeline to only delete related resources #608

ghost commented Jan 15, 2020 •

edited by ghost

Loading

ghost commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

chmouel commented Jan 15, 2020 •

edited

Loading

danielhelfand commented Jan 15, 2020

ghost commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

vdemeester commented Jan 16, 2020

chmouel commented Jan 16, 2020 •

edited

Loading

vdemeester commented Jan 16, 2020

chmouel commented Jan 16, 2020 •

edited

Loading

vdemeester commented Jan 16, 2020

chmouel commented Jan 16, 2020

ghost commented Jan 16, 2020 •

edited by ghost

Loading

ghost commented Jan 16, 2020

chmouel commented Jan 16, 2020

vdemeester commented Jan 17, 2020

siamaksade commented Jan 17, 2020

ghost commented Jan 17, 2020

ghost commented Jan 21, 2020

tekton-robot commented Jan 24, 2020

tekton-robot commented Jan 27, 2020

danielhelfand commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

vdemeester commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

vdemeester left a comment

tekton-robot commented Jan 28, 2020

chmouel commented Jan 28, 2020

ghost commented Jan 28, 2020

Add flag to task and pipeline to only delete related resources #608

Add flag to task and pipeline to only delete related resources #608

Conversation

ghost commented Jan 15, 2020 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

ghost commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

chmouel commented Jan 15, 2020 • edited Loading

danielhelfand commented Jan 15, 2020

ghost commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

tekton-robot commented Jan 15, 2020

vdemeester commented Jan 16, 2020

chmouel commented Jan 16, 2020 • edited Loading

vdemeester commented Jan 16, 2020

chmouel commented Jan 16, 2020 • edited Loading

vdemeester commented Jan 16, 2020

chmouel commented Jan 16, 2020

ghost commented Jan 16, 2020 • edited by ghost Loading

ghost commented Jan 16, 2020

chmouel commented Jan 16, 2020

vdemeester commented Jan 17, 2020

siamaksade commented Jan 17, 2020

ghost commented Jan 17, 2020

ghost commented Jan 21, 2020

tekton-robot commented Jan 24, 2020

tekton-robot commented Jan 27, 2020

danielhelfand commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

vdemeester commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 28, 2020

chmouel commented Jan 28, 2020

ghost commented Jan 28, 2020

ghost commented Jan 15, 2020 •

edited by ghost

Loading

chmouel commented Jan 15, 2020 •

edited

Loading

chmouel commented Jan 16, 2020 •

edited

Loading

chmouel commented Jan 16, 2020 •

edited

Loading

ghost commented Jan 16, 2020 •

edited by ghost

Loading