From ccfc2c3045485e3372f35eff6278bb9ea188e391 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 1 Oct 2025 18:44:24 +0200 Subject: [PATCH] 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",