diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 58281d831..b8367cb5c 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -164,33 +164,18 @@ 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: - // - // 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. - - 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 +192,45 @@ 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 { + // 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) +} + +// 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 != "" { + 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",