Add --keep to delete --all, to keep the last N pipelineruns #720

chmouel · 2020-02-12T21:44:59Z

Changes

Add a --keep to delete --all so we can keep N numbers. Users can set this up in
a cronjobs to clean old pipelineruns but keeping only the last ones.

Closes #718

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)
Run the code checkers with make check
Regenerate the manpages, docs and go formatting with make generated
Commit messages follow commit message best practices

See the contribution guide
for more details.

vdemeester

/meow

tekton-robot · 2020-02-13T07:34:53Z

@vdemeester:

In response to this:

/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.

danielhelfand

A couple things:

There needs to be a prompt specifically for --keep when not using -f. Currently it's what is shown below, but it should be a message about keeping a certain amount of pipelines:

Are you sure you want to delete all pipelineruns in namespace "default" (y/n):

This is the error message for if --keep is used without --all:

Error: must provide pipelineruns to delete or --pipeline flag

There should be a clearer error message.

danielhelfand · 2020-02-17T20:07:14Z

pkg/cmd/pipelinerun/delete.go

@@ -70,6 +71,7 @@ or
 	f.AddFlags(c)
 	c.Flags().BoolVarP(&opts.ForceDelete, "force", "f", false, "Whether to force deletion (default: false)")
 	c.Flags().StringVarP(&opts.ParentResourceName, "pipeline", "p", "", "The name of a pipeline whose pipelineruns should be deleted (does not delete the pipeline)")
+	c.Flags().IntVarP(&opts.Keep, "keep", "", 0, "Keep N number of pipelineruns when deleting alls")


Suggested change

c.Flags().IntVarP(&opts.Keep, "keep", "", 0, "Keep N number of pipelineruns when deleting alls")

c.Flags().IntVarP(&opts.Keep, "keep", "", 0, "keep n number of pipelineruns when using --all flag")

Would be nice in some way to indicate that ordering here is done by most recent pipelinerun.

humm usually i would ask to my english master 👨‍🏫 (ie you 😄 )

what about ?

Keep the last n number of pipelineruns?

keep n least recent number of pipelineruns sounds good to me.

danielhelfand · 2020-02-17T20:21:32Z

pkg/cmd/pipelinerun/delete.go

-	for _, pr := range pipelineRuns.Items {
+	var counter = 0
+	for _, pr := range prhsort.SortPipelineRunsByStartTime(pipelineRuns.Items) {
+		if keep > 0 && counter != keep {


Should there be some kind of error message if keep < 0? Currently, it will delete all pipelineruns.

in case If the user choose ?:

--last=-1

I guess that's quite a weird user case

Right, I mean it's totally unlikely, but we should prevent it by checking if keep < 0 similar to what is done for limit. So fail fast in RunE in this case.

@danielhelfand isn't the code above already checking that keep can't be < 0 ?

vdemeester

/lgtm

danielhelfand · 2020-02-27T08:13:04Z

/hold

There is some feedback that I think should be addressed in this pr still.

danielhelfand · 2020-02-27T11:10:46Z

A couple things:

There needs to be a prompt specifically for --keep when not using -f. Currently it's what is shown below, but it should be a message about keeping a certain amount of pipelines:

Are you sure you want to delete all pipelineruns in namespace "default" (y/n):

This is the error message for if --keep is used without --all:

Error: must provide pipelineruns to delete or --pipeline flag

There should be a clearer error message.

@vdemeester Less about the issue with keep < 0. More about the issues listed above.

vdemeester · 2020-02-27T11:18:01Z

@danielhelfand ah indeed

vdemeester · 2020-02-27T11:32:57Z

@danielhelfand taken into account 👼
/hold cancel

tekton-robot · 2020-02-27T11:37:50Z

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/helper/options/delete.go	95.8%	89.7%	-6.2

danielhelfand · 2020-02-27T14:08:33Z

pkg/cmd/pipelinerun/delete.go

@@ -80,13 +82,16 @@ func deletePipelineRuns(s *cli.Stream, p cli.Params, prNames []string, opts *opt
 	if err != nil {
 		return fmt.Errorf("failed to create tekton client")
 	}
+	if opts.Keep > 0 && !opts.DeleteAllNs {


This error message will only be reached if using -f. My recommendation would be to check earlier in RunE or Args to account for prompt situation:

If I run tkn pr delete --keep 7, I get the following:

Error: must provide pipelinerun name(s) or use --pipeline flag or --all flag to use delete

Closes tektoncd#718 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com> Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

Closes tektoncd#718 Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

danielhelfand · 2020-02-27T15:06:06Z

pkg/helper/options/delete.go

 }

 func (o *DeleteOptions) CheckOptions(s *cli.Stream, resourceNames []string, ns string) error {
+	if o.Keep > 0 && !(o.DeleteAllNs || o.DeleteAll) {
+		return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")


Promise this is the last thing. Could the error message be:

Suggested change

return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")

return fmt.Errorf("must use --all flag with --keep")

danielhelfand · 2020-02-27T15:06:21Z

pkg/helper/options/delete.go

 }

 func (o *DeleteOptions) CheckOptions(s *cli.Stream, resourceNames []string, ns string) error {
+	if o.Keep > 0 && !(o.DeleteAllNs || o.DeleteAll) {
+		return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")


Promise this is the last thing. Could the error message be:

Suggested change

return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")

return fmt.Errorf("must use --all flag with --keep")

danielhelfand · 2020-02-27T15:07:07Z

pkg/helper/options/delete.go

 }

 func (o *DeleteOptions) CheckOptions(s *cli.Stream, resourceNames []string, ns string) error {
+	if o.Keep > 0 && !(o.DeleteAllNs || o.DeleteAll) {
+		return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")


Promise this is the last thing. Could the error message be:

Suggested change

return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")

return fmt.Errorf("must use --all flag with --keep")

danielhelfand · 2020-02-27T15:07:23Z

pkg/helper/options/delete.go

 }

 func (o *DeleteOptions) CheckOptions(s *cli.Stream, resourceNames []string, ns string) error {
+	if o.Keep > 0 && !(o.DeleteAllNs || o.DeleteAll) {


Promise this is the last thing. Could the error message be:

Suggested change

if o.Keep > 0 && !(o.DeleteAllNs || o.DeleteAll) {

return fmt.Errorf("must use --all flag with --keep")

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2020-02-27T15:20:40Z

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/helper/options/delete.go	95.8%	87.1%	-8.7

danielhelfand

/lgtm

tekton-robot · 2020-02-27T15:26:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand, 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~~ [danielhelfand,vdemeester]

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

tekton-robot requested review from hrishin and piyush-garg February 12, 2020 21:45

googlebot added the cla: yes label Feb 12, 2020

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 12, 2020

vdemeester approved these changes Feb 13, 2020

View reviewed changes

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

danielhelfand suggested changes Feb 17, 2020

View reviewed changes

chmouel force-pushed the issue-718-add---limit-to-delete---all-for-pipelineruns-and-task branch from 9019171 to 6a2ec02 Compare February 19, 2020 14:55

vdemeester approved these changes Feb 27, 2020

View reviewed changes

tekton-robot assigned vdemeester Feb 27, 2020

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

vdemeester added this to the 0.8.0 🐯 milestone Feb 27, 2020

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2020

vdemeester force-pushed the issue-718-add---limit-to-delete---all-for-pipelineruns-and-task branch from 6a2ec02 to 7d18aeb Compare February 27, 2020 11:28

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2020

vdemeester force-pushed the issue-718-add---limit-to-delete---all-for-pipelineruns-and-task branch from 7d18aeb to f9310a5 Compare February 27, 2020 11:32

tektoncd deleted a comment from tekton-robot Feb 27, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2020

vdemeester force-pushed the issue-718-add---limit-to-delete---all-for-pipelineruns-and-task branch from f9310a5 to 87dc38e Compare February 27, 2020 11:34

danielhelfand reviewed Feb 27, 2020

View reviewed changes

chmouel and others added 2 commits February 27, 2020 15:25

Add --keep to --all, to keep the last N pipelineruns

3f79e4f

Closes tektoncd#718 Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com> Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

Add --keep to --all, to keep the last N taskruns

8270263

Closes tektoncd#718 Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester force-pushed the issue-718-add---limit-to-delete---all-for-pipelineruns-and-task branch 2 times, most recently from 3eed134 to 8270263 Compare February 27, 2020 14:36

danielhelfand reviewed Feb 27, 2020

View reviewed changes

Enhance the error message for --keep ⌨

ab2e8e0

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

vdemeester requested a review from danielhelfand February 27, 2020 15:22

danielhelfand approved these changes Feb 27, 2020

View reviewed changes

tekton-robot assigned danielhelfand Feb 27, 2020

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

tekton-robot merged commit 9a2c0be into tektoncd:master Feb 27, 2020

chmouel deleted the issue-718-add---limit-to-delete---all-for-pipelineruns-and-task branch March 2, 2020 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --keep to delete --all, to keep the last N pipelineruns #720

Add --keep to delete --all, to keep the last N pipelineruns #720

chmouel commented Feb 12, 2020

vdemeester left a comment

tekton-robot commented Feb 13, 2020

danielhelfand left a comment •

edited

Loading

danielhelfand Feb 17, 2020

danielhelfand Feb 17, 2020

chmouel Feb 18, 2020

danielhelfand Feb 18, 2020 •

edited

Loading

danielhelfand Feb 17, 2020

chmouel Feb 18, 2020

danielhelfand Feb 18, 2020

vdemeester Feb 27, 2020

vdemeester left a comment

danielhelfand commented Feb 27, 2020

danielhelfand commented Feb 27, 2020

vdemeester commented Feb 27, 2020

vdemeester commented Feb 27, 2020

tekton-robot commented Feb 27, 2020

danielhelfand Feb 27, 2020

danielhelfand Feb 27, 2020

danielhelfand Feb 27, 2020

danielhelfand Feb 27, 2020

danielhelfand Feb 27, 2020

tekton-robot commented Feb 27, 2020

danielhelfand left a comment

tekton-robot commented Feb 27, 2020

	c.Flags().IntVarP(&opts.Keep, "keep", "", 0, "Keep N number of pipelineruns when deleting alls")
	c.Flags().IntVarP(&opts.Keep, "keep", "", 0, "keep n number of pipelineruns when using --all flag")

	return fmt.Errorf("must provide pipelineruns to delete or --pipeline flag")
	return fmt.Errorf("must use --all flag with --keep")

	if o.Keep > 0 && !(o.DeleteAllNs \|\| o.DeleteAll) {
	return fmt.Errorf("must use --all flag with --keep")

Add --keep to delete --all, to keep the last N pipelineruns #720

Add --keep to delete --all, to keep the last N pipelineruns #720

Conversation

chmouel commented Feb 12, 2020

Changes

Submitter Checklist

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 13, 2020

danielhelfand left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhelfand Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

danielhelfand commented Feb 27, 2020

danielhelfand commented Feb 27, 2020

vdemeester commented Feb 27, 2020

vdemeester commented Feb 27, 2020

tekton-robot commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Feb 27, 2020

danielhelfand left a comment

Choose a reason for hiding this comment

tekton-robot commented Feb 27, 2020

danielhelfand left a comment •

edited

Loading

danielhelfand Feb 18, 2020 •

edited

Loading