Skip to content

Commit 926e293

Browse files
mxiemxie
authored
Merge pull request cli#11592 from luxass/cache-delete-with-ref
feat: add support for `--ref` in `gh cache delete`
2 parents ade5816 + 08e4aa8 commit 926e293

2 files changed

Lines changed: 128 additions & 3 deletions

File tree

‎pkg/cmd/cache/delete/delete.go‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ type DeleteOptions struct {
2525
DeleteAll bool
2626
SucceedOnNoCaches bool
2727
Identifier string
28+
Ref string
2829
}
2930

3031
func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Command {
@@ -51,6 +52,12 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co
5152
# Delete a cache by id in a specific repo
5253
$ gh cache delete 1234 --repo cli/cli
5354
55+
# Delete a cache by key and branch ref
56+
$ gh cache delete cache-key --ref refs/heads/feature-branch
57+
58+
# Delete a cache by key and PR ref
59+
$ gh cache delete cache-key --ref refs/pull/<PR-number>/merge
60+
5461
# Delete all caches (exit code 1 on no caches)
5562
$ gh cache delete --all
5663
@@ -69,14 +76,31 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co
6976
return err
7077
}
7178

79+
if err := cmdutil.MutuallyExclusive(
80+
"--ref cannot be used with --all",
81+
opts.DeleteAll, opts.Ref != "",
82+
); err != nil {
83+
return err
84+
}
85+
7286
if !opts.DeleteAll && opts.SucceedOnNoCaches {
7387
return cmdutil.FlagErrorf("--succeed-on-no-caches must be used in conjunction with --all")
7488
}
7589

90+
if opts.Ref != "" && len(args) == 0 {
91+
return cmdutil.FlagErrorf("must provide a cache key")
92+
}
93+
7694
if !opts.DeleteAll && len(args) == 0 {
7795
return cmdutil.FlagErrorf("must provide either cache id, cache key, or use --all")
7896
}
7997

98+
if len(args) > 0 && opts.Ref != "" {
99+
if _, ok := parseCacheID(args[0]); ok {
100+
return cmdutil.FlagErrorf("--ref cannot be used with cache ID")
101+
}
102+
}
103+
80104
if len(args) == 1 {
81105
opts.Identifier = args[0]
82106
}
@@ -90,6 +114,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co
90114
}
91115

92116
cmd.Flags().BoolVarP(&opts.DeleteAll, "all", "a", false, "Delete all caches")
117+
cmd.Flags().StringVarP(&opts.Ref, "ref", "r", "", "Delete by cache key and ref, formatted as refs/heads/<branch name> or refs/pull/<number>/merge")
93118
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`")
94119

95120
return cmd
@@ -142,19 +167,39 @@ func deleteCaches(opts *DeleteOptions, client *api.Client, repo ghrepo.Interface
142167
base := fmt.Sprintf("repos/%s/actions/caches", repoName)
143168

144169
for _, cache := range toDelete {
170+
// TODO(babakks): We use two different endpoints here which have different
171+
// response schemas:
172+
//
173+
// 1. /repos/OWNER/REPO/actions/caches/ID (for deleting by cache ID)
174+
// - returns HTTP 204 (NO CONTENT) on success
175+
// 2. /repos/OWNER/REPO/actions/caches?key=KEY[&ref=REF] (for deleting by cache key, and optionally a ref)
176+
// - returns HTTP 200 on success including information about the deleted caches
177+
//
178+
// So, if/when we decided to use the data in the response body we need
179+
// to be careful with parsing. Probably want to split these API calls
180+
// into separate functions.
181+
145182
path := ""
146-
if id, err := strconv.Atoi(cache); err == nil {
183+
if id, ok := parseCacheID(cache); ok {
147184
path = fmt.Sprintf("%s/%d", base, id)
148185
} else {
149186
path = fmt.Sprintf("%s?key=%s", base, url.QueryEscape(cache))
187+
188+
if opts.Ref != "" {
189+
path += fmt.Sprintf("&ref=%s", url.QueryEscape(opts.Ref))
190+
}
150191
}
151192

152193
err := client.REST(repo.RepoHost(), "DELETE", path, nil, nil)
153194
if err != nil {
154195
var httpErr api.HTTPError
155196
if errors.As(err, &httpErr) {
156197
if httpErr.StatusCode == http.StatusNotFound {
157-
err = fmt.Errorf("%s Could not find a cache matching %s in %s", cs.FailureIcon(), cache, repoName)
198+
if opts.Ref == "" {
199+
err = fmt.Errorf("%s Could not find a cache matching %s in %s", cs.FailureIcon(), cache, repoName)
200+
} else {
201+
err = fmt.Errorf("%s Could not find a cache matching %s (with ref %s) in %s", cs.FailureIcon(), cache, opts.Ref, repoName)
202+
}
158203
} else {
159204
err = fmt.Errorf("%s Failed to delete cache: %w", cs.FailureIcon(), err)
160205
}
@@ -172,3 +217,8 @@ func deleteCaches(opts *DeleteOptions, client *api.Client, repo ghrepo.Interface
172217

173218
return nil
174219
}
220+
221+
func parseCacheID(arg string) (int, bool) {
222+
id, err := strconv.Atoi(arg)
223+
return id, err == nil
224+
}

‎pkg/cmd/cache/delete/delete_test.go‎

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,36 @@ func TestNewCmdDelete(t *testing.T) {
5858
cli: "--succeed-on-no-caches 123",
5959
wantsErr: "--succeed-on-no-caches must be used in conjunction with --all",
6060
},
61+
{
62+
name: "key argument and delete all flag",
63+
cli: "cache-key --all",
64+
wantsErr: "specify only one of cache id, cache key, or --all",
65+
},
6166
{
6267
name: "id argument and delete all flag",
6368
cli: "1 --all",
6469
wantsErr: "specify only one of cache id, cache key, or --all",
6570
},
71+
{
72+
name: "key argument with ref",
73+
cli: "cache-key --ref refs/heads/main",
74+
wants: DeleteOptions{Identifier: "cache-key", Ref: "refs/heads/main"},
75+
},
76+
{
77+
name: "ref flag without cache key",
78+
cli: "--ref refs/heads/main",
79+
wantsErr: "must provide a cache key",
80+
},
81+
{
82+
name: "ref flag with cache id",
83+
cli: "123 --ref refs/heads/main",
84+
wantsErr: "--ref cannot be used with cache ID",
85+
},
86+
{
87+
name: "ref flag with all flag",
88+
cli: "--all --ref refs/heads/main",
89+
wantsErr: "--ref cannot be used with --all",
90+
},
6691
}
6792

6893
for _, tt := range tests {
@@ -89,6 +114,7 @@ func TestNewCmdDelete(t *testing.T) {
89114
assert.Equal(t, tt.wants.DeleteAll, gotOpts.DeleteAll)
90115
assert.Equal(t, tt.wants.SucceedOnNoCaches, gotOpts.SucceedOnNoCaches)
91116
assert.Equal(t, tt.wants.Identifier, gotOpts.Identifier)
117+
assert.Equal(t, tt.wants.Ref, gotOpts.Ref)
92118
})
93119
}
94120
}
@@ -209,7 +235,8 @@ func TestDeleteRun(t *testing.T) {
209235
httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{
210236
"key": []string{"a weird＿cache+key"},
211237
}),
212-
httpmock.StatusStringResponse(204, ""),
238+
// The response is a JSON object but we don't need it here.
239+
httpmock.StatusStringResponse(200, "{}"),
213240
)
214241
},
215242
tty: true,
@@ -263,6 +290,54 @@ func TestDeleteRun(t *testing.T) {
263290
wantErr: false,
264291
wantStdout: "",
265292
},
293+
{
294+
name: "deletes cache with ref tty",
295+
opts: DeleteOptions{Identifier: "cache-key", Ref: "refs/heads/main"},
296+
stubs: func(reg *httpmock.Registry) {
297+
reg.Register(
298+
httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{
299+
"key": []string{"cache-key"},
300+
"ref": []string{"refs/heads/main"},
301+
}),
302+
// The response is a JSON object but we don't need it here.
303+
httpmock.StatusStringResponse(200, "{}"),
304+
)
305+
},
306+
tty: true,
307+
wantStdout: "✓ Deleted 1 cache from OWNER/REPO\n",
308+
},
309+
{
310+
name: "deletes cache with ref non-tty",
311+
opts: DeleteOptions{Identifier: "cache-key", Ref: "refs/heads/main"},
312+
stubs: func(reg *httpmock.Registry) {
313+
reg.Register(
314+
httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{
315+
"key": []string{"cache-key"},
316+
"ref": []string{"refs/heads/main"},
317+
}),
318+
// The response is a JSON object but we don't need it here.
319+
httpmock.StatusStringResponse(200, "{}"),
320+
)
321+
},
322+
tty: false,
323+
wantStdout: "",
324+
},
325+
{
326+
// As of now, the API returns HTTP 404 for invalid or non-existent refs.
327+
name: "cache key exists but ref is invalid/not-found",
328+
opts: DeleteOptions{Identifier: "existing-cache-key", Ref: "invalid-ref"},
329+
stubs: func(reg *httpmock.Registry) {
330+
reg.Register(
331+
httpmock.QueryMatcher("DELETE", "repos/OWNER/REPO/actions/caches", url.Values{
332+
"key": []string{"existing-cache-key"},
333+
"ref": []string{"invalid-ref"},
334+
}),
335+
httpmock.StatusStringResponse(404, ""),
336+
)
337+
},
338+
wantErr: true,
339+
wantErrMsg: "X Could not find a cache matching existing-cache-key (with ref invalid-ref) in OWNER/REPO",
340+
},
266341
}
267342

268343
for _, tt := range tests {

0 commit comments

Comments
 (0)