From ccfc2c3045485e3372f35eff6278bb9ea188e391 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 1 Oct 2025 18:44:24 +0200 Subject: [PATCH 1/3] fix(cache): report correct deleted count for key and key+ref deletions Previously, deleting caches by key or key+ref could misreport the number of deleted entries, especially when multiple caches matched the criteria. This change ensures the actual number of deleted caches is reported by consuming the API response's total_count field. --- pkg/cmd/cache/delete/delete.go | 45 ++++++++++++++++++--------- pkg/cmd/cache/delete/delete_test.go | 48 +++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 21 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 58281d831..3f17d5d31 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -164,33 +164,27 @@ func deleteCaches(opts *DeleteOptions, client *api.Client, repo ghrepo.Interface cs := opts.IO.ColorScheme() repoName := ghrepo.FullName(repo) opts.IO.StartProgressIndicator() - base := fmt.Sprintf("repos/%s/actions/caches", repoName) + totalDeleted := 0 for _, cache := range toDelete { - // TODO(babakks): We use two different endpoints here which have different - // response schemas: + // We use two different endpoints with different response schemas: // // 1. /repos/OWNER/REPO/actions/caches/ID (for deleting by cache ID) // - returns HTTP 204 (NO CONTENT) on success // 2. /repos/OWNER/REPO/actions/caches?key=KEY[&ref=REF] (for deleting by cache key, and optionally a ref) // - returns HTTP 200 on success including information about the deleted caches // - // So, if/when we decided to use the data in the response body we need - // to be careful with parsing. Probably want to split these API calls - // into separate functions. + // The API calls are split into separate functions to handle the different response handling. - path := "" + var count int + var err error if id, ok := parseCacheID(cache); ok { - path = fmt.Sprintf("%s/%d", base, id) + err = deleteCacheByID(client, repo, id) + count = 1 } else { - path = fmt.Sprintf("%s?key=%s", base, url.QueryEscape(cache)) - - if opts.Ref != "" { - path += fmt.Sprintf("&ref=%s", url.QueryEscape(opts.Ref)) - } + count, err = deleteCacheByKey(client, repo, cache, opts.Ref) } - err := client.REST(repo.RepoHost(), "DELETE", path, nil, nil) if err != nil { var httpErr api.HTTPError if errors.As(err, &httpErr) { @@ -207,17 +201,38 @@ func deleteCaches(opts *DeleteOptions, client *api.Client, repo ghrepo.Interface opts.IO.StopProgressIndicator() return err } + + totalDeleted += count } opts.IO.StopProgressIndicator() if opts.IO.IsStdoutTTY() { - fmt.Fprintf(opts.IO.Out, "%s Deleted %s from %s\n", cs.SuccessIcon(), text.Pluralize(len(toDelete), "cache"), repoName) + fmt.Fprintf(opts.IO.Out, "%s Deleted %s from %s\n", cs.SuccessIcon(), text.Pluralize(totalDeleted, "cache"), repoName) } return nil } +func deleteCacheByID(client *api.Client, repo ghrepo.Interface, id int) error { + path := fmt.Sprintf("repos/%s/actions/caches/%d", ghrepo.FullName(repo), id) + return client.REST(repo.RepoHost(), "DELETE", path, nil, nil) +} + +func deleteCacheByKey(client *api.Client, repo ghrepo.Interface, key, ref string) (int, error) { + path := fmt.Sprintf("repos/%s/actions/caches?key=%s", ghrepo.FullName(repo), url.QueryEscape(key)) + if ref != "" { + path += fmt.Sprintf("&ref=%s", url.QueryEscape(ref)) + } + var payload shared.CachePayload + err := client.REST(repo.RepoHost(), "DELETE", path, nil, &payload) + if err != nil { + return 0, err + } + + return payload.TotalCount, nil +} + func parseCacheID(arg string) (int, bool) { id, err := strconv.Atoi(arg) return id, err == nil diff --git a/pkg/cmd/cache/delete/delete_test.go b/pkg/cmd/cache/delete/delete_test.go index 4cfa3ce26..a05c57c7c 100644 --- a/pkg/cmd/cache/delete/delete_test.go +++ b/pkg/cmd/cache/delete/delete_test.go @@ -235,13 +235,30 @@ func TestDeleteRun(t *testing.T) { httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{ "key": []string{"a weird＿cache+key"}, }), - // The response is a JSON object but we don't need it here. - httpmock.StatusStringResponse(200, "{}"), + httpmock.JSONResponse(shared.CachePayload{ + TotalCount: 1, + }), ) }, tty: true, wantStdout: "✓ Deleted 1 cache from OWNER/REPO\n", }, + { + name: "deletes multiple caches by key", + opts: DeleteOptions{Identifier: "shared-cache-key"}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{ + "key": []string{"shared-cache-key"}, + }), + httpmock.JSONResponse(shared.CachePayload{ + TotalCount: 5, + }), + ) + }, + tty: true, + wantStdout: "✓ Deleted 5 caches from OWNER/REPO\n", + }, { name: "no caches to delete when deleting all", opts: DeleteOptions{DeleteAll: true}, @@ -299,8 +316,9 @@ func TestDeleteRun(t *testing.T) { "key": []string{"cache-key"}, "ref": []string{"refs/heads/main"}, }), - // The response is a JSON object but we don't need it here. - httpmock.StatusStringResponse(200, "{}"), + httpmock.JSONResponse(shared.CachePayload{ + TotalCount: 1, + }), ) }, tty: true, @@ -315,13 +333,31 @@ func TestDeleteRun(t *testing.T) { "key": []string{"cache-key"}, "ref": []string{"refs/heads/main"}, }), - // The response is a JSON object but we don't need it here. - httpmock.StatusStringResponse(200, "{}"), + httpmock.JSONResponse(shared.CachePayload{ + TotalCount: 1, + }), ) }, tty: false, wantStdout: "", }, + { + name: "deletes multiple caches by key and ref", + opts: DeleteOptions{Identifier: "cache-key", Ref: "refs/heads/feature"}, + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{ + "key": []string{"cache-key"}, + "ref": []string{"refs/heads/feature"}, + }), + httpmock.JSONResponse(shared.CachePayload{ + TotalCount: 3, + }), + ) + }, + tty: true, + wantStdout: "✓ Deleted 3 caches from OWNER/REPO\n", + }, { // As of now, the API returns HTTP 404 for invalid or non-existent refs. name: "cache key exists but ref is invalid/not-found", From d574873f3b921ec4664d72ba8eab3982ac643318 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Thu, 2 Oct 2025 11:33:26 +0100 Subject: [PATCH 2/3] docs(cache delete): add godoc for `deleteCacheByKey` Signed-off-by: Babak K. Shandiz --- pkg/cmd/cache/delete/delete.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 3f17d5d31..480dbc2f2 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -219,6 +219,12 @@ func deleteCacheByID(client *api.Client, repo ghrepo.Interface, id int) error { return client.REST(repo.RepoHost(), "DELETE", path, nil, nil) } +// deleteCacheByKey deletes cache entries by given key (and optional ref) and +// returns the number of deleted entries. +// +// Note that a key/ref combination does not necessarily map to a single cache +// entry. There may be more than one entries with the same key/ref combination, +// but those entries will have different IDs. func deleteCacheByKey(client *api.Client, repo ghrepo.Interface, key, ref string) (int, error) { path := fmt.Sprintf("repos/%s/actions/caches?key=%s", ghrepo.FullName(repo), url.QueryEscape(key)) if ref != "" { From ab9c99ec04e9d9cdfaff5ffe59cb5c23d6a1ff97 Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Thu, 2 Oct 2025 11:36:00 +0100 Subject: [PATCH 3/3] docs(cache delete): remove redundant comment Signed-off-by: Babak K. Shandiz --- pkg/cmd/cache/delete/delete.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 480dbc2f2..b8367cb5c 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -167,15 +167,6 @@ func deleteCaches(opts *DeleteOptions, client *api.Client, repo ghrepo.Interface totalDeleted := 0 for _, cache := range toDelete { - // We use two different endpoints with different response schemas: - // - // 1. /repos/OWNER/REPO/actions/caches/ID (for deleting by cache ID) - // - returns HTTP 204 (NO CONTENT) on success - // 2. /repos/OWNER/REPO/actions/caches?key=KEY[&ref=REF] (for deleting by cache key, and optionally a ref) - // - returns HTTP 200 on success including information about the deleted caches - // - // The API calls are split into separate functions to handle the different response handling. - var count int var err error if id, ok := parseCacheID(cache); ok { @@ -215,6 +206,7 @@ func deleteCaches(opts *DeleteOptions, client *api.Client, repo ghrepo.Interface } func deleteCacheByID(client *api.Client, repo ghrepo.Interface, id int) error { + // returns HTTP 204 (NO CONTENT) on success path := fmt.Sprintf("repos/%s/actions/caches/%d", ghrepo.FullName(repo), id) return client.REST(repo.RepoHost(), "DELETE", path, nil, nil) }