From 82ee9a2c75a0b1dcd942a2c18da41f4d45b81698 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Wed, 29 Jan 2025 14:53:09 +0500 Subject: [PATCH 1/9] [gh cache delete --all] Add `--succeed-on-no-caches` flag to return exit code 0 --- pkg/cmd/cache/delete/delete.go | 33 +++++++++++++++++++++++------ pkg/cmd/cache/delete/delete_test.go | 16 ++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 65a9d696a..81138ddd0 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -22,8 +22,9 @@ type DeleteOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - DeleteAll bool - Identifier string + DeleteAll bool + SucceedOnNoCaches bool + Identifier string } func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Command { @@ -33,7 +34,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co } cmd := &cobra.Command{ - Use: "delete [| | --all]", + Use: "delete [ | | --all]", Short: "Delete GitHub Actions caches", Long: heredoc.Docf(` Delete GitHub Actions caches. @@ -50,8 +51,11 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co # Delete a cache by id in a specific repo $ gh cache delete 1234 --repo cli/cli - # Delete all caches + # Delete all caches (exit code 1 on no caches) $ gh cache delete --all + + # Delete all caches (exit code 0 on no caches) + $ gh cache delete --all --succeed-on-no-caches `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { @@ -65,6 +69,10 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co return err } + if opts.SucceedOnNoCaches && (!opts.DeleteAll || len(args) == 1) { + return cmdutil.FlagErrorf("--succeed-on-no-caches must be used in conjunction with --all") + } + if !opts.DeleteAll && len(args) == 0 { return cmdutil.FlagErrorf("must provide either cache id, cache key, or use --all") } @@ -82,6 +90,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co } cmd.Flags().BoolVarP(&opts.DeleteAll, "all", "a", false, "Delete all caches") + cmd.Flags().BoolVarP(&opts.SucceedOnNoCaches, "succeed-on-no-caches", "s", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`") return cmd } @@ -100,12 +109,24 @@ func deleteRun(opts *DeleteOptions) error { var toDelete []string if opts.DeleteAll { + opts.IO.StartProgressIndicator() caches, err := shared.GetCaches(client, repo, shared.GetCachesOptions{Limit: -1}) + opts.IO.StopProgressIndicator() if err != nil { - return err + if opts.SucceedOnNoCaches { + fmt.Println(err) + return nil + } else { + return err + } } if len(caches.ActionsCaches) == 0 { - return fmt.Errorf("%s No caches to delete", opts.IO.ColorScheme().FailureIcon()) + if opts.SucceedOnNoCaches { + fmt.Printf("%s No caches to delete\n", opts.IO.ColorScheme().SuccessIcon()) + return nil + } else { + return fmt.Errorf("%s No caches to delete", opts.IO.ColorScheme().FailureIcon()) + } } for _, cache := range caches.ActionsCaches { toDelete = append(toDelete, strconv.Itoa(cache.Id)) diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index 43fc7d213..e81fdbcec 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -43,6 +43,21 @@ func TestNewCmdDelete(t *testing.T) { cli: "--all", wants: DeleteOptions{DeleteAll: true}, }, + { + name: "delete all and succeed-on-no-caches flags", + cli: "--all --succeed-on-no-caches", + wants: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true}, + }, + { + name: "succeed-on-no-caches flag", + cli: "--succeed-on-no-caches", + wantsErr: "--succeed-on-no-caches must be used in conjunction with --all", + }, + { + name: "succeed-on-no-caches flag and id argument", + cli: "--succeed-on-no-caches 123", + wantsErr: "--succeed-on-no-caches must be used in conjunction with --all", + }, { name: "id argument and delete all flag", cli: "1 --all", @@ -72,6 +87,7 @@ func TestNewCmdDelete(t *testing.T) { } assert.NoError(t, err) assert.Equal(t, tt.wants.DeleteAll, gotOpts.DeleteAll) + assert.Equal(t, tt.wants.SucceedOnNoCaches, gotOpts.SucceedOnNoCaches) assert.Equal(t, tt.wants.Identifier, gotOpts.Identifier) }) } From d6be4981be31da2a2365df0ffbb58163c66ce130 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Fri, 31 Jan 2025 12:28:00 +0500 Subject: [PATCH 2/9] Address PR review comments --- pkg/cmd/cache/delete/delete.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 81138ddd0..e244cc0a1 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -69,7 +69,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co return err } - if opts.SucceedOnNoCaches && (!opts.DeleteAll || len(args) == 1) { + if !opts.DeleteAll && opts.SucceedOnNoCaches { return cmdutil.FlagErrorf("--succeed-on-no-caches must be used in conjunction with --all") } @@ -113,16 +113,11 @@ func deleteRun(opts *DeleteOptions) error { caches, err := shared.GetCaches(client, repo, shared.GetCachesOptions{Limit: -1}) opts.IO.StopProgressIndicator() if err != nil { - if opts.SucceedOnNoCaches { - fmt.Println(err) - return nil - } else { - return err - } + return err } if len(caches.ActionsCaches) == 0 { if opts.SucceedOnNoCaches { - fmt.Printf("%s No caches to delete\n", opts.IO.ColorScheme().SuccessIcon()) + fmt.Fprintf(opts.IO.Out, "%s No caches to delete\n", opts.IO.ColorScheme().SuccessIcon()) return nil } else { return fmt.Errorf("%s No caches to delete", opts.IO.ColorScheme().FailureIcon()) From 9e13890c77ac65d1b06b0032b4444e0caaf70583 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Thu, 6 Feb 2025 11:56:08 +0500 Subject: [PATCH 3/9] Remove short (abbreviated) flag support --- pkg/cmd/cache/delete/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index e244cc0a1..c05674170 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -90,7 +90,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co } cmd.Flags().BoolVarP(&opts.DeleteAll, "all", "a", false, "Delete all caches") - cmd.Flags().BoolVarP(&opts.SucceedOnNoCaches, "succeed-on-no-caches", "s", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`") + cmd.Flags().BoolVarP(&opts.SucceedOnNoCaches, "succeed-on-no-caches", "", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`") return cmd } From 2f0f387e112137c0239d9605ece202a40f38504e Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 6 Feb 2025 09:47:01 -0800 Subject: [PATCH 4/9] Add test cases for succeed on no cache and api errors for --all (#1) --- pkg/cmd/cache/delete/delete_test.go | 61 +++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index e81fdbcec..a9ec88dea 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -176,6 +176,19 @@ func TestDeleteRun(t *testing.T) { tty: true, wantStdout: "✓ Deleted 2 caches from OWNER/REPO\n", }, + { + name: "attempts to delete all caches but api errors", + opts: DeleteOptions{DeleteAll: true}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), + httpmock.StatusStringResponse(500, ""), + ) + }, + tty: true, + wantErr: true, + wantErrMsg: "HTTP 500 (https://api.github.com/repos/OWNER/REPO/actions/caches?per_page=100)", + }, { name: "displays delete error", opts: DeleteOptions{Identifier: "123"}, @@ -202,6 +215,54 @@ func TestDeleteRun(t *testing.T) { tty: true, wantStdout: "✓ Deleted 1 cache from OWNER/REPO\n", }, + { + name: "no caches to delete when deleting all", + opts: DeleteOptions{Identifier: "123", DeleteAll: true}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), + httpmock.JSONResponse(shared.CachePayload{ + ActionsCaches: []shared.Cache{}, + TotalCount: 0, + }), + ) + }, + tty: false, + wantErr: true, + wantErrMsg: "X No caches to delete", + }, + { + name: "no caches to delete when deleting all but succeed on no cache tty", + opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), + httpmock.JSONResponse(shared.CachePayload{ + ActionsCaches: []shared.Cache{}, + TotalCount: 0, + }), + ) + }, + tty: true, + wantErr: false, + wantStdout: "✓ No caches to delete\n", + }, + { + name: "no caches to delete when deleting all but succeed on no cache non-tty", + opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), + httpmock.JSONResponse(shared.CachePayload{ + ActionsCaches: []shared.Cache{}, + TotalCount: 0, + }), + ) + }, + tty: false, + wantErr: false, + wantStdout: "", + }, } for _, tt := range tests { From 09ec38e8d776790089eac7865c6cfcf0a0e79834 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Thu, 6 Feb 2025 22:50:26 +0500 Subject: [PATCH 5/9] Update tests --- pkg/cmd/cache/delete/delete_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index a9ec88dea..57ecb9381 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -217,7 +217,7 @@ func TestDeleteRun(t *testing.T) { }, { name: "no caches to delete when deleting all", - opts: DeleteOptions{Identifier: "123", DeleteAll: true}, + opts: DeleteOptions{DeleteAll: true}, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -233,7 +233,7 @@ func TestDeleteRun(t *testing.T) { }, { name: "no caches to delete when deleting all but succeed on no cache tty", - opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true}, + opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true}, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -249,7 +249,7 @@ func TestDeleteRun(t *testing.T) { }, { name: "no caches to delete when deleting all but succeed on no cache non-tty", - opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true}, + opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true}, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -261,7 +261,7 @@ func TestDeleteRun(t *testing.T) { }, tty: false, wantErr: false, - wantStdout: "", + wantStdout: "✓ No caches to delete\n", }, } From a29e74483a8cfe6ac0f0486280f436ddabbfc693 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Fri, 7 Feb 2025 17:56:52 +0500 Subject: [PATCH 6/9] Update tests --- pkg/cmd/cache/delete/delete_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index a9ec88dea..57ecb9381 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -217,7 +217,7 @@ func TestDeleteRun(t *testing.T) { }, { name: "no caches to delete when deleting all", - opts: DeleteOptions{Identifier: "123", DeleteAll: true}, + opts: DeleteOptions{DeleteAll: true}, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -233,7 +233,7 @@ func TestDeleteRun(t *testing.T) { }, { name: "no caches to delete when deleting all but succeed on no cache tty", - opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true}, + opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true}, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -249,7 +249,7 @@ func TestDeleteRun(t *testing.T) { }, { name: "no caches to delete when deleting all but succeed on no cache non-tty", - opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true}, + opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true}, stubs: func(reg *httpmock.Registry) { reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"), @@ -261,7 +261,7 @@ func TestDeleteRun(t *testing.T) { }, tty: false, wantErr: false, - wantStdout: "", + wantStdout: "✓ No caches to delete\n", }, } From 17e7b57b3bb31630a747e408df9b160fec5a8a94 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Fri, 7 Feb 2025 18:03:35 +0500 Subject: [PATCH 7/9] Use API without shorthand flag --- pkg/cmd/cache/delete/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index c05674170..a797eb607 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -90,7 +90,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co } cmd.Flags().BoolVarP(&opts.DeleteAll, "all", "a", false, "Delete all caches") - cmd.Flags().BoolVarP(&opts.SucceedOnNoCaches, "succeed-on-no-caches", "", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`") + cmd.Flags().BoolVar(&opts.SucceedOnNoCaches, "succeed-on-no-caches", false, "Return exit code 0 if no caches found. Must be used in conjunction with `--all`") return cmd } From 76efb5ac817a1b6e5be7808607f2c3b8f5b72590 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Wed, 19 Feb 2025 10:22:31 +0500 Subject: [PATCH 8/9] Fix non-TTY case --- pkg/cmd/cache/delete/delete.go | 4 +++- pkg/cmd/cache/delete/delete_test.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index a797eb607..ab76368ad 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -117,7 +117,9 @@ func deleteRun(opts *DeleteOptions) error { } if len(caches.ActionsCaches) == 0 { if opts.SucceedOnNoCaches { - fmt.Fprintf(opts.IO.Out, "%s No caches to delete\n", opts.IO.ColorScheme().SuccessIcon()) + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.Out, "%s No caches to delete\n", opts.IO.ColorScheme().SuccessIcon()) + } return nil } else { return fmt.Errorf("%s No caches to delete", opts.IO.ColorScheme().FailureIcon()) diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index 57ecb9381..8660d693b 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -261,7 +261,7 @@ func TestDeleteRun(t *testing.T) { }, tty: false, wantErr: false, - wantStdout: "✓ No caches to delete\n", + wantStdout: "", }, } From 9389628323b89d013496320b04eaa925a063295d Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Wed, 19 Feb 2025 10:31:20 +0500 Subject: [PATCH 9/9] Update tests --- pkg/cmd/cache/delete/delete_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index 57ecb9381..8660d693b 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -261,7 +261,7 @@ func TestDeleteRun(t *testing.T) { }, tty: false, wantErr: false, - wantStdout: "✓ No caches to delete\n", + wantStdout: "", }, }