Skip to content

Commit 608dff7

Browse files
williammartinCopilot
williammartin
and
Copilot
authored
Route release deletions through api.Client (#14077)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd6661ad-7e77-46d0-b0e7-0a296f4e252b
1 parent 8b72a8e commit 608dff7

12 files changed

Lines changed: 254 additions & 93 deletions

File tree

‎acceptance/testdata/release/release-delete.txtar‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
# Create a repository with a file so it has a default branch
2+
exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private
3+
4+
# Defer repo cleanup
5+
defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING
6+
7+
# Create a release in the repo
8+
exec gh release create v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --notes 'awesome release' --latest
9+
10+
# Upload an asset to the release
11+
exec gh release upload v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING asset.txt
12+
13+
# Delete the asset from the release
14+
exec gh release delete-asset v1.2.3 asset.txt --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --yes
15+
16+
# Verify the release has no assets
17+
exec gh release view v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --json assets --jq '.assets | length'
18+
stdout '0'
19+
20+
# Downloading the deleted asset should fail
21+
! exec gh release download v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING
22+
stderr 'no assets to download'
23+
24+
# Delete the release and its tag
25+
exec gh release delete v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --yes --cleanup-tag
26+
27+
# Wait for tag deletion to become visible through the ref lookup
28+
sleep 5
29+
30+
# Verify the release is gone
31+
! exec gh release view v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING
32+
stderr 'release not found'
33+
34+
# Verify the tag is gone
35+
! exec gh api repos/$ORG/$SCRIPT_NAME-$RANDOM_STRING/git/ref/tags/v1.2.3
36+
stderr 'Not Found'
37+
38+
-- asset.txt --
39+
Hello, world!

‎pkg/cmd/release/delete-asset/delete_asset.go‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func deleteAssetRun(opts *DeleteAssetOptions) error {
9797
return fmt.Errorf("asset %s not found in release %s", opts.AssetName, release.TagName)
9898
}
9999

100-
err = deleteAsset(httpClient, safeurl.NewImmutableSafeURL(assetURL))
100+
err = deleteAsset(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(assetURL))
101101
if err != nil {
102102
return err
103103
}
@@ -112,20 +112,9 @@ func deleteAssetRun(opts *DeleteAssetOptions) error {
112112
return nil
113113
}
114114

115-
func deleteAsset(httpClient *http.Client, assetURL safeurl.SafeURL) error {
116-
req, err := http.NewRequest("DELETE", assetURL.String(), nil)
117-
if err != nil {
118-
return err
119-
}
120-
121-
resp, err := httpClient.Do(req)
122-
if err != nil {
123-
return err
124-
}
125-
defer resp.Body.Close()
126-
127-
if resp.StatusCode > 299 {
128-
return api.HandleHTTPError(resp)
129-
}
130-
return nil
115+
func deleteAsset(httpClient *http.Client, host string, assetURL safeurl.SafeURL) error {
116+
// TODO(api-client-rollout)
117+
// This line of code is part of a mechanical roll out of the api client.
118+
// As a follow up, consider whether the api client can be injected to this call site, rather than constructed
119+
return api.NewClientFromHTTP(httpClient).REST(host, http.MethodDelete, assetURL.String(), nil, nil)
131120
}

‎pkg/cmd/release/delete-asset/delete_asset_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,10 @@ import (
66
"net/http"
77
"testing"
88

9+
"github.com/cli/cli/v2/api"
910
"github.com/cli/cli/v2/internal/ghrepo"
1011
"github.com/cli/cli/v2/internal/prompter"
12+
"github.com/cli/cli/v2/internal/safeurl"
1113
"github.com/cli/cli/v2/pkg/cmd/release/shared"
1214
"github.com/cli/cli/v2/pkg/cmdutil"
1315
"github.com/cli/cli/v2/pkg/httpmock"
@@ -199,3 +201,24 @@ func Test_deleteAssetRun(t *testing.T) {
199201
})
200202
}
201203
}
204+
205+
func Test_deleteAsset_httpError(t *testing.T) {
206+
reg := &httpmock.Registry{}
207+
defer reg.Verify(t)
208+
reg.Register(
209+
func(req *http.Request) bool {
210+
return req.Method == http.MethodDelete &&
211+
req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/assets/1" &&
212+
req.URL.Host == "api.github.com"
213+
},
214+
httpmock.StatusStringResponse(404, `{"message":"Not Found"}`),
215+
)
216+
217+
httpClient := &http.Client{Transport: reg}
218+
err := deleteAsset(httpClient, "example.com", safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/releases/assets/1"))
219+
220+
var httpErr api.HTTPError
221+
require.ErrorAs(t, err, &httpErr)
222+
assert.Equal(t, http.StatusNotFound, httpErr.StatusCode)
223+
assert.Contains(t, err.Error(), "HTTP 404")
224+
}

‎pkg/cmd/release/delete/delete.go‎

Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77

88
"github.com/cli/cli/v2/api"
99
"github.com/cli/cli/v2/git"
10-
"github.com/cli/cli/v2/internal/ghinstance"
1110
"github.com/cli/cli/v2/internal/ghrepo"
1211
"github.com/cli/cli/v2/internal/safeurl"
1312
"github.com/cli/cli/v2/pkg/cmd/release/shared"
@@ -93,7 +92,7 @@ func deleteRun(opts *DeleteOptions) error {
9392
}
9493
}
9594

96-
err = deleteRelease(httpClient, safeurl.NewImmutableSafeURL(release.APIURL))
95+
err = deleteRelease(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(release.APIURL))
9796
if err != nil {
9897
return err
9998
}
@@ -122,42 +121,20 @@ func deleteRun(opts *DeleteOptions) error {
122121
return nil
123122
}
124123

125-
func deleteRelease(httpClient *http.Client, releaseURL safeurl.SafeURL) error {
126-
req, err := http.NewRequest("DELETE", releaseURL.String(), nil)
127-
if err != nil {
128-
return err
129-
}
130-
131-
resp, err := httpClient.Do(req)
132-
if err != nil {
133-
return err
134-
}
135-
defer resp.Body.Close()
136-
137-
if resp.StatusCode > 299 {
138-
return api.HandleHTTPError(resp)
139-
}
140-
return nil
124+
func deleteRelease(httpClient *http.Client, host string, releaseURL safeurl.SafeURL) error {
125+
// TODO(api-client-rollout)
126+
// This line of code is part of a mechanical roll out of the api client.
127+
// As a follow up, consider whether the api client can be injected to this call site, rather than constructed
128+
return api.NewClientFromHTTP(httpClient).REST(host, http.MethodDelete, releaseURL.String(), nil, nil)
141129
}
142130

143131
func deleteTag(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) error {
144-
url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName))
132+
path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName))
145133
if err != nil {
146134
return err
147135
}
148-
req, err := http.NewRequest("DELETE", url.String(), nil)
149-
if err != nil {
150-
return err
151-
}
152-
153-
resp, err := httpClient.Do(req)
154-
if err != nil {
155-
return err
156-
}
157-
defer resp.Body.Close()
158-
159-
if resp.StatusCode > 299 {
160-
return api.HandleHTTPError(resp)
161-
}
162-
return nil
136+
// TODO(api-client-rollout)
137+
// This line of code is part of a mechanical roll out of the api client.
138+
// As a follow up, consider whether the api client can be injected to this call site, rather than constructed
139+
return api.NewClientFromHTTP(httpClient).REST(baseRepo.RepoHost(), http.MethodDelete, path.String(), nil, nil)
163140
}

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,12 @@ import (
77
"testing"
88

99
"github.com/MakeNowJust/heredoc"
10+
"github.com/cli/cli/v2/api"
1011
"github.com/cli/cli/v2/git"
1112
"github.com/cli/cli/v2/internal/ghrepo"
1213
"github.com/cli/cli/v2/internal/prompter"
1314
"github.com/cli/cli/v2/internal/run"
15+
"github.com/cli/cli/v2/internal/safeurl"
1416
"github.com/cli/cli/v2/pkg/cmd/release/shared"
1517
"github.com/cli/cli/v2/pkg/cmdutil"
1618
"github.com/cli/cli/v2/pkg/httpmock"
@@ -241,3 +243,42 @@ func Test_deleteRun(t *testing.T) {
241243
})
242244
}
243245
}
246+
247+
func Test_deleteRelease_httpError(t *testing.T) {
248+
reg := &httpmock.Registry{}
249+
defer reg.Verify(t)
250+
reg.Register(
251+
func(req *http.Request) bool {
252+
return req.Method == http.MethodDelete &&
253+
req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/23456" &&
254+
req.URL.Host == "api.github.com"
255+
},
256+
httpmock.StatusStringResponse(404, `{"message":"Not Found"}`),
257+
)
258+
259+
httpClient := &http.Client{Transport: reg}
260+
err := deleteRelease(httpClient, "example.com", safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/releases/23456"))
261+
262+
var httpErr api.HTTPError
263+
require.ErrorAs(t, err, &httpErr)
264+
assert.Equal(t, http.StatusNotFound, httpErr.StatusCode)
265+
assert.Contains(t, err.Error(), "HTTP 404")
266+
}
267+
268+
func Test_deleteTag_httpError(t *testing.T) {
269+
reg := &httpmock.Registry{}
270+
defer reg.Verify(t)
271+
reg.Register(
272+
httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/tags%2Fv1.2.3"),
273+
httpmock.StatusStringResponse(404, `{"message":"Not Found"}`),
274+
)
275+
276+
httpClient := &http.Client{Transport: reg}
277+
baseRepo, _ := ghrepo.FromFullName("OWNER/REPO")
278+
err := deleteTag(httpClient, baseRepo, "v1.2.3")
279+
280+
var httpErr api.HTTPError
281+
require.ErrorAs(t, err, &httpErr)
282+
assert.Equal(t, http.StatusNotFound, httpErr.StatusCode)
283+
assert.Contains(t, err.Error(), "HTTP 404")
284+
}

‎pkg/cmd/release/edit/edit_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,14 @@ package edit
22

33
import (
44
"bytes"
5+
"errors"
56
"fmt"
67
"io"
78
"net/http"
89
"os"
910
"testing"
1011

12+
"github.com/cli/cli/v2/api"
1113
"github.com/cli/cli/v2/internal/ghrepo"
1214
"github.com/cli/cli/v2/pkg/cmd/release/shared"
1315
"github.com/cli/cli/v2/pkg/cmdutil"
@@ -480,6 +482,99 @@ func mockSuccessfulEditResponse(reg *httpmock.Registry, cb func(params map[strin
480482
reg.Register(matcher, responder)
481483
}
482484

485+
func Test_editRelease_httpError(t *testing.T) {
486+
reg := &httpmock.Registry{}
487+
defer reg.Verify(t)
488+
reg.Register(
489+
func(req *http.Request) bool {
490+
return req.Method == http.MethodPatch &&
491+
req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" &&
492+
req.URL.Host == "api.github.com"
493+
},
494+
httpmock.StatusStringResponse(404, `{"message":"Not Found"}`),
495+
)
496+
497+
httpClient := &http.Client{Transport: reg}
498+
release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"})
499+
500+
var httpErr api.HTTPError
501+
require.ErrorAs(t, err, &httpErr)
502+
assert.Equal(t, http.StatusNotFound, httpErr.StatusCode)
503+
assert.Contains(t, err.Error(), "HTTP 404")
504+
assert.Nil(t, release)
505+
}
506+
507+
func Test_editRelease_decodeError(t *testing.T) {
508+
reg := &httpmock.Registry{}
509+
defer reg.Verify(t)
510+
reg.Register(
511+
func(req *http.Request) bool {
512+
return req.Method == http.MethodPatch &&
513+
req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" &&
514+
req.URL.Host == "api.github.com"
515+
},
516+
httpmock.StatusStringResponse(200, `{`),
517+
)
518+
519+
httpClient := &http.Client{Transport: reg}
520+
release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"})
521+
522+
require.Error(t, err)
523+
assert.NotNil(t, release) // decode was attempted - non-nil pointer even on decode error
524+
}
525+
526+
func Test_editRelease_204(t *testing.T) {
527+
reg := &httpmock.Registry{}
528+
defer reg.Verify(t)
529+
reg.Register(
530+
func(req *http.Request) bool {
531+
return req.Method == http.MethodPatch &&
532+
req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" &&
533+
req.URL.Host == "api.github.com"
534+
},
535+
httpmock.StatusStringResponse(204, ""),
536+
)
537+
538+
httpClient := &http.Client{Transport: reg}
539+
release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"})
540+
541+
require.Error(t, err)
542+
assert.Contains(t, err.Error(), "unexpected end of JSON input")
543+
assert.NotNil(t, release)
544+
}
545+
546+
func Test_editRelease_bodyReadError(t *testing.T) {
547+
readErr := errors.New("read: connection reset by peer")
548+
reg := &httpmock.Registry{}
549+
defer reg.Verify(t)
550+
reg.Register(
551+
func(req *http.Request) bool {
552+
return req.Method == http.MethodPatch &&
553+
req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" &&
554+
req.URL.Host == "api.github.com"
555+
},
556+
func(_ *http.Request) (*http.Response, error) {
557+
return &http.Response{
558+
StatusCode: 200,
559+
Body: io.NopCloser(errorReader{err: readErr}),
560+
Header: http.Header{},
561+
}, nil
562+
},
563+
)
564+
565+
httpClient := &http.Client{Transport: reg}
566+
release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"})
567+
568+
require.Error(t, err)
569+
assert.ErrorIs(t, err, readErr)
570+
assert.Nil(t, release)
571+
}
572+
573+
// errorReader always returns the given error on Read, used to simulate body read failures.
574+
type errorReader struct{ err error }
575+
576+
func (e errorReader) Read(_ []byte) (int, error) { return 0, e.err }
577+
483578
func boolPtr(b bool) *bool {
484579
return &b
485580
}

‎pkg/cmd/release/edit/http.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ func editRelease(httpClient *http.Client, repo ghrepo.Interface, releaseID int64
3333

3434
req.Header.Set("Content-Type", "application/json; charset=utf-8")
3535

36+
// TODO(api-client-rollout)
37+
// This has been deferred from moving to api.Client because its return shape depends on the response status code, which api.Client.REST does not expose on success.
3638
resp, err := httpClient.Do(req)
3739
if err != nil {
3840
return nil, err

‎pkg/cmd/run/download/http.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ func (p *apiPlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) erro
2929
}
3030

3131
func downloadArtifact(httpClient *http.Client, url safeurl.SafeURL, destDir safepaths.Absolute) error {
32+
// TODO(api-client-rollout)
33+
// This has been deferred from moving to api.Client due to streaming the artifact ZIP response body to disk instead of decoding JSON.
3234
req, err := http.NewRequest("GET", url.String(), nil)
3335
if err != nil {
3436
return err

0 commit comments

Comments
 (0)