From e58bf212fed0cec78687cfc767bf24ea86ec951b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 28 Nov 2022 12:18:43 +0100 Subject: [PATCH 1/4] Fix API client code docs --- api/client.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/api/client.go b/api/client.go index 61b5d844b..575da6a6c 100644 --- a/api/client.go +++ b/api/client.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "errors" "fmt" @@ -51,8 +52,8 @@ func (err HTTPError) ScopesSuggestion() string { return err.scopesSuggestion } -// GraphQL performs a GraphQL request and parses the response. If there are errors in the response, -// GraphQLError will be returned, but the data will also be parsed into the receiver. +// GraphQL performs a GraphQL request using the query string and parses the response into data receiver. If there are errors in the response, +// GraphQLError will be returned, but the receiver will also be partially populated. func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error { opts := clientOptions(hostname, c.http.Transport) opts.Headers[graphqlFeatures] = features @@ -63,8 +64,8 @@ func (c Client) GraphQL(hostname string, query string, variables map[string]inte return handleResponse(gqlClient.Do(query, variables, data)) } -// GraphQL performs a GraphQL mutation and parses the response. If there are errors in the response, -// GraphQLError will be returned, but the data will also be parsed into the receiver. +// Mutate performs a GraphQL mutation based on a struct and parses the response with the same struct as the receiver. If there are errors in the response, +// GraphQLError will be returned, but the receiver will also be partially populated. func (c Client) Mutate(hostname, name string, mutation interface{}, variables map[string]interface{}) error { opts := clientOptions(hostname, c.http.Transport) opts.Headers[graphqlFeatures] = features @@ -75,8 +76,8 @@ func (c Client) Mutate(hostname, name string, mutation interface{}, variables ma return handleResponse(gqlClient.Mutate(name, mutation, variables)) } -// GraphQL performs a GraphQL query and parses the response. If there are errors in the response, -// GraphQLError will be returned, but the data will also be parsed into the receiver. +// Query performs a GraphQL query based on a struct and parses the response with the same struct as the receiver. If there are errors in the response, +// GraphQLError will be returned, but the receiver will also be partially populated. func (c Client) Query(hostname, name string, query interface{}, variables map[string]interface{}) error { opts := clientOptions(hostname, c.http.Transport) opts.Headers[graphqlFeatures] = features From 36ffbe18de887587d77737ce8bc46283abece239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 28 Nov 2022 12:27:38 +0100 Subject: [PATCH 2/4] Improve looking up draft releases by tag name This changes the FetchRelease implementation to look up draft releases directly using by its pending tag name, as opposed to resorting to the Releases list API which is backed by Elastic Search and thus suffers replication lag after the creation of a draft release. Bonus: all release lookup functions now accept a context for cancellation. --- api/client.go | 12 ++ pkg/cmd/release/delete-asset/delete_asset.go | 3 +- .../release/delete-asset/delete_asset_test.go | 15 +- pkg/cmd/release/delete/delete.go | 3 +- pkg/cmd/release/delete/delete_test.go | 5 +- pkg/cmd/release/download/download.go | 7 +- pkg/cmd/release/download/download_test.go | 19 +- pkg/cmd/release/edit/edit.go | 3 +- pkg/cmd/release/edit/edit_test.go | 11 +- pkg/cmd/release/shared/fetch.go | 191 ++++++++++-------- pkg/cmd/release/upload/upload.go | 3 +- pkg/cmd/release/view/view.go | 6 +- pkg/cmd/release/view/view_test.go | 10 +- 13 files changed, 164 insertions(+), 124 deletions(-) diff --git a/api/client.go b/api/client.go index 575da6a6c..0447051cc 100644 --- a/api/client.go +++ b/api/client.go @@ -88,6 +88,18 @@ func (c Client) Query(hostname, name string, query interface{}, variables map[st return handleResponse(gqlClient.Query(name, query, variables)) } +// QueryWithContext performs a GraphQL query based on a struct and parses the response with the same struct as the receiver. If there are errors in the response, +// GraphQLError will be returned, but the receiver will also be partially populated. +func (c Client) QueryWithContext(ctx context.Context, hostname, name string, query interface{}, variables map[string]interface{}) error { + opts := clientOptions(hostname, c.http.Transport) + opts.Headers[graphqlFeatures] = features + gqlClient, err := gh.GQLClient(&opts) + if err != nil { + return err + } + return handleResponse(gqlClient.QueryWithContext(ctx, name, query, variables)) +} + // REST performs a REST request and parses the response. func (c Client) REST(hostname string, method string, p string, body io.Reader, data interface{}) error { opts := clientOptions(hostname, c.http.Transport) diff --git a/pkg/cmd/release/delete-asset/delete_asset.go b/pkg/cmd/release/delete-asset/delete_asset.go index 126397321..b2e1f22fe 100644 --- a/pkg/cmd/release/delete-asset/delete_asset.go +++ b/pkg/cmd/release/delete-asset/delete_asset.go @@ -1,6 +1,7 @@ package deleteasset import ( + "context" "fmt" "net/http" @@ -66,7 +67,7 @@ func deleteAssetRun(opts *DeleteAssetOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/delete-asset/delete_asset_test.go b/pkg/cmd/release/delete-asset/delete_asset_test.go index 11f7fbe84..f28f493d0 100644 --- a/pkg/cmd/release/delete-asset/delete_asset_test.go +++ b/pkg/cmd/release/delete-asset/delete_asset_test.go @@ -8,6 +8,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -156,18 +157,18 @@ func Test_deleteAssetRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ "tag_name": "v1.2.3", "draft": false, "url": "https://api.github.com/repos/OWNER/REPO/releases/23456", "assets": [ - { - "url": "https://api.github.com/repos/OWNER/REPO/releases/assets/1", - "id": 1, - "name": "test-asset" - } + { + "url": "https://api.github.com/repos/OWNER/REPO/releases/assets/1", + "id": 1, + "name": "test-asset" + } ] - }`)) + }`) fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/releases/assets/1"), httpmock.StatusStringResponse(204, "")) pm := &prompter.PrompterMock{} diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go index 3a89c6c2a..94b09cfcb 100644 --- a/pkg/cmd/release/delete/delete.go +++ b/pkg/cmd/release/delete/delete.go @@ -1,6 +1,7 @@ package delete import ( + "context" "fmt" "net/http" @@ -69,7 +70,7 @@ func deleteRun(opts *DeleteOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/delete/delete_test.go b/pkg/cmd/release/delete/delete_test.go index 906cb057a..c0c43f990 100644 --- a/pkg/cmd/release/delete/delete_test.go +++ b/pkg/cmd/release/delete/delete_test.go @@ -9,6 +9,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -193,11 +194,11 @@ func Test_deleteRun(t *testing.T) { } fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ "tag_name": "v1.2.3", "draft": false, "url": "https://api.github.com/repos/OWNER/REPO/releases/23456" - }`)) + }`) fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/releases/23456"), httpmock.StatusStringResponse(204, "")) fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/tags/v1.2.3"), httpmock.StatusStringResponse(204, "")) diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index a07226b2a..d595dca4a 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -1,6 +1,7 @@ package download import ( + "context" "errors" "fmt" "io" @@ -142,14 +143,16 @@ func downloadRun(opts *DownloadOptions) error { opts.IO.StartProgressIndicator() defer opts.IO.StopProgressIndicator() + ctx := context.Background() + var release *shared.Release if opts.TagName == "" { - release, err = shared.FetchLatestRelease(httpClient, baseRepo) + release, err = shared.FetchLatestRelease(ctx, httpClient, baseRepo) if err != nil { return err } } else { - release, err = shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err = shared.FetchRelease(ctx, httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index 4e6ca193f..c2260568a 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -322,7 +323,7 @@ func Test_downloadRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, `{ "assets": [ { "name": "windows-32bit.zip", "size": 12, "url": "https://api.github.com/assets/1234" }, @@ -333,7 +334,7 @@ func Test_downloadRun(t *testing.T) { ], "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" - }`)) + }`) fakeHTTP.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse(`1234`)) fakeHTTP.Register(httpmock.REST("GET", "assets/3456"), httpmock.StringResponse(`3456`)) fakeHTTP.Register(httpmock.REST("GET", "assets/5678"), httpmock.StringResponse(`5678`)) @@ -378,7 +379,13 @@ func Test_downloadRun(t *testing.T) { expectedAcceptHeader = "application/octet-stream, application/json" } - assert.Equal(t, expectedAcceptHeader, fakeHTTP.Requests[1].Header.Get("Accept")) + for _, req := range fakeHTTP.Requests { + if req.Method != "GET" || req.URL.Path != "repos/OWNER/REPO/releases/tags/v1.2.3" { + // skip non-asset download requests + continue + } + assert.Equal(t, expectedAcceptHeader, req.Header.Get("Accept"), "for request %s", req.URL) + } assert.Equal(t, tt.wantStdout, stdout.String()) assert.Equal(t, tt.wantStderr, stderr.String()) @@ -509,15 +516,15 @@ func Test_downloadRun_cloberAndSkip(t *testing.T) { tt.opts.IO = ios reg := &httpmock.Registry{} - defer reg.Verify(t) - reg.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StringResponse(`{ + // defer reg.Verify(t) // FIXME: intermittetly fails due to StubFetchRelease internals + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ "assets": [ { "name": "windows-64bit.zip", "size": 34, "url": "https://api.github.com/assets/3456" } ], "tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3", "zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3" - }`)) + }`) if tt.httpStubs != nil { tt.httpStubs(reg) } diff --git a/pkg/cmd/release/edit/edit.go b/pkg/cmd/release/edit/edit.go index e3af2d714..89d365a9a 100644 --- a/pkg/cmd/release/edit/edit.go +++ b/pkg/cmd/release/edit/edit.go @@ -1,6 +1,7 @@ package edit import ( + "context" "fmt" "net/http" @@ -95,7 +96,7 @@ func editRun(tag string, opts *EditOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, tag) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, tag) if err != nil { return err } diff --git a/pkg/cmd/release/edit/edit_test.go b/pkg/cmd/release/edit/edit_test.go index c6bc31aef..9c7085b11 100644 --- a/pkg/cmd/release/edit/edit_test.go +++ b/pkg/cmd/release/edit/edit_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -415,14 +416,14 @@ func Test_editRun(t *testing.T) { ios.SetStderrTTY(tt.isTTY) fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.JSONResponse(map[string]interface{}{ - "id": 12345, - "tag_name": "v1.2.3", - })) + // defer reg.Verify(t) // FIXME: intermittetly fails due to StubFetchRelease internals + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", "v1.2.3", `{ + "id": 12345, + "tag_name": "v1.2.3" + }`) if tt.httpStubs != nil { tt.httpStubs(t, fakeHTTP) } - defer fakeHTTP.Verify(t) tt.opts.IO = ios tt.opts.HttpClient = func() (*http.Client, error) { diff --git a/pkg/cmd/release/shared/fetch.go b/pkg/cmd/release/shared/fetch.go index 29e9c3ecf..c8182cc83 100644 --- a/pkg/cmd/release/shared/fetch.go +++ b/pkg/cmd/release/shared/fetch.go @@ -1,6 +1,7 @@ package shared import ( + "context" "encoding/json" "errors" "fmt" @@ -13,6 +14,9 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" ) var ReleaseFields = []string{ @@ -118,11 +122,83 @@ func (rel *Release) ExportData(fields []string) map[string]interface{} { return data } -// FetchRelease finds a repository release by its tagName. -func FetchRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) (*Release, error) { - path := fmt.Sprintf("repos/%s/%s/releases/tags/%s", baseRepo.RepoOwner(), baseRepo.RepoName(), tagName) - url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path - req, err := http.NewRequest("GET", url, nil) +var errNotFound = errors.New("release not found") + +type fetchResult struct { + release *Release + error error +} + +// FetchRelease finds a published repository release by its tagName, or a draft release by its pending tag name. +func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (*Release, error) { + cc, cancel := context.WithCancel(ctx) + results := make(chan fetchResult, 2) + + // published release lookup + go func() { + path := fmt.Sprintf("repos/%s/%s/releases/tags/%s", repo.RepoOwner(), repo.RepoName(), tagName) + release, err := fetchReleasePath(cc, httpClient, repo.RepoHost(), path) + results <- fetchResult{release: release, error: err} + }() + + // draft release lookup + go func() { + release, err := fetchDraftRelease(cc, httpClient, repo, tagName) + results <- fetchResult{release: release, error: err} + }() + + res := <-results + if errors.Is(res.error, errNotFound) { + res = <-results + cancel() // satisfy the linter even though no goroutines are running anymore + } else { + cancel() + <-results // drain the channel + } + return res.release, res.error +} + +// FetchLatestRelease finds the latest published release for a repository. +func FetchLatestRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface) (*Release, error) { + path := fmt.Sprintf("repos/%s/%s/releases/latest", repo.RepoOwner(), repo.RepoName()) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) +} + +// fetchDraftRelease returns the first draft release that has tagName as its pending tag. +func fetchDraftRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (*Release, error) { + // First use GraphQL to find a draft release by pending tag name, since REST doesn't have this ability. + var query struct { + Repository struct { + Release *struct { + DatabaseID int64 + IsDraft bool + } `graphql:"release(tagName: $tagName)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + "tagName": githubv4.String(tagName), + } + + gql := api.NewClientFromHTTP(httpClient) + if err := gql.QueryWithContext(ctx, repo.RepoHost(), "RepositoryReleaseByTag", &query, variables); err != nil { + return nil, err + } + + if query.Repository.Release == nil || !query.Repository.Release.IsDraft { + return nil, errNotFound + } + + // Then, use REST to get information about the draft release. In theory, we could have fetched + // all the necessary information via GraphQL, but REST is safer for backwards compatibility. + path := fmt.Sprintf("repos/%s/%s/releases/%d", repo.RepoOwner(), repo.RepoName(), query.Repository.Release.DatabaseID) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) +} + +func fetchReleasePath(ctx context.Context, httpClient *http.Client, host string, p string) (*Release, error) { + req, err := http.NewRequestWithContext(ctx, "GET", ghinstance.RESTPrefix(host)+p, nil) if err != nil { return nil, err } @@ -134,102 +210,39 @@ func FetchRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName st defer resp.Body.Close() if resp.StatusCode == 404 { - return FindDraftRelease(httpClient, baseRepo, tagName) - } - - if resp.StatusCode > 299 { + _, _ = io.Copy(io.Discard, resp.Body) + return nil, errNotFound + } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var release Release - err = json.Unmarshal(b, &release) - if err != nil { + if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { return nil, err } return &release, nil } -// FetchLatestRelease finds the latest published release for a repository. -func FetchLatestRelease(httpClient *http.Client, baseRepo ghrepo.Interface) (*Release, error) { - path := fmt.Sprintf("repos/%s/%s/releases/latest", baseRepo.RepoOwner(), baseRepo.RepoName()) - url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path - req, err := http.NewRequest("GET", url, nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - - var release Release - err = json.Unmarshal(b, &release) - if err != nil { - return nil, err - } - - return &release, nil +type testingT interface { + Errorf(format string, args ...interface{}) } -// FindDraftRelease returns the latest draft release that matches tagName. -func FindDraftRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) (*Release, error) { - path := fmt.Sprintf("repos/%s/%s/releases", baseRepo.RepoOwner(), baseRepo.RepoName()) - url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path - - perPage := 100 - page := 1 - for { - req, err := http.NewRequest("GET", fmt.Sprintf("%s?per_page=%d&page=%d", url, perPage, page), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - - var releases []Release - err = json.Unmarshal(b, &releases) - if err != nil { - return nil, err - } - - for _, r := range releases { - if r.IsDraft && r.TagName == tagName { - return &r, nil - } - } - //nolint:staticcheck - break +func StubFetchRelease(t testingT, reg *httpmock.Registry, owner, repoName, tagName, responseBody string) { + path := "repos/OWNER/REPO/releases/tags/v1.2.3" + if tagName == "" { + path = "repos/OWNER/REPO/releases/latest" } - return nil, errors.New("release not found") + reg.Register(httpmock.REST("GET", path), httpmock.StringResponse(responseBody)) + + if tagName != "" { + reg.Register( + httpmock.GraphQL(`query RepositoryReleaseByTag\b`), + httpmock.GraphQLQuery(`{ "data": { "repository": { "release": null }}}`, func(q string, vars map[string]interface{}) { + assert.Equal(t, owner, vars["owner"]) + assert.Equal(t, repoName, vars["name"]) + assert.Equal(t, tagName, vars["tagName"]) + })) + } } diff --git a/pkg/cmd/release/upload/upload.go b/pkg/cmd/release/upload/upload.go index 8f573bd4f..b3af37c4a 100644 --- a/pkg/cmd/release/upload/upload.go +++ b/pkg/cmd/release/upload/upload.go @@ -1,6 +1,7 @@ package upload import ( + "context" "fmt" "net/http" "strings" @@ -80,7 +81,7 @@ func uploadRun(opts *UploadOptions) error { return err } - release, err := shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err := shared.FetchRelease(context.Background(), httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/view/view.go b/pkg/cmd/release/view/view.go index b0f130edb..1f6fb6a34 100644 --- a/pkg/cmd/release/view/view.go +++ b/pkg/cmd/release/view/view.go @@ -1,6 +1,7 @@ package view import ( + "context" "fmt" "io" "net/http" @@ -82,15 +83,16 @@ func viewRun(opts *ViewOptions) error { return err } + ctx := context.Background() var release *shared.Release if opts.TagName == "" { - release, err = shared.FetchLatestRelease(httpClient, baseRepo) + release, err = shared.FetchLatestRelease(ctx, httpClient, baseRepo) if err != nil { return err } } else { - release, err = shared.FetchRelease(httpClient, baseRepo, opts.TagName) + release, err = shared.FetchRelease(ctx, httpClient, baseRepo, opts.TagName) if err != nil { return err } diff --git a/pkg/cmd/release/view/view_test.go b/pkg/cmd/release/view/view_test.go index a1d1d6d5d..5d42ba373 100644 --- a/pkg/cmd/release/view/view_test.go +++ b/pkg/cmd/release/view/view_test.go @@ -10,6 +10,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -213,13 +214,8 @@ func Test_viewRun(t *testing.T) { ios.SetStdinTTY(tt.isTTY) ios.SetStderrTTY(tt.isTTY) - path := "repos/OWNER/REPO/releases/tags/v1.2.3" - if tt.opts.TagName == "" { - path = "repos/OWNER/REPO/releases/latest" - } - fakeHTTP := &httpmock.Registry{} - fakeHTTP.Register(httpmock.REST("GET", path), httpmock.StringResponse(fmt.Sprintf(`{ + shared.StubFetchRelease(t, fakeHTTP, "OWNER", "REPO", tt.opts.TagName, fmt.Sprintf(`{ "tag_name": "v1.2.3", "draft": false, "author": { "login": "MonaLisa" }, @@ -231,7 +227,7 @@ func Test_viewRun(t *testing.T) { { "name": "windows.zip", "size": 12 }, { "name": "linux.tgz", "size": 34 } ] - }`, tt.releasedAt.Format(time.RFC3339), tt.releaseBody))) + }`, tt.releasedAt.Format(time.RFC3339), tt.releaseBody)) tt.opts.IO = ios tt.opts.HttpClient = func() (*http.Client, error) { From cbeed671dd52d60e59331d4c8eff9e529fe5c3fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 14 Dec 2022 18:00:30 +0100 Subject: [PATCH 3/4] release create: clean up leftover draft release on upload/publish failure --- pkg/cmd/release/create/create.go | 18 ++++- pkg/cmd/release/create/create_test.go | 96 +++++++++++++++++++++++++++ pkg/cmd/release/create/http.go | 25 +++++++ 3 files changed, 136 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index ee9ad3429..e667f65ef 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -426,6 +426,7 @@ func createRun(opts *CreateOptions) error { } hasAssets := len(opts.Assets) > 0 + draftWhileUploading := false if hasAssets && !opts.Draft { // Check for an existing release @@ -437,6 +438,7 @@ func createRun(opts *CreateOptions) error { } } // Save the release initially as draft and publish it after all assets have finished uploading + draftWhileUploading = true params["draft"] = true } @@ -445,6 +447,16 @@ func createRun(opts *CreateOptions) error { return err } + cleanupDraftRelease := func(err error) error { + if !draftWhileUploading { + return err + } + if cleanupErr := deleteRelease(httpClient, newRelease); cleanupErr != nil { + return fmt.Errorf("%w\ncleaning up draft failed: %v", err, cleanupErr) + } + return err + } + if hasAssets { uploadURL := newRelease.UploadURL if idx := strings.IndexRune(uploadURL, '{'); idx > 0 { @@ -455,13 +467,13 @@ func createRun(opts *CreateOptions) error { err = shared.ConcurrentUpload(httpClient, uploadURL, opts.Concurrency, opts.Assets) opts.IO.StopProgressIndicator() if err != nil { - return err + return cleanupDraftRelease(err) } - if !opts.Draft { + if draftWhileUploading { rel, err := publishRelease(httpClient, newRelease.APIURL, opts.DiscussionCategory) if err != nil { - return err + return cleanupDraftRelease(err) } newRelease = rel } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index aec10911a..eef4e05b5 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -728,6 +728,102 @@ func Test_createRun(t *testing.T) { wantStderr: ``, wantErr: `a release with the same tag name already exists: v1.2.3`, }, + { + name: "clean up draft after uploading files fails", + isTTY: false, + opts: CreateOptions{ + TagName: "v1.2.3", + Name: "", + Body: "", + BodyProvided: true, + Draft: false, + Target: "", + Assets: []*shared.AssetForUpload{ + { + Name: "ball.tgz", + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`TARBALL`)), nil + }, + }, + }, + Concurrency: 1, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register(httpmock.REST("HEAD", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StatusStringResponse(404, ``)) + reg.Register(httpmock.REST("POST", "repos/OWNER/REPO/releases"), httpmock.StatusStringResponse(201, `{ + "url": "https://api.github.com/releases/123", + "upload_url": "https://api.github.com/assets/upload", + "html_url": "https://github.com/OWNER/REPO/releases/tag/v1.2.3" + }`)) + reg.Register(httpmock.REST("POST", "assets/upload"), httpmock.StatusStringResponse(500, `{}`)) + reg.Register(httpmock.REST("DELETE", "releases/123"), httpmock.StatusStringResponse(204, ``)) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: `HTTP 500 (https://api.github.com/assets/upload?label=&name=ball.tgz)`, + }, + { + name: "clean up draft after publishing fails", + isTTY: false, + opts: CreateOptions{ + TagName: "v1.2.3", + Name: "", + Body: "", + BodyProvided: true, + Draft: false, + Target: "", + Assets: []*shared.AssetForUpload{ + { + Name: "ball.tgz", + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`TARBALL`)), nil + }, + }, + }, + Concurrency: 1, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register(httpmock.REST("HEAD", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StatusStringResponse(404, ``)) + reg.Register(httpmock.REST("POST", "repos/OWNER/REPO/releases"), httpmock.StatusStringResponse(201, `{ + "url": "https://api.github.com/releases/123", + "upload_url": "https://api.github.com/assets/upload", + "html_url": "https://github.com/OWNER/REPO/releases/tag/v1.2.3" + }`)) + reg.Register(httpmock.REST("POST", "assets/upload"), httpmock.StatusStringResponse(201, `{}`)) + reg.Register(httpmock.REST("PATCH", "releases/123"), httpmock.StatusStringResponse(500, `{}`)) + reg.Register(httpmock.REST("DELETE", "releases/123"), httpmock.StatusStringResponse(204, ``)) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: `HTTP 500 (https://api.github.com/releases/123)`, + }, + { + name: "upload files but release already exists", + isTTY: true, + opts: CreateOptions{ + TagName: "v1.2.3", + Name: "", + Body: "", + BodyProvided: true, + Draft: false, + Target: "", + Assets: []*shared.AssetForUpload{ + { + Name: "ball.tgz", + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`TARBALL`)), nil + }, + }, + }, + Concurrency: 1, + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register(httpmock.REST("HEAD", "repos/OWNER/REPO/releases/tags/v1.2.3"), httpmock.StatusStringResponse(200, ``)) + }, + wantStdout: ``, + wantStderr: ``, + wantErr: `a release with the same tag name already exists: v1.2.3`, + }, { name: "upload files and create discussion", isTTY: true, diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 2a0b65468..f970d46f1 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -225,3 +225,28 @@ func publishRelease(httpClient *http.Client, releaseURL string, discussionCatego err = json.Unmarshal(b, &release) return &release, err } + +func deleteRelease(httpClient *http.Client, release *shared.Release) error { + req, err := http.NewRequest("DELETE", release.APIURL, nil) + if err != nil { + return err + } + + resp, err := httpClient.Do(req) + if err != nil { + return err + } + if resp.Body != nil { + defer resp.Body.Close() + } + + success := resp.StatusCode >= 200 && resp.StatusCode < 300 + if !success { + return api.HandleHTTPError(resp) + } + + if resp.StatusCode != 204 { + _, _ = io.Copy(io.Discard, resp.Body) + } + return nil +} From 8cb312a620a2e8171c835a80e624c980048d68c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 14 Dec 2022 21:19:13 +0100 Subject: [PATCH 4/4] Fix release assets upload retry logic Enables up to 3 retries of uploading a single asset when encountering a network error or a HTTP 5xx error. Bonus: - simplifies ConcurrentUpload implementation - support Go context cancellation --- go.mod | 2 +- go.sum | 4 +- pkg/cmd/release/create/create_test.go | 4 +- pkg/cmd/release/shared/upload.go | 88 +++++++++++++-------------- pkg/cmd/release/shared/upload_test.go | 55 ++++++++++++++++- 5 files changed, 103 insertions(+), 50 deletions(-) diff --git a/go.mod b/go.mod index f2f0eac9b..e1f9d849b 100644 --- a/go.mod +++ b/go.mod @@ -36,7 +36,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.5 golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 - golang.org/x/sync v0.0.0-20210220032951-036812b2e83c + golang.org/x/sync v0.1.0 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 golang.org/x/text v0.3.8 google.golang.org/grpc v1.49.0 diff --git a/go.sum b/go.sum index 4816ac2c1..53d80dced 100644 --- a/go.sum +++ b/go.sum @@ -342,8 +342,8 @@ golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20200625203802-6e8e738ad208/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.0.0-20210220032951-036812b2e83c h1:5KslGYwFpkhGh+Q16bwMP3cOontH8FOep7tGV86Y7SQ= -golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= +golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190222072716-a9d3bda3a223/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index eef4e05b5..ee5c1caac 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -755,12 +755,12 @@ func Test_createRun(t *testing.T) { "upload_url": "https://api.github.com/assets/upload", "html_url": "https://github.com/OWNER/REPO/releases/tag/v1.2.3" }`)) - reg.Register(httpmock.REST("POST", "assets/upload"), httpmock.StatusStringResponse(500, `{}`)) + reg.Register(httpmock.REST("POST", "assets/upload"), httpmock.StatusStringResponse(422, `{}`)) reg.Register(httpmock.REST("DELETE", "releases/123"), httpmock.StatusStringResponse(204, ``)) }, wantStdout: ``, wantStderr: ``, - wantErr: `HTTP 500 (https://api.github.com/assets/upload?label=&name=ball.tgz)`, + wantErr: `HTTP 422 (https://api.github.com/assets/upload?label=&name=ball.tgz)`, }, { name: "clean up draft after publishing fails", diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 802f623c4..939deffe7 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -1,6 +1,7 @@ package shared import ( + "context" "encoding/json" "errors" "io" @@ -13,8 +14,15 @@ import ( "time" "github.com/cli/cli/v2/api" + "golang.org/x/sync/errgroup" ) +type httpDoer interface { + Do(*http.Request) (*http.Response, error) +} + +type errNetwork struct{ error } + type AssetForUpload struct { Name string Label string @@ -90,64 +98,61 @@ func fileExt(fn string) string { return path.Ext(fn) } -func ConcurrentUpload(httpClient *http.Client, uploadURL string, numWorkers int, assets []*AssetForUpload) error { +func ConcurrentUpload(httpClient httpDoer, uploadURL string, numWorkers int, assets []*AssetForUpload) error { if numWorkers == 0 { return errors.New("the number of concurrent workers needs to be greater than 0") } - jobs := make(chan AssetForUpload, len(assets)) - results := make(chan error, len(assets)) - - if len(assets) < numWorkers { - numWorkers = len(assets) - } - - for w := 1; w <= numWorkers; w++ { - go func() { - for a := range jobs { - results <- uploadWithDelete(httpClient, uploadURL, a) - } - }() - } + ctx := context.Background() + g, gctx := errgroup.WithContext(ctx) + g.SetLimit(numWorkers) for _, a := range assets { - jobs <- *a + asset := *a + g.Go(func() error { + return uploadWithDelete(gctx, httpClient, uploadURL, asset) + }) } - close(jobs) - var uploadError error - for i := 0; i < len(assets); i++ { - if err := <-results; err != nil { - uploadError = err - } - } - return uploadError + return g.Wait() } +var retryInterval = time.Millisecond * 200 + const maxRetries = 3 -func uploadWithDelete(httpClient *http.Client, uploadURL string, a AssetForUpload) error { +func shouldRetry(err error) bool { + var networkError errNetwork + if errors.As(err, &networkError) { + return true + } + var httpError api.HTTPError + return errors.As(err, &httpError) && httpError.StatusCode >= 500 +} + +func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL string, a AssetForUpload) error { if a.ExistingURL != "" { - err := deleteAsset(httpClient, a.ExistingURL) - if err != nil { + if err := deleteAsset(ctx, httpClient, a.ExistingURL); err != nil { return err } } retries := 0 for { - var httpError api.HTTPError - _, err := uploadAsset(httpClient, uploadURL, a) - // retry upload several times upon receiving HTTP 5xx - if err == nil || !errors.As(err, &httpError) || httpError.StatusCode < 500 || retries < maxRetries { + _, err := uploadAsset(ctx, httpClient, uploadURL, a) + if err == nil || retries == maxRetries || !shouldRetry(err) { return err } retries++ - time.Sleep(time.Duration(retries) * time.Second) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(time.Duration(retries) * retryInterval): + } } } -func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload) (*ReleaseAsset, error) { +func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL string, asset AssetForUpload) (*ReleaseAsset, error) { u, err := url.Parse(uploadURL) if err != nil { return nil, err @@ -163,7 +168,7 @@ func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload } defer f.Close() - req, err := http.NewRequest("POST", u.String(), f) + req, err := http.NewRequestWithContext(ctx, "POST", u.String(), f) if err != nil { return nil, err } @@ -173,7 +178,7 @@ func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload resp, err := httpClient.Do(req) if err != nil { - return nil, err + return nil, errNetwork{err} } defer resp.Body.Close() @@ -182,22 +187,17 @@ func uploadAsset(httpClient *http.Client, uploadURL string, asset AssetForUpload return nil, api.HandleHTTPError(resp) } - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var newAsset ReleaseAsset - err = json.Unmarshal(b, &newAsset) - if err != nil { + dec := json.NewDecoder(resp.Body) + if err := dec.Decode(&newAsset); err != nil { return nil, err } return &newAsset, nil } -func deleteAsset(httpClient *http.Client, assetURL string) error { - req, err := http.NewRequest("DELETE", assetURL, nil) +func deleteAsset(ctx context.Context, httpClient httpDoer, assetURL string) error { + req, err := http.NewRequestWithContext(ctx, "DELETE", assetURL, nil) if err != nil { return err } diff --git a/pkg/cmd/release/shared/upload_test.go b/pkg/cmd/release/shared/upload_test.go index da1aa8918..f2b623312 100644 --- a/pkg/cmd/release/shared/upload_test.go +++ b/pkg/cmd/release/shared/upload_test.go @@ -1,6 +1,14 @@ package shared -import "testing" +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "testing" + "time" +) func Test_typeForFilename(t *testing.T) { tests := []struct { @@ -67,3 +75,48 @@ func Test_typeForFilename(t *testing.T) { }) } } + +func Test_uploadWithDelete_retry(t *testing.T) { + retryInterval = time.Millisecond + ctx := context.Background() + + tries := 0 + client := funcClient(func(req *http.Request) (*http.Response, error) { + tries++ + if tries == 1 { + return nil, errors.New("made up exception") + } else if tries == 2 { + return &http.Response{ + Request: req, + StatusCode: 500, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + }, nil + } + return &http.Response{ + Request: req, + StatusCode: 200, + Body: io.NopCloser(bytes.NewBufferString(`{}`)), + }, nil + }) + err := uploadWithDelete(ctx, client, "http://example.com/upload", AssetForUpload{ + Name: "asset", + Label: "", + Size: 8, + Open: func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewBufferString(`somebody`)), nil + }, + MIMEType: "application/octet-stream", + }) + if err != nil { + t.Errorf("uploadWithDelete() error: %v", err) + } + if tries != 3 { + t.Errorf("tries = %d, expected %d", tries, 3) + } +} + +type funcClient func(*http.Request) (*http.Response, error) + +func (f funcClient) Do(req *http.Request) (*http.Response, error) { + return f(req) +}