STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Improve looking up draft releases by tag name

Browse source 
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.
This commit is contained in:
Mislav Marohnić 2022-11-28 12:27:38 +01:00
parent e58bf212fe
commit 36ffbe18de
No known key found for this signature in database
13 changed files with 164 additions and 124 deletions
Download patch file Download diff file Expand all files Collapse all files

12
api/client.go
View file

@ -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)

3
pkg/cmd/release/delete-asset/delete_asset.go
View file

@ -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
}

15
pkg/cmd/release/delete-asset/delete_asset_test.go
View file

@ -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{}

3
pkg/cmd/release/delete/delete.go
View file

@ -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
}

5
pkg/cmd/release/delete/delete_test.go
View file

@ -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, ""))

7
pkg/cmd/release/download/download.go
View file

@ -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
}

19
pkg/cmd/release/download/download_test.go
View file

@ -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)
}

3
pkg/cmd/release/edit/edit.go
View file

@ -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
}

11
pkg/cmd/release/edit/edit_test.go
View file

@ -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) {

191
pkg/cmd/release/shared/fetch.go
View file

@ -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"])
}))
}
}

3
pkg/cmd/release/upload/upload.go
View file

@ -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
}

6
pkg/cmd/release/view/view.go
View file

@ -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
}

10
pkg/cmd/release/view/view_test.go
View file

@ -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) {