From 28cbfc4aab369f7f6b665979bccf846a5023ccd1 Mon Sep 17 00:00:00 2001 From: wrslatz <20246633+wrslatz@users.noreply.github.com> Date: Fri, 3 Sep 2021 12:18:24 -0400 Subject: [PATCH 01/29] Document installing extensions using full repo URL --- pkg/cmd/extension/command.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 0ea822e8c..8f983c5a3 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -69,6 +69,20 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { &cobra.Command{ Use: "install ", Short: "Install a gh extension from a repository", + Long: heredoc.Doc(` + GitHub CLI extensions are installed from a repository. + + The argument accepts the short form of the repo ("owner/repo") or the + full repository URL. If the full repository URL is not used, gh will install + the extension using the hostname to which gh is currently authenticated. + + Using the full repository URL allows GitHub Enterprise users to install + extensions from public GitHub. + + Examples: + gh extension install owner/repo + gh extension install https://github.com/owner/repo + `), Args: cmdutil.MinimumArgs(1, "must specify a repository to install from"), RunE: func(cmd *cobra.Command, args []string) error { if args[0] == "." { From e8b015b80dfb21a3408ab97961be7aba52a4c58b Mon Sep 17 00:00:00 2001 From: Des Preston Date: Fri, 9 Jul 2021 15:15:35 -0400 Subject: [PATCH 02/29] show warning when limit exceeds search api max Fixes #3839 --- api/client.go | 5 +++++ pkg/cmd/issue/list/http.go | 5 +++++ pkg/cmd/issue/list/list.go | 15 +++++++++++---- pkg/cmd/pr/list/http.go | 5 +++++ pkg/cmd/pr/list/http_test.go | 11 +++++++++++ pkg/cmd/pr/list/list.go | 15 +++++++++++---- pkg/cmd/repo/list/http.go | 4 ++++ pkg/cmd/repo/list/list.go | 15 +++++++++++---- 8 files changed, 63 insertions(+), 12 deletions(-) diff --git a/api/client.go b/api/client.go index a5741a42e..c7e8663ab 100644 --- a/api/client.go +++ b/api/client.go @@ -3,6 +3,7 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -16,6 +17,10 @@ import ( "github.com/shurcooL/graphql" ) +// The Search API returns a max of 1000 results for each search. +// (https://docs.github.com/en/rest/reference/search#about-the-search-api) +var ErrSearchAPIMaxLimit = errors.New("limit was set to >1000 but Github search API returns max 1000 results") + // ClientOption represents an argument to NewClient type ClientOption = func(http.RoundTripper) http.RoundTripper diff --git a/pkg/cmd/issue/list/http.go b/pkg/cmd/issue/list/http.go index 905615256..b9dac4d0d 100644 --- a/pkg/cmd/issue/list/http.go +++ b/pkg/cmd/issue/list/http.go @@ -126,6 +126,11 @@ loop: } res := api.IssuesAndTotalCount{Issues: issues, TotalCount: totalCount} + + if limit > 1000 { + return &res, api.ErrSearchAPIMaxLimit + } + return &res, nil } diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index 48d76d23e..2b64bc918 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -151,9 +151,9 @@ func listRun(opts *ListOptions) error { filterOptions.Fields = opts.Exporter.Fields() } - listResult, err := issueList(httpClient, baseRepo, filterOptions, opts.LimitResults) - if err != nil { - return err + listResult, listErr := issueList(httpClient, baseRepo, filterOptions, opts.LimitResults) + if listErr != nil && listErr != api.ErrSearchAPIMaxLimit { + return listErr } err = opts.IO.StartPager() @@ -168,7 +168,14 @@ func listRun(opts *ListOptions) error { if isTerminal { title := prShared.ListHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, !filterOptions.IsDefault()) - fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) + out := fmt.Sprintf("\n%s\n", title) + + if listErr == api.ErrSearchAPIMaxLimit { + icon := opts.IO.ColorScheme().WarningIcon() + out = fmt.Sprintf("%s%s warning: %s\n", out, icon, listErr.Error()) + } + + fmt.Fprintf(opts.IO.Out, "%s\n", out) } issueShared.PrintIssues(opts.IO, "", len(listResult.Issues), listResult.Issues) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 621853aaf..b27e14794 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -183,6 +183,7 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters } res := api.PullRequestAndTotalCount{} + var check = make(map[int]struct{}) client := api.NewClientFromHTTP(httpClient) @@ -217,6 +218,10 @@ loop: } } + if limit > 1000 { + return &res, api.ErrSearchAPIMaxLimit + } + return &res, nil } diff --git a/pkg/cmd/pr/list/http_test.go b/pkg/cmd/pr/list/http_test.go index bdb136100..2b8f3ab1e 100644 --- a/pkg/cmd/pr/list/http_test.go +++ b/pkg/cmd/pr/list/http_test.go @@ -144,6 +144,17 @@ func Test_listPullRequests(t *testing.T) { })) }, }, + { + name: "with labels and limit above 1000", + args: args{ + repo: ghrepo.New("OWNER", "REPO"), + limit: 2000, + filters: prShared.FilterOptions{ + Labels: []string{"hello", "one world"}, + }, + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 15c943541..a5446090b 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -151,9 +151,9 @@ func listRun(opts *ListOptions) error { return opts.Browser.Browse(openURL) } - listResult, err := listPullRequests(httpClient, baseRepo, filters, opts.LimitResults) - if err != nil { - return err + listResult, listErr := listPullRequests(httpClient, baseRepo, filters, opts.LimitResults) + if listErr != nil && listErr != api.ErrSearchAPIMaxLimit { + return listErr } err = opts.IO.StartPager() @@ -168,7 +168,14 @@ func listRun(opts *ListOptions) error { if opts.IO.IsStdoutTTY() { title := shared.ListHeader(ghrepo.FullName(baseRepo), "pull request", len(listResult.PullRequests), listResult.TotalCount, !filters.IsDefault()) - fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) + out := fmt.Sprintf("\n%s\n", title) + + if listErr == api.ErrSearchAPIMaxLimit { + icon := opts.IO.ColorScheme().WarningIcon() + out = fmt.Sprintf("%s%s warning: %s\n", out, icon, listErr.Error()) + } + + fmt.Fprintln(opts.IO.Out, out) } cs := opts.IO.ColorScheme() diff --git a/pkg/cmd/repo/list/http.go b/pkg/cmd/repo/list/http.go index fa5319304..0ea40e924 100644 --- a/pkg/cmd/repo/list/http.go +++ b/pkg/cmd/repo/list/http.go @@ -175,6 +175,10 @@ pagination: variables["endCursor"] = githubv4.String(result.Search.PageInfo.EndCursor) } + if limit > 1000 { + return &listResult, api.ErrSearchAPIMaxLimit + } + return &listResult, nil } diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index 7387ca84d..089562d81 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -130,9 +130,9 @@ func listRun(opts *ListOptions) error { return err } - listResult, err := listRepos(httpClient, host, opts.Limit, opts.Owner, filter) - if err != nil { - return err + listResult, listErr := listRepos(httpClient, host, opts.Limit, opts.Owner, filter) + if listErr != nil { + return listErr } if err := opts.IO.StartPager(); err != nil { @@ -174,7 +174,14 @@ func listRun(opts *ListOptions) error { if opts.IO.IsStdoutTTY() { hasFilters := filter.Visibility != "" || filter.Fork || filter.Source || filter.Language != "" || filter.Topic != "" title := listHeader(listResult.Owner, len(listResult.Repositories), listResult.TotalCount, hasFilters) - fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) + out := fmt.Sprintf("\n%s\n", title) + + if listErr == api.ErrSearchAPIMaxLimit { + icon := opts.IO.ColorScheme().WarningIcon() + out = fmt.Sprintf("%s%s warning: %s\n", out, icon, listErr.Error()) + } + + fmt.Fprintln(opts.IO.Out, out) } return tp.Render() From 619333adb60203aff2e19455790a353c76fa3628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 6 Sep 2021 16:00:31 +0200 Subject: [PATCH 03/29] Avoid using error values to pass information about the search cap --- api/client.go | 5 ----- api/queries_issue.go | 5 +++-- api/queries_pr.go | 1 + pkg/cmd/issue/list/http.go | 7 +------ pkg/cmd/issue/list/list.go | 18 +++++++----------- pkg/cmd/pr/list/http.go | 7 +------ pkg/cmd/pr/list/http_test.go | 11 ----------- pkg/cmd/pr/list/list.go | 18 +++++++----------- pkg/cmd/repo/list/http.go | 4 ---- pkg/cmd/repo/list/list.go | 18 +++++++----------- 10 files changed, 27 insertions(+), 67 deletions(-) diff --git a/api/client.go b/api/client.go index c7e8663ab..a5741a42e 100644 --- a/api/client.go +++ b/api/client.go @@ -3,7 +3,6 @@ package api import ( "bytes" "encoding/json" - "errors" "fmt" "io" "io/ioutil" @@ -17,10 +16,6 @@ import ( "github.com/shurcooL/graphql" ) -// The Search API returns a max of 1000 results for each search. -// (https://docs.github.com/en/rest/reference/search#about-the-search-api) -var ErrSearchAPIMaxLimit = errors.New("limit was set to >1000 but Github search API returns max 1000 results") - // ClientOption represents an argument to NewClient type ClientOption = func(http.RoundTripper) http.RoundTripper diff --git a/api/queries_issue.go b/api/queries_issue.go index d035e6ad8..d09497569 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -16,8 +16,9 @@ type IssuesPayload struct { } type IssuesAndTotalCount struct { - Issues []Issue - TotalCount int + Issues []Issue + TotalCount int + SearchCapped bool } type Issue struct { diff --git a/api/queries_pr.go b/api/queries_pr.go index 1e060b1da..388446f42 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -26,6 +26,7 @@ type PullRequestsPayload struct { type PullRequestAndTotalCount struct { TotalCount int PullRequests []PullRequest + SearchCapped bool } type PullRequest struct { diff --git a/pkg/cmd/issue/list/http.go b/pkg/cmd/issue/list/http.go index b9dac4d0d..a992b553d 100644 --- a/pkg/cmd/issue/list/http.go +++ b/pkg/cmd/issue/list/http.go @@ -126,11 +126,6 @@ loop: } res := api.IssuesAndTotalCount{Issues: issues, TotalCount: totalCount} - - if limit > 1000 { - return &res, api.ErrSearchAPIMaxLimit - } - return &res, nil } @@ -176,7 +171,7 @@ func searchIssues(client *api.Client, repo ghrepo.Interface, filters prShared.Fi "query": searchQuery, } - ic := api.IssuesAndTotalCount{} + ic := api.IssuesAndTotalCount{SearchCapped: limit > 1000} loop: for { diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index 2b64bc918..2db0815b4 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -151,9 +151,9 @@ func listRun(opts *ListOptions) error { filterOptions.Fields = opts.Exporter.Fields() } - listResult, listErr := issueList(httpClient, baseRepo, filterOptions, opts.LimitResults) - if listErr != nil && listErr != api.ErrSearchAPIMaxLimit { - return listErr + listResult, err := issueList(httpClient, baseRepo, filterOptions, opts.LimitResults) + if err != nil { + return err } err = opts.IO.StartPager() @@ -166,16 +166,12 @@ func listRun(opts *ListOptions) error { return opts.Exporter.Write(opts.IO, listResult.Issues) } + if listResult.SearchCapped { + fmt.Fprintln(opts.IO.ErrOut, "warning: this query uses the Search API which is capped at 1000 results maximum") + } if isTerminal { title := prShared.ListHeader(ghrepo.FullName(baseRepo), "issue", len(listResult.Issues), listResult.TotalCount, !filterOptions.IsDefault()) - out := fmt.Sprintf("\n%s\n", title) - - if listErr == api.ErrSearchAPIMaxLimit { - icon := opts.IO.ColorScheme().WarningIcon() - out = fmt.Sprintf("%s%s warning: %s\n", out, icon, listErr.Error()) - } - - fmt.Fprintf(opts.IO.Out, "%s\n", out) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) } issueShared.PrintIssues(opts.IO, "", len(listResult.Issues), listResult.Issues) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index b27e14794..47cbd9d61 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -182,8 +182,7 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters "q": q.String(), } - res := api.PullRequestAndTotalCount{} - + res := api.PullRequestAndTotalCount{SearchCapped: limit > 1000} var check = make(map[int]struct{}) client := api.NewClientFromHTTP(httpClient) @@ -218,10 +217,6 @@ loop: } } - if limit > 1000 { - return &res, api.ErrSearchAPIMaxLimit - } - return &res, nil } diff --git a/pkg/cmd/pr/list/http_test.go b/pkg/cmd/pr/list/http_test.go index 2b8f3ab1e..bdb136100 100644 --- a/pkg/cmd/pr/list/http_test.go +++ b/pkg/cmd/pr/list/http_test.go @@ -144,17 +144,6 @@ func Test_listPullRequests(t *testing.T) { })) }, }, - { - name: "with labels and limit above 1000", - args: args{ - repo: ghrepo.New("OWNER", "REPO"), - limit: 2000, - filters: prShared.FilterOptions{ - Labels: []string{"hello", "one world"}, - }, - }, - wantErr: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index a5446090b..dd62da726 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -151,9 +151,9 @@ func listRun(opts *ListOptions) error { return opts.Browser.Browse(openURL) } - listResult, listErr := listPullRequests(httpClient, baseRepo, filters, opts.LimitResults) - if listErr != nil && listErr != api.ErrSearchAPIMaxLimit { - return listErr + listResult, err := listPullRequests(httpClient, baseRepo, filters, opts.LimitResults) + if err != nil { + return err } err = opts.IO.StartPager() @@ -166,16 +166,12 @@ func listRun(opts *ListOptions) error { return opts.Exporter.Write(opts.IO, listResult.PullRequests) } + if listResult.SearchCapped { + fmt.Fprintln(opts.IO.ErrOut, "warning: this query uses the Search API which is capped at 1000 results maximum") + } if opts.IO.IsStdoutTTY() { title := shared.ListHeader(ghrepo.FullName(baseRepo), "pull request", len(listResult.PullRequests), listResult.TotalCount, !filters.IsDefault()) - out := fmt.Sprintf("\n%s\n", title) - - if listErr == api.ErrSearchAPIMaxLimit { - icon := opts.IO.ColorScheme().WarningIcon() - out = fmt.Sprintf("%s%s warning: %s\n", out, icon, listErr.Error()) - } - - fmt.Fprintln(opts.IO.Out, out) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) } cs := opts.IO.ColorScheme() diff --git a/pkg/cmd/repo/list/http.go b/pkg/cmd/repo/list/http.go index 0ea40e924..fa5319304 100644 --- a/pkg/cmd/repo/list/http.go +++ b/pkg/cmd/repo/list/http.go @@ -175,10 +175,6 @@ pagination: variables["endCursor"] = githubv4.String(result.Search.PageInfo.EndCursor) } - if limit > 1000 { - return &listResult, api.ErrSearchAPIMaxLimit - } - return &listResult, nil } diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index 089562d81..af6f505c0 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -130,9 +130,9 @@ func listRun(opts *ListOptions) error { return err } - listResult, listErr := listRepos(httpClient, host, opts.Limit, opts.Owner, filter) - if listErr != nil { - return listErr + listResult, err := listRepos(httpClient, host, opts.Limit, opts.Owner, filter) + if err != nil { + return err } if err := opts.IO.StartPager(); err != nil { @@ -171,17 +171,13 @@ func listRun(opts *ListOptions) error { tp.EndRow() } + if listResult.FromSearch && opts.Limit > 1000 { + fmt.Fprintln(opts.IO.ErrOut, "warning: this query uses the Search API which is capped at 1000 results maximum") + } if opts.IO.IsStdoutTTY() { hasFilters := filter.Visibility != "" || filter.Fork || filter.Source || filter.Language != "" || filter.Topic != "" title := listHeader(listResult.Owner, len(listResult.Repositories), listResult.TotalCount, hasFilters) - out := fmt.Sprintf("\n%s\n", title) - - if listErr == api.ErrSearchAPIMaxLimit { - icon := opts.IO.ColorScheme().WarningIcon() - out = fmt.Sprintf("%s%s warning: %s\n", out, icon, listErr.Error()) - } - - fmt.Fprintln(opts.IO.Out, out) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) } return tp.Render() From 2a2088dc891daaeea7f0a81d2d2b5bcde166d5db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 6 Sep 2021 20:04:40 +0200 Subject: [PATCH 04/29] :nail_care: tweak extension docs --- pkg/cmd/extension/command.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 8f983c5a3..3d5215829 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -30,6 +30,8 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { will be forwarded to the %[1]sgh-%[1]s executable of the extension. An extension cannot override any of the core gh commands. + + See the list of available extensions at `, "`"), Aliases: []string{"extensions"}, } @@ -67,23 +69,25 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { }, }, &cobra.Command{ - Use: "install ", + Use: "install ", Short: "Install a gh extension from a repository", Long: heredoc.Doc(` - GitHub CLI extensions are installed from a repository. + Install a GitHub repository locally as a GitHub CLI extension. - The argument accepts the short form of the repo ("owner/repo") or the - full repository URL. If the full repository URL is not used, gh will install - the extension using the hostname to which gh is currently authenticated. + The repository argument can be specified in "owner/repo" format as well as a full URL. + The URL format is useful when the repository is not hosted on github.com. - Using the full repository URL allows GitHub Enterprise users to install - extensions from public GitHub. + To install an extension in development from the current directory, use "." as the + value of the repository argument. - Examples: - gh extension install owner/repo - gh extension install https://github.com/owner/repo + See the list of available extensions at `), - Args: cmdutil.MinimumArgs(1, "must specify a repository to install from"), + Example: heredoc.Doc(` + $ gh extension install owner/gh-extension + $ gh extension install https://git.example.com/owner/gh-extension + $ gh extension install . + `), + Args: cmdutil.MinimumArgs(1, "must specify a repository to install from"), RunE: func(cmd *cobra.Command, args []string) error { if args[0] == "." { wd, err := os.Getwd() From 7519421fc0a712e1a7a28628f035ac77ce2de33c Mon Sep 17 00:00:00 2001 From: Andrew Hsu Date: Thu, 9 Sep 2021 10:24:42 -0500 Subject: [PATCH 05/29] fix link to code in project-layout doc Update link to code so it matches given example in doc: `gh help issue list`. --- docs/project-layout.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/project-layout.md b/docs/project-layout.md index 4bbdbad40..60d0c2aac 100644 --- a/docs/project-layout.md +++ b/docs/project-layout.md @@ -24,7 +24,7 @@ commands is: pkg/cmd///.go ``` Following the above example, the main implementation for the `gh issue list` command, including its help -text, is in [pkg/cmd/issue/view/view.go](../pkg/cmd/issue/view/view.go) +text, is in [pkg/cmd/issue/list/list.go](../pkg/cmd/issue/list/list.go) Other help topics not specific to any command, for example `gh help environment`, are found in [pkg/cmd/root/help_topic.go](../pkg/cmd/root/help_topic.go). From 11466fda12b7ca98780b3b9981d232e2fd9e6e21 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 20:48:23 +0300 Subject: [PATCH 06/29] Add --draft and --non-draft filters to gh pr list --- pkg/cmd/pr/list/http.go | 12 ++++- pkg/cmd/pr/list/list.go | 12 ++++- pkg/cmd/pr/list/list_test.go | 91 ++++++++++++++++++++++++++++---- pkg/cmd/pr/shared/params.go | 7 +++ pkg/cmd/pr/shared/params_test.go | 28 ++++++++++ pkg/githubsearch/query.go | 8 +++ 6 files changed, 146 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 621853aaf..04ced2cca 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -10,8 +10,12 @@ import ( "github.com/cli/cli/v2/pkg/githubsearch" ) +func shouldUseSearch(filters prShared.FilterOptions) bool { + return filters.Draft || filters.NonDraft || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 +} + func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { - if filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 { + if shouldUseSearch(filters) { return searchPullRequests(httpClient, repo, filters, limit) } @@ -177,6 +181,12 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters q.SetBaseBranch(filters.BaseBranch) } + if filters.Draft { + q.SetDraft(true) + } else if filters.NonDraft { + q.SetDraft(false) + } + pageLimit := min(limit, 100) variables := map[string]interface{}{ "q": q.String(), diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 15c943541..4ef060bb7 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -37,6 +37,8 @@ type ListOptions struct { Author string Assignee string Search string + Draft bool + NonDraft bool } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { @@ -74,6 +76,10 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} } + if opts.Draft && opts.NonDraft { + return &cmdutil.FlagError{Err: fmt.Errorf("specify only one of `--draft` or `--non-draft`")} + } + if runF != nil { return runF(opts) } @@ -92,6 +98,9 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Author, "author", "A", "", "Filter by author") cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search pull requests with `query`") + cmd.Flags().BoolVar(&opts.Draft, "draft", false, "Show drafts only") + cmd.Flags().BoolVar(&opts.NonDraft, "non-draft", false, "Show non-drafts only") + cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.PullRequestFields) return cmd @@ -132,12 +141,13 @@ func listRun(opts *ListOptions) error { Labels: opts.Labels, BaseBranch: opts.BaseBranch, Search: opts.Search, + Draft: opts.Draft, + NonDraft: opts.NonDraft, Fields: defaultFields, } if opts.Exporter != nil { filters.Fields = opts.Exporter.Fields() } - if opts.WebMode { prListURL := ghrepo.GenerateRepoURL(baseRepo, "pulls") openURL, err := shared.ListURLWithQuery(prListURL, filters) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index a912e4cb6..5d0086999 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -186,6 +186,52 @@ func TestPRList_filteringAssigneeLabels(t *testing.T) { } } +func TestPRList_bothNonDraftAndDraft(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + _, err := runCommand(http, true, `--draft --non-draft`) + if err == nil || err.Error() != "specify only one of `--draft` or `--non-draft`" { + t.Fatal(err) + } +} + +func TestPRList_filteringDraft(t *testing.T) { + tests := []struct { + name string + cli string + expectedQuery string + }{{ + "draft", + "--draft", + `repo:OWNER/REPO is:pr is:open draft:true`, + }, + { + "non-draft", + "--non-draft", + `repo:OWNER/REPO is:pr is:open draft:false`, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestSearch\b`), + httpmock.GraphQLQuery(`{}`, func(_ string, params map[string]interface{}) { + assert.Equal(t, test.expectedQuery, params["q"].(string)) + })) + + _, err := runCommand(http, true, test.cli) + if err != nil { + t.Fatal(err) + } + }) + } + +} + func TestPRList_withInvalidLimitFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) @@ -197,18 +243,43 @@ func TestPRList_withInvalidLimitFlag(t *testing.T) { } func TestPRList_web(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) + tests := []struct { + name string + cli string + expectedBrowserURL string + }{{ + "test", + "-a peter -l bug -l docs -L 10 -s merged -B trunk", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", + }, + { + "draft", + "--draft", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", + }, + { + "non-draft", + "--non-draft", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", + }} - _, cmdTeardown := run.Stub() - defer cmdTeardown(t) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) - output, err := runCommand(http, true, "--web -a peter -l bug -l docs -L 10 -s merged -B trunk") - if err != nil { - t.Errorf("error running command `pr list` with `--web` flag: %v", err) + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + output, err := runCommand(http, true, "--web "+test.cli) + if err != nil { + t.Errorf("error running command `pr list` with `--web` flag: %v", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "Opening github.com/OWNER/REPO/pulls in your browser.\n", output.Stderr()) + assert.Equal(t, test.expectedBrowserURL, output.BrowsedURL) + }) } - assert.Equal(t, "", output.String()) - assert.Equal(t, "Opening github.com/OWNER/REPO/pulls in your browser.\n", output.Stderr()) - assert.Equal(t, "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", output.BrowsedURL) } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 92096e9d8..5e5ec4ef8 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -157,6 +157,8 @@ type FilterOptions struct { Mention string Milestone string Search string + Draft bool + NonDraft bool Fields []string } @@ -241,6 +243,11 @@ func SearchQueryBuild(options FilterOptions) string { if options.Search != "" { q.AddQuery(options.Search) } + if options.Draft { + q.SetDraft(true) + } else if options.NonDraft { + q.SetDraft(false) + } return q.String() } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index fa41ac307..696950722 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -34,6 +34,34 @@ func Test_listURLWithQuery(t *testing.T) { want: "https://example.com/path?a=b&q=is%3Aissue+is%3Aopen", wantErr: false, }, + { + name: "draft", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "pr", + State: "open", + Draft: true, + NonDraft: true, // shouldn't impact anything - we always prefer `draft` + }, + }, + want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Atrue", + wantErr: false, + }, + { + name: "non-draft", + args: args{ + listURL: "https://example.com/path", + options: FilterOptions{ + Entity: "pr", + State: "open", + Draft: false, + NonDraft: true, + }, + }, + want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Afalse", + wantErr: false, + }, { name: "all", args: args{ diff --git a/pkg/githubsearch/query.go b/pkg/githubsearch/query.go index f25707710..b0938613a 100644 --- a/pkg/githubsearch/query.go +++ b/pkg/githubsearch/query.go @@ -54,6 +54,7 @@ type Query struct { forkState string visibility string isArchived *bool + isDraft *bool } func (q *Query) InRepository(nameWithOwner string) { @@ -139,6 +140,10 @@ func (q *Query) SetArchived(isArchived bool) { q.isArchived = &isArchived } +func (q *Query) SetDraft(isDraft bool) { + q.isDraft = &isDraft +} + func (q *Query) String() string { var qs string @@ -198,6 +203,9 @@ func (q *Query) String() string { if q.headBranch != "" { qs += fmt.Sprintf("head:%s ", quote(q.headBranch)) } + if q.isDraft != nil { + qs += fmt.Sprintf("draft:%v ", *q.isDraft) + } if q.sort != "" { qs += fmt.Sprintf("sort:%s ", q.sort) From 373d1efb583bc65a71ba3838ed4d60e6aaa6718f Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 20:50:21 +0300 Subject: [PATCH 07/29] format code --- pkg/cmd/pr/list/list_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 5d0086999..1823175c7 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -201,11 +201,12 @@ func TestPRList_filteringDraft(t *testing.T) { name string cli string expectedQuery string - }{{ - "draft", - "--draft", - `repo:OWNER/REPO is:pr is:open draft:true`, - }, + }{ + { + "draft", + "--draft", + `repo:OWNER/REPO is:pr is:open draft:true`, + }, { "non-draft", "--non-draft", @@ -247,11 +248,12 @@ func TestPRList_web(t *testing.T) { name string cli string expectedBrowserURL string - }{{ - "test", - "-a peter -l bug -l docs -L 10 -s merged -B trunk", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", - }, + }{ + { + "test", + "-a peter -l bug -l docs -L 10 -s merged -B trunk", + "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", + }, { "draft", "--draft", From f3053c36287078aa144ff7ac259ac5d89de0a3e0 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 20:58:13 +0300 Subject: [PATCH 08/29] fix tests --- pkg/cmd/pr/list/list_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 1823175c7..2d3aa62aa 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -191,9 +191,8 @@ func TestPRList_bothNonDraftAndDraft(t *testing.T) { defer http.Verify(t) _, err := runCommand(http, true, `--draft --non-draft`) - if err == nil || err.Error() != "specify only one of `--draft` or `--non-draft`" { - t.Fatal(err) - } + assert.NotNil(t, err) + assert.Equal(t, err.Error(), "specify only one of `--draft` or `--non-draft`") } func TestPRList_filteringDraft(t *testing.T) { @@ -238,9 +237,8 @@ func TestPRList_withInvalidLimitFlag(t *testing.T) { defer http.Verify(t) _, err := runCommand(http, true, `--limit=0`) - if err == nil && err.Error() != "invalid limit: 0" { - t.Errorf("error running command `issue list`: %v", err) - } + assert.NotNil(t, err) + assert.Equal(t, err.Error(), "invalid value for --limit: 0") } func TestPRList_web(t *testing.T) { From 1926971a762325d3f8a46b08efcb54770ab2aa8c Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Sun, 12 Sep 2021 21:04:03 +0300 Subject: [PATCH 09/29] Remove non-relevant test --- pkg/cmd/pr/list/list_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 2d3aa62aa..700a03f08 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -176,16 +176,6 @@ func TestPRList_filteringAssignee(t *testing.T) { } } -func TestPRList_filteringAssigneeLabels(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - - _, err := runCommand(http, true, `-l one,two -a hubot`) - if err == nil && err.Error() != "multiple labels with --assignee are not supported" { - t.Fatal(err) - } -} - func TestPRList_bothNonDraftAndDraft(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) From 2f45173370be73d6a1475c18fdadaf89c54cf7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 13 Sep 2021 13:28:47 +0200 Subject: [PATCH 10/29] Publish docs site using a deploy key instead of PAT I'd like to decommission SITE_GITHUB_TOKEN as it's a PAT that has write access to all my `github/*` repositories. Instead, I've created a deploy key that only has access to `github/cli.github.com`. ssh-keygen -t ed25519 -C "gh docs push" -N "" -f ~/.ssh/gh-docs-publish gh repo -R github/cli.github.com deploy-key add ~/.ssh/gh-docs-publish.pub # testing: GIT_SSH_COMMAND='ssh -i $HOME/.ssh/gh-docs-publish' git push ... --- .github/workflows/releases.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index 79e150113..6f9f6547a 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -33,7 +33,7 @@ jobs: repository: github/cli.github.com path: site fetch-depth: 0 - token: ${{secrets.SITE_GITHUB_TOKEN}} + ssh-key: ${{secrets.SITE_SSH_KEY}} - name: Update site man pages env: GIT_COMMITTER_NAME: cli automation From a8492bb0eab73171b46fa2c3871a2cfb71a9bf34 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 13 Sep 2021 10:14:20 -0700 Subject: [PATCH 11/29] Allow user input for git fetch in repo sync --- pkg/cmd/repo/sync/git.go | 10 ++++++++-- pkg/cmd/repo/sync/sync.go | 11 ++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index ffb9a4179..1ec86b053 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/pkg/iostreams" ) type gitClient interface { @@ -20,7 +21,9 @@ type gitClient interface { ResetHard(string) error } -type gitExecuter struct{} +type gitExecuter struct { + io *iostreams.IOStreams +} func (g *gitExecuter) BranchRemote(branch string) (string, error) { args := []string{"rev-parse", "--symbolic-full-name", "--abbrev-ref", fmt.Sprintf("%s@{u}", branch)} @@ -64,11 +67,14 @@ func (g *gitExecuter) CurrentBranch() (string, error) { } func (g *gitExecuter) Fetch(remote, ref string) error { - args := []string{"fetch", remote, ref} + args := []string{"fetch", "-q", remote, ref} cmd, err := git.GitCommand(args...) if err != nil { return err } + cmd.Stdin = g.io.In + cmd.Stdout = g.io.Out + cmd.Stderr = g.io.ErrOut return cmd.Run() } diff --git a/pkg/cmd/repo/sync/sync.go b/pkg/cmd/repo/sync/sync.go index c4dc72695..4e07be890 100644 --- a/pkg/cmd/repo/sync/sync.go +++ b/pkg/cmd/repo/sync/sync.go @@ -34,7 +34,7 @@ func NewCmdSync(f *cmdutil.Factory, runF func(*SyncOptions) error) *cobra.Comman IO: f.IOStreams, BaseRepo: f.BaseRepo, Remotes: f.Remotes, - Git: &gitExecuter{}, + Git: &gitExecuter{io: f.IOStreams}, } cmd := &cobra.Command{ @@ -134,6 +134,11 @@ func syncLocalRepo(opts *SyncOptions) error { } } + // Git fetch might require input from user, so do it before starting progess indicator. + if err := opts.Git.Fetch(remote, fmt.Sprintf("refs/heads/%s", opts.Branch)); err != nil { + return err + } + opts.IO.StartProgressIndicator() err = executeLocalRepoSync(srcRepo, remote, opts) opts.IO.StopProgressIndicator() @@ -232,10 +237,6 @@ func executeLocalRepoSync(srcRepo ghrepo.Interface, remote string, opts *SyncOpt branch := opts.Branch useForce := opts.Force - if err := git.Fetch(remote, fmt.Sprintf("refs/heads/%s", branch)); err != nil { - return err - } - hasLocalBranch := git.HasLocalBranch(branch) if hasLocalBranch { branchRemote, err := git.BranchRemote(branch) From e13398f6b4d58a40d819893d7b9258124a1de7c5 Mon Sep 17 00:00:00 2001 From: Andrew Hsu Date: Tue, 14 Sep 2021 09:08:19 -0500 Subject: [PATCH 12/29] fix browse of markdown files with line ranges (#4310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mislav Marohnić --- pkg/cmd/browse/browse.go | 154 +++++++++++++++++++--------------- pkg/cmd/browse/browse_test.go | 73 +++++++--------- 2 files changed, 115 insertions(+), 112 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index fdb500cbf..71c75267f 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/utils" "github.com/spf13/cobra" ) @@ -112,86 +113,101 @@ func runBrowse(opts *BrowseOptions) error { return fmt.Errorf("unable to determine base repository: %w", err) } - httpClient, err := opts.HttpClient() + section, err := parseSection(baseRepo, opts) if err != nil { - return fmt.Errorf("unable to create an http client: %w", err) - } - url := ghrepo.GenerateRepoURL(baseRepo, "") - - if opts.SelectorArg == "" { - if opts.ProjectsFlag { - url += "/projects" - } - if opts.SettingsFlag { - url += "/settings" - } - if opts.WikiFlag { - url += "/wiki" - } - if opts.Branch != "" { - url += "/tree/" + opts.Branch + "/" - } - } else { - if isNumber(opts.SelectorArg) { - url += "/issues/" + opts.SelectorArg - } else { - fileArg, err := parseFileArg(opts.SelectorArg) - if err != nil { - return err - } - if opts.Branch != "" { - url += "/tree/" + opts.Branch + "/" - } else { - apiClient := api.NewClientFromHTTP(httpClient) - branchName, err := api.RepoDefaultBranch(apiClient, baseRepo) - if err != nil { - return err - } - url += "/tree/" + branchName + "/" - } - url += fileArg - } + return err } + url := ghrepo.GenerateRepoURL(baseRepo, section) if opts.NoBrowserFlag { - fmt.Fprintf(opts.IO.Out, "%s\n", url) - return nil - } else { - if opts.IO.IsStdoutTTY() { - fmt.Fprintf(opts.IO.Out, "now opening %s in browser\n", url) - } - return opts.Browser.Browse(url) + _, err := fmt.Fprintln(opts.IO.Out, url) + return err } + + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.Out, "Opening %s in your browser.\n", utils.DisplayURL(url)) + } + return opts.Browser.Browse(url) } -func parseFileArg(fileArg string) (string, error) { - arr := strings.Split(fileArg, ":") - if len(arr) > 2 { - return "", fmt.Errorf("invalid use of colon\nUse 'gh browse --help' for more information about browse\n") +func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error) { + if opts.SelectorArg == "" { + if opts.ProjectsFlag { + return "projects", nil + } else if opts.SettingsFlag { + return "settings", nil + } else if opts.WikiFlag { + return "wiki", nil + } else if opts.Branch == "" { + return "", nil + } } - if len(arr) > 1 { - out := arr[0] + "#L" - lineRange := strings.Split(arr[1], "-") - - if len(lineRange) > 0 { - if !isNumber(lineRange[0]) { - return "", fmt.Errorf("invalid line number after colon\nUse 'gh browse --help' for more information about browse\n") - } - out += lineRange[0] - } - - if len(lineRange) > 1 { - if !isNumber(lineRange[1]) { - return "", fmt.Errorf("invalid line range after colon\nUse 'gh browse --help' for more information about browse\n") - } - out += "-L" + lineRange[1] - } - - return out, nil + if isNumber(opts.SelectorArg) { + return fmt.Sprintf("issues/%s", opts.SelectorArg), nil } - return arr[0], nil + filePath, rangeStart, rangeEnd, err := parseFile(opts.SelectorArg) + if err != nil { + return "", err + } + + branchName := opts.Branch + if branchName == "" { + httpClient, err := opts.HttpClient() + if err != nil { + return "", err + } + apiClient := api.NewClientFromHTTP(httpClient) + branchName, err = api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return "", fmt.Errorf("error determining the default branch: %w", err) + } + } + + if rangeStart > 0 { + var rangeFragment string + if rangeEnd > 0 && rangeStart != rangeEnd { + rangeFragment = fmt.Sprintf("L%d-L%d", rangeStart, rangeEnd) + } else { + rangeFragment = fmt.Sprintf("L%d", rangeStart) + } + return fmt.Sprintf("blob/%s/%s?plain=1#%s", branchName, filePath, rangeFragment), nil + } + return fmt.Sprintf("tree/%s/%s", branchName, filePath), nil +} + +func parseFile(f string) (p string, start int, end int, err error) { + parts := strings.SplitN(f, ":", 3) + if len(parts) > 2 { + err = fmt.Errorf("invalid file argument: %q", f) + return + } + + p = parts[0] + if len(parts) < 2 { + return + } + + if idx := strings.IndexRune(parts[1], '-'); idx >= 0 { + start, err = strconv.Atoi(parts[1][:idx]) + if err != nil { + err = fmt.Errorf("invalid file argument: %q", f) + return + } + end, err = strconv.Atoi(parts[1][idx+1:]) + if err != nil { + err = fmt.Errorf("invalid file argument: %q", f) + } + return + } + + start, err = strconv.Atoi(parts[1]) + if err != nil { + err = fmt.Errorf("invalid file argument: %q", f) + } + end = start + return } func isNumber(arg string) bool { diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index bf3349c3c..0cb4df786 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -213,7 +213,7 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("ravocean", "angur"), defaultBranch: "trunk", - expectedURL: "https://github.com/ravocean/angur/tree/trunk/path/to/file.txt#L32", + expectedURL: "https://github.com/ravocean/angur/blob/trunk/path/to/file.txt?plain=1#L32", }, { name: "file with line range", @@ -222,12 +222,37 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("ravocean", "angur"), defaultBranch: "trunk", - expectedURL: "https://github.com/ravocean/angur/tree/trunk/path/to/file.txt#L32-L40", + expectedURL: "https://github.com/ravocean/angur/blob/trunk/path/to/file.txt?plain=1#L32-L40", + }, + { + name: "invalid default branch", + opts: BrowseOptions{ + SelectorArg: "chocolate-pecan-pie.txt", + }, + baseRepo: ghrepo.New("andrewhsu", "recipies"), + defaultBranch: "", + wantsErr: true, + }, + { + name: "file with invalid line number after colon", + opts: BrowseOptions{ + SelectorArg: "laptime-notes.txt:w-9", + }, + baseRepo: ghrepo.New("andrewhsu", "sonoma-raceway"), + wantsErr: true, + }, + { + name: "file with invalid file format", + opts: BrowseOptions{ + SelectorArg: "path/to/file.txt:32:32", + }, + baseRepo: ghrepo.New("ttran112", "ttrain211"), + wantsErr: true, }, { name: "file with invalid line number", opts: BrowseOptions{ - SelectorArg: "path/to/file.txt:32:32", + SelectorArg: "path/to/file.txt:32a", }, baseRepo: ghrepo.New("ttran112", "ttrain211"), wantsErr: true, @@ -258,7 +283,7 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("github", "ThankYouGitHub"), wantsErr: false, - expectedURL: "https://github.com/github/ThankYouGitHub/tree/first-browse-pull/browse.go#L32", + expectedURL: "https://github.com/github/ThankYouGitHub/blob/first-browse-pull/browse.go?plain=1#L32", }, { name: "no browser with branch file and line number", @@ -269,7 +294,7 @@ func Test_runBrowse(t *testing.T) { }, baseRepo: ghrepo.New("mislav", "will_paginate"), wantsErr: false, - expectedURL: "https://github.com/mislav/will_paginate/tree/3-0-stable/init.rb#L6", + expectedURL: "https://github.com/mislav/will_paginate/blob/3-0-stable/init.rb?plain=1#L6", }, } @@ -313,41 +338,3 @@ func Test_runBrowse(t *testing.T) { }) } } - -func Test_parseFileArg(t *testing.T) { - tests := []struct { - name string - arg string - errorExpected bool - expectedFileArg string - stderrExpected string - }{ - { - name: "non line number", - arg: "main.go", - errorExpected: false, - expectedFileArg: "main.go", - }, - { - name: "line number", - arg: "main.go:32", - errorExpected: false, - expectedFileArg: "main.go#L32", - }, - { - name: "non line number error", - arg: "ma:in.go", - errorExpected: true, - stderrExpected: "invalid line number after colon\nUse 'gh browse --help' for more information about browse\n", - }, - } - for _, tt := range tests { - fileArg, err := parseFileArg(tt.arg) - if tt.errorExpected { - assert.Equal(t, err.Error(), tt.stderrExpected) - } else { - assert.Equal(t, err, nil) - assert.Equal(t, tt.expectedFileArg, fileArg) - } - } -} From eeca99864087b41fbff6eceed7b6878545722b37 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 8 Sep 2021 17:27:06 -0500 Subject: [PATCH 13/29] binary extension support in gh extension install --- pkg/cmd/extension/command.go | 48 ++++++++++++- pkg/cmd/extension/command_test.go | 53 +++++++++++++- pkg/cmd/extension/http.go | 113 ++++++++++++++++++++++++++++++ pkg/cmd/extension/manager.go | 82 +++++++++++++++++++++- pkg/cmd/extension/manager_test.go | 59 +++++++++++++++- pkg/extensions/extension.go | 6 +- pkg/extensions/manager_mock.go | 93 ++++++++++++++++++------ 7 files changed, 425 insertions(+), 29 deletions(-) create mode 100644 pkg/cmd/extension/http.go diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 3d5215829..f83637681 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -3,10 +3,13 @@ package extension import ( "errors" "fmt" + "net/http" "os" "strings" + "time" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -102,15 +105,38 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { return err } if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { + // TODO i feel like this should check for a gh-foo script return err } + client, err := f.HttpClient() + if err != nil { + return fmt.Errorf("could not make http client: %w", err) + } + client = api.NewCachedClient(client, time.Second*30) + + isBin, err := isBinExtension(client, repo) + if err != nil { + return fmt.Errorf("could not check for binary extension: %w", err) + } + if isBin { + return m.InstallBin(client, repo) + } + + hs, err := hasScript(client, repo) + if err != nil { + return err + } + if !hs { + return errors.New("extension is uninstallable: missing executable") + } + cfg, err := f.Config() if err != nil { return err } protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.Install(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) }, }, func() *cobra.Command { @@ -220,6 +246,26 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, return nil } +func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { + hs, err := hasScript(client, repo) + if err != nil || hs { + return + } + + _, err = fetchLatestRelease(client, repo) + if err != nil { + httpErr, ok := err.(api.HTTPError) + if ok && httpErr.StatusCode == 404 { + err = nil + return + } + return + } + + isBin = true + return +} + func normalizeExtensionSelector(n string) string { if idx := strings.IndexRune(n, '/'); idx >= 0 { n = n[idx+1:] diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 9dc46b3ba..635fcba17 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -3,14 +3,17 @@ package extension import ( "io" "io/ioutil" + "net/http" "os" "strings" "testing" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/extensions" + "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -25,6 +28,7 @@ func TestNewCmdExtension(t *testing.T) { tests := []struct { name string args []string + httpStubs func(*httpmock.Registry) managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T) isTTY bool wantErr bool @@ -33,17 +37,22 @@ func TestNewCmdExtension(t *testing.T) { wantStderr string }{ { - name: "install an extension", + name: "install a git extension", args: []string{"install", "owner/gh-some-ext"}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), + httpmock.StringResponse("a script")) + }, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { em.ListFunc = func(bool) []extensions.Extension { return []extensions.Extension{} } - em.InstallFunc = func(s string, out, errOut io.Writer) error { + em.InstallGitFunc = func(s string, out, errOut io.Writer) error { return nil } return func(t *testing.T) { - installCalls := em.InstallCalls() + installCalls := em.InstallGitCalls() assert.Equal(t, 1, len(installCalls)) assert.Equal(t, "https://github.com/owner/gh-some-ext.git", installCalls[0].URL) listCalls := em.ListCalls() @@ -51,6 +60,33 @@ func TestNewCmdExtension(t *testing.T) { } }, }, + { + name: "install a binary extension", + args: []string{"install", "owner/gh-bin-ext"}, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/gh-bin-ext/contents/gh-bin-ext"), + httpmock.StatusStringResponse(404, "no")) + reg.Register( + httpmock.REST("GET", "repos/owner/gh-bin-ext/releases/latest"), + httpmock.StringResponse("{}")) + }, + managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.ListFunc = func(bool) []extensions.Extension { + return []extensions.Extension{} + } + em.InstallBinFunc = func(_ *http.Client, _ ghrepo.Interface) error { + return nil + } + return func(t *testing.T) { + installCalls := em.InstallBinCalls() + assert.Equal(t, 1, len(installCalls)) + assert.Equal(t, "gh-bin-ext", installCalls[0].Repo.RepoName()) + listCalls := em.ListCalls() + assert.Equal(t, 1, len(listCalls)) + } + }, + }, { name: "install an extension with same name as existing extension", args: []string{"install", "owner/gh-existing-ext"}, @@ -281,12 +317,23 @@ func TestNewCmdExtension(t *testing.T) { assertFunc = tt.managerStubs(em) } + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + if tt.httpStubs != nil { + tt.httpStubs(®) + } + f := cmdutil.Factory{ Config: func() (config.Config, error) { return config.NewBlankConfig(), nil }, IOStreams: ios, ExtensionManager: em, + HttpClient: func() (*http.Client, error) { + return &client, nil + }, } cmd := NewCmdExtension(&f) diff --git a/pkg/cmd/extension/http.go b/pkg/cmd/extension/http.go new file mode 100644 index 000000000..de6f61e4b --- /dev/null +++ b/pkg/cmd/extension/http.go @@ -0,0 +1,113 @@ +package extension + +import ( + "encoding/json" + "fmt" + "io" + "io/ioutil" + "net/http" + "os" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/internal/ghrepo" +) + +func hasScript(httpClient *http.Client, repo ghrepo.Interface) (hs bool, err error) { + path := fmt.Sprintf("repos/%s/%s/contents/%s", + repo.RepoOwner(), repo.RepoName(), repo.RepoName()) + url := ghinstance.RESTPrefix(repo.RepoHost()) + path + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return + } + + resp, err := httpClient.Do(req) + if err != nil { + return + } + defer resp.Body.Close() + + if resp.StatusCode == 404 { + return + } + + if resp.StatusCode > 299 { + err = api.HandleHTTPError(resp) + return + } + + hs = true + return +} + +type releaseAsset struct { + Name string + APIURL string `json:"url"` +} + +type release struct { + Assets []releaseAsset +} + +// downloadAsset downloads a single asset to the given file path. +func downloadAsset(httpClient *http.Client, asset releaseAsset, destPath string) error { + req, err := http.NewRequest("GET", asset.APIURL, nil) + if err != nil { + return err + } + + req.Header.Set("Accept", "application/octet-stream") + + resp, err := httpClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return api.HandleHTTPError(resp) + } + + f, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0755) + if err != nil { + return err + } + defer f.Close() + + _, err = io.Copy(f, resp.Body) + return err +} + +// 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 := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + var r release + err = json.Unmarshal(b, &r) + if err != nil { + return nil, err + } + + return &r, nil +} diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index b488704e5..8ffbc422e 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "io/ioutil" + "net/http" "os" "os/exec" "path" @@ -15,9 +16,11 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/findsh" "github.com/cli/safeexec" + "gopkg.in/yaml.v3" ) type Manager struct { @@ -25,6 +28,7 @@ type Manager struct { lookPath func(string) (string, error) findSh func() (string, error) newCommand func(string, ...string) *exec.Cmd + platform func() string } func NewManager() *Manager { @@ -33,6 +37,9 @@ func NewManager() *Manager { lookPath: safeexec.LookPath, findSh: findsh.Find, newCommand: exec.Command, + platform: func() string { + return fmt.Sprintf("%s-%s", runtime.GOOS, runtime.GOARCH) + }, } } @@ -172,7 +179,80 @@ func (m *Manager) InstallLocal(dir string) error { return makeSymlink(dir, targetLink) } -func (m *Manager) Install(cloneURL string, stdout, stderr io.Writer) error { +type BinManifest struct { + Owner string + Name string + Host string + // TODO I may end up not using this; just thinking ahead to local installs + Path string +} + +func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { + var r *release + r, err := fetchLatestRelease(client, repo) + if err != nil { + return err + } + + suffix := m.platform() + var asset *releaseAsset + for _, a := range r.Assets { + if strings.HasSuffix(a.Name, suffix) { + asset = &a + break + } + } + + if asset == nil { + return fmt.Errorf("%s unsupported for %s. Open an issue: `gh issue create -R%s/%s -t'Support %s'`", + repo.RepoName(), + suffix, repo.RepoOwner(), repo.RepoName(), suffix) + } + + name := repo.RepoName() + targetDir := filepath.Join(m.installDir(), name) + // TODO clean this up if function errs? + err = os.MkdirAll(targetDir, 0755) + if err != nil { + return fmt.Errorf("failed to create installation directory: %w", err) + } + + binPath := filepath.Join(targetDir, name) + + err = downloadAsset(client, *asset, binPath) + if err != nil { + return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) + } + + manifest := BinManifest{ + Name: name, + Owner: repo.RepoOwner(), + Host: repo.RepoHost(), + Path: binPath, + } + + bs, err := yaml.Marshal(manifest) + if err != nil { + return fmt.Errorf("failed to serialize manifest: %w", err) + } + + manifestPath := filepath.Join(targetDir, "manifest.yml") + + f, err := os.OpenFile(manifestPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) + if err != nil { + return fmt.Errorf("failed to open manifest for writing: %w", err) + } + defer f.Close() + + _, err = f.Write(bs) + if err != nil { + return fmt.Errorf("failed write manifest file: %w", err) + } + + return nil +} + +func (m *Manager) InstallGit(cloneURL string, stdout, stderr io.Writer) error { exe, err := m.lookPath("git") if err != nil { return err diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 2fd458bf1..c03d3fa62 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "io/ioutil" + "net/http" "os" "os/exec" "path/filepath" @@ -11,7 +12,10 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v3" ) func TestHelperProcess(t *testing.T) { @@ -39,6 +43,9 @@ func newTestManager(dir string) *Manager { cmd.Env = []string{"GH_WANT_HELPER_PROCESS=1"} return cmd }, + platform: func() string { + return "amiga-arm64" + }, } } @@ -190,18 +197,66 @@ func TestManager_Upgrade_NoExtensions(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_Install(t *testing.T) { +func TestManager_InstallGit(t *testing.T) { tempDir := t.TempDir() m := newTestManager(tempDir) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} - err := m.Install("https://github.com/owner/gh-some-ext.git", stdout, stderr) + err := m.InstallGit("https://github.com/owner/gh-some-ext.git", stdout, stderr) assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("[git clone https://github.com/owner/gh-some-ext.git %s]\n", filepath.Join(tempDir, "extensions", "gh-some-ext")), stdout.String()) assert.Equal(t, "", stderr.String()) } +func TestManager_InstallBin(t *testing.T) { + repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") + + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-amiga-arm64", + APIURL: "https://example.com/release/cool", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "release/cool"), + httpmock.StringResponse("FAKE BINARY")) + + tempDir := t.TempDir() + m := newTestManager(tempDir) + + err := m.InstallBin(&client, repo) + assert.NoError(t, err) + + manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) + assert.NoError(t, err) + + var bm BinManifest + err = yaml.Unmarshal(manifest, &bm) + assert.NoError(t, err) + + assert.Equal(t, BinManifest{ + Name: "gh-bin-ext", + Owner: "owner", + Host: "example.com", + Path: filepath.Join(tempDir, "extensions/gh-bin-ext/gh-bin-ext"), + }, bm) + + fakeBin, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/gh-bin-ext")) + assert.NoError(t, err) + + assert.Equal(t, "FAKE BINARY", string(fakeBin)) +} + func TestManager_Create(t *testing.T) { tempDir := t.TempDir() oldWd, _ := os.Getwd() diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 54379ec7f..6102c1e9a 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -2,6 +2,9 @@ package extensions import ( "io" + "net/http" + + "github.com/cli/cli/v2/internal/ghrepo" ) //go:generate moq -rm -out extension_mock.go . Extension @@ -16,7 +19,8 @@ type Extension interface { //go:generate moq -rm -out manager_mock.go . ExtensionManager type ExtensionManager interface { List(includeMetadata bool) []Extension - Install(url string, stdout, stderr io.Writer) error + InstallGit(url string, stdout, stderr io.Writer) error + InstallBin(client *http.Client, repo ghrepo.Interface) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index 157262698..97596e70d 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -4,7 +4,9 @@ package extensions import ( + "github.com/cli/cli/v2/internal/ghrepo" "io" + "net/http" "sync" ) @@ -24,8 +26,11 @@ var _ ExtensionManager = &ExtensionManagerMock{} // DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // panic("mock out the Dispatch method") // }, -// InstallFunc: func(url string, stdout io.Writer, stderr io.Writer) error { -// panic("mock out the Install method") +// InstallBinFunc: func(client *http.Client, repo ghrepo.Interface) error { +// panic("mock out the InstallBin method") +// }, +// InstallGitFunc: func(url string, stdout io.Writer, stderr io.Writer) error { +// panic("mock out the InstallGit method") // }, // InstallLocalFunc: func(dir string) error { // panic("mock out the InstallLocal method") @@ -52,8 +57,11 @@ type ExtensionManagerMock struct { // DispatchFunc mocks the Dispatch method. DispatchFunc func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) - // InstallFunc mocks the Install method. - InstallFunc func(url string, stdout io.Writer, stderr io.Writer) error + // InstallBinFunc mocks the InstallBin method. + InstallBinFunc func(client *http.Client, repo ghrepo.Interface) error + + // InstallGitFunc mocks the InstallGit method. + InstallGitFunc func(url string, stdout io.Writer, stderr io.Writer) error // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -85,8 +93,15 @@ type ExtensionManagerMock struct { // Stderr is the stderr argument value. Stderr io.Writer } - // Install holds details about calls to the Install method. - Install []struct { + // InstallBin holds details about calls to the InstallBin method. + InstallBin []struct { + // Client is the client argument value. + Client *http.Client + // Repo is the repo argument value. + Repo ghrepo.Interface + } + // InstallGit holds details about calls to the InstallGit method. + InstallGit []struct { // URL is the url argument value. URL string // Stdout is the stdout argument value. @@ -123,7 +138,8 @@ type ExtensionManagerMock struct { } lockCreate sync.RWMutex lockDispatch sync.RWMutex - lockInstall sync.RWMutex + lockInstallBin sync.RWMutex + lockInstallGit sync.RWMutex lockInstallLocal sync.RWMutex lockList sync.RWMutex lockRemove sync.RWMutex @@ -204,10 +220,45 @@ func (mock *ExtensionManagerMock) DispatchCalls() []struct { return calls } -// Install calls InstallFunc. -func (mock *ExtensionManagerMock) Install(url string, stdout io.Writer, stderr io.Writer) error { - if mock.InstallFunc == nil { - panic("ExtensionManagerMock.InstallFunc: method is nil but ExtensionManager.Install was just called") +// InstallBin calls InstallBinFunc. +func (mock *ExtensionManagerMock) InstallBin(client *http.Client, repo ghrepo.Interface) error { + if mock.InstallBinFunc == nil { + panic("ExtensionManagerMock.InstallBinFunc: method is nil but ExtensionManager.InstallBin was just called") + } + callInfo := struct { + Client *http.Client + Repo ghrepo.Interface + }{ + Client: client, + Repo: repo, + } + mock.lockInstallBin.Lock() + mock.calls.InstallBin = append(mock.calls.InstallBin, callInfo) + mock.lockInstallBin.Unlock() + return mock.InstallBinFunc(client, repo) +} + +// InstallBinCalls gets all the calls that were made to InstallBin. +// Check the length with: +// len(mockedExtensionManager.InstallBinCalls()) +func (mock *ExtensionManagerMock) InstallBinCalls() []struct { + Client *http.Client + Repo ghrepo.Interface +} { + var calls []struct { + Client *http.Client + Repo ghrepo.Interface + } + mock.lockInstallBin.RLock() + calls = mock.calls.InstallBin + mock.lockInstallBin.RUnlock() + return calls +} + +// InstallGit calls InstallGitFunc. +func (mock *ExtensionManagerMock) InstallGit(url string, stdout io.Writer, stderr io.Writer) error { + if mock.InstallGitFunc == nil { + panic("ExtensionManagerMock.InstallGitFunc: method is nil but ExtensionManager.InstallGit was just called") } callInfo := struct { URL string @@ -218,16 +269,16 @@ func (mock *ExtensionManagerMock) Install(url string, stdout io.Writer, stderr i Stdout: stdout, Stderr: stderr, } - mock.lockInstall.Lock() - mock.calls.Install = append(mock.calls.Install, callInfo) - mock.lockInstall.Unlock() - return mock.InstallFunc(url, stdout, stderr) + mock.lockInstallGit.Lock() + mock.calls.InstallGit = append(mock.calls.InstallGit, callInfo) + mock.lockInstallGit.Unlock() + return mock.InstallGitFunc(url, stdout, stderr) } -// InstallCalls gets all the calls that were made to Install. +// InstallGitCalls gets all the calls that were made to InstallGit. // Check the length with: -// len(mockedExtensionManager.InstallCalls()) -func (mock *ExtensionManagerMock) InstallCalls() []struct { +// len(mockedExtensionManager.InstallGitCalls()) +func (mock *ExtensionManagerMock) InstallGitCalls() []struct { URL string Stdout io.Writer Stderr io.Writer @@ -237,9 +288,9 @@ func (mock *ExtensionManagerMock) InstallCalls() []struct { Stdout io.Writer Stderr io.Writer } - mock.lockInstall.RLock() - calls = mock.calls.Install - mock.lockInstall.RUnlock() + mock.lockInstallGit.RLock() + calls = mock.calls.InstallGit + mock.lockInstallGit.RUnlock() return calls } From f9e49f4aecf383d235b0c81e78a311dd3cb3d88a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 16 Sep 2021 12:25:44 +0200 Subject: [PATCH 14/29] Do not swallow prompt error during `repo create` --- pkg/cmd/repo/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index a99e471cc..1f74a8188 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -251,7 +251,7 @@ func createRun(opts *CreateOptions) error { if !isVisibilityPassed { newVisibility, err := getVisibility() if err != nil { - return nil + return err } visibility = newVisibility } From 0305788536307b65250f3bc98581eab7d06b6a8e Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 16 Sep 2021 09:02:10 -0700 Subject: [PATCH 15/29] Typo --- pkg/cmd/repo/sync/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/sync/sync.go b/pkg/cmd/repo/sync/sync.go index 4e07be890..8a71161f5 100644 --- a/pkg/cmd/repo/sync/sync.go +++ b/pkg/cmd/repo/sync/sync.go @@ -134,7 +134,7 @@ func syncLocalRepo(opts *SyncOptions) error { } } - // Git fetch might require input from user, so do it before starting progess indicator. + // Git fetch might require input from user, so do it before starting progress indicator. if err := opts.Git.Fetch(remote, fmt.Sprintf("refs/heads/%s", opts.Branch)); err != nil { return err } From 6a34f53c6c50046cfbe47eb681208fc89f83e3e5 Mon Sep 17 00:00:00 2001 From: Siarhei Fedartsou Date: Fri, 17 Sep 2021 22:05:47 +0300 Subject: [PATCH 16/29] Change pr list --draft UX --- pkg/cmd/pr/list/http.go | 8 +++----- pkg/cmd/pr/list/list.go | 13 ++++++------- pkg/cmd/pr/list/list_test.go | 20 ++++++-------------- pkg/cmd/pr/shared/params.go | 11 ++++------- pkg/cmd/pr/shared/params_test.go | 17 +++++++++-------- 5 files changed, 28 insertions(+), 41 deletions(-) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 04ced2cca..8f77b8023 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -11,7 +11,7 @@ import ( ) func shouldUseSearch(filters prShared.FilterOptions) bool { - return filters.Draft || filters.NonDraft || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 + return filters.Draft != nil || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 } func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { @@ -181,10 +181,8 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters q.SetBaseBranch(filters.BaseBranch) } - if filters.Draft { - q.SetDraft(true) - } else if filters.NonDraft { - q.SetDraft(false) + if filters.Draft != nil { + q.SetDraft(*filters.Draft) } pageLimit := min(limit, 100) diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 4ef060bb7..2e9056e88 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -37,15 +37,16 @@ type ListOptions struct { Author string Assignee string Search string - Draft bool - NonDraft bool + Draft *bool } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { + draft := false opts := &ListOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Browser: f.Browser, + Draft: &draft, } cmd := &cobra.Command{ @@ -76,8 +77,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} } - if opts.Draft && opts.NonDraft { - return &cmdutil.FlagError{Err: fmt.Errorf("specify only one of `--draft` or `--non-draft`")} + if !cmd.Flags().Changed("draft") { + opts.Draft = nil } if runF != nil { @@ -98,8 +99,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Author, "author", "A", "", "Filter by author") cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search pull requests with `query`") - cmd.Flags().BoolVar(&opts.Draft, "draft", false, "Show drafts only") - cmd.Flags().BoolVar(&opts.NonDraft, "non-draft", false, "Show non-drafts only") + cmd.Flags().BoolVar(opts.Draft, "draft", false, "Filter by draft/non-draft") cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.PullRequestFields) @@ -142,7 +142,6 @@ func listRun(opts *ListOptions) error { BaseBranch: opts.BaseBranch, Search: opts.Search, Draft: opts.Draft, - NonDraft: opts.NonDraft, Fields: defaultFields, } if opts.Exporter != nil { diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 700a03f08..9043d9274 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -176,15 +176,6 @@ func TestPRList_filteringAssignee(t *testing.T) { } } -func TestPRList_bothNonDraftAndDraft(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - - _, err := runCommand(http, true, `--draft --non-draft`) - assert.NotNil(t, err) - assert.Equal(t, err.Error(), "specify only one of `--draft` or `--non-draft`") -} - func TestPRList_filteringDraft(t *testing.T) { tests := []struct { name string @@ -193,12 +184,12 @@ func TestPRList_filteringDraft(t *testing.T) { }{ { "draft", - "--draft", + "--draft=1", `repo:OWNER/REPO is:pr is:open draft:true`, }, { "non-draft", - "--non-draft", + "--draft=false", `repo:OWNER/REPO is:pr is:open draft:false`, }} @@ -244,14 +235,15 @@ func TestPRList_web(t *testing.T) { }, { "draft", - "--draft", + "--draft=true", "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", }, { "non-draft", - "--non-draft", + "--draft=0", "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", - }} + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 5e5ec4ef8..846a05022 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -157,8 +157,8 @@ type FilterOptions struct { Mention string Milestone string Search string - Draft bool - NonDraft bool + // filter by draft/non-draft, `nil` means "no filter" + Draft *bool Fields []string } @@ -243,12 +243,9 @@ func SearchQueryBuild(options FilterOptions) string { if options.Search != "" { q.AddQuery(options.Search) } - if options.Draft { - q.SetDraft(true) - } else if options.NonDraft { - q.SetDraft(false) + if options.Draft != nil { + q.SetDraft(*options.Draft) } - return q.String() } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 696950722..a40916077 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -16,6 +16,9 @@ func Test_listURLWithQuery(t *testing.T) { listURL string options FilterOptions } + draft := true + noDraft := false + tests := []struct { name string args args @@ -39,10 +42,9 @@ func Test_listURLWithQuery(t *testing.T) { args: args{ listURL: "https://example.com/path", options: FilterOptions{ - Entity: "pr", - State: "open", - Draft: true, - NonDraft: true, // shouldn't impact anything - we always prefer `draft` + Entity: "pr", + State: "open", + Draft: &draft, }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Atrue", @@ -53,10 +55,9 @@ func Test_listURLWithQuery(t *testing.T) { args: args{ listURL: "https://example.com/path", options: FilterOptions{ - Entity: "pr", - State: "open", - Draft: false, - NonDraft: true, + Entity: "pr", + State: "open", + Draft: &noDraft, }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Afalse", From 9f43967042bbf5e57a63db1e34124ba4d647c995 Mon Sep 17 00:00:00 2001 From: Dan Burzo Date: Mon, 20 Sep 2021 19:35:06 +0300 Subject: [PATCH 17/29] Fixes #4346: allow git+https URL protocol --- git/url.go | 5 +++++ git/url_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/git/url.go b/git/url.go index 7d4aac4d7..1a3e97fd6 100644 --- a/git/url.go +++ b/git/url.go @@ -14,6 +14,7 @@ func isSupportedProtocol(u string) bool { strings.HasPrefix(u, "git+ssh:") || strings.HasPrefix(u, "git:") || strings.HasPrefix(u, "http:") || + strings.HasPrefix(u, "git+https:") || strings.HasPrefix(u, "https:") } @@ -43,6 +44,10 @@ func ParseURL(rawURL string) (u *url.URL, err error) { u.Scheme = "ssh" } + if u.Scheme == "git+https" { + u.Scheme = "https" + } + if u.Scheme != "ssh" { return } diff --git a/git/url_test.go b/git/url_test.go index 679739b41..f5b3b50d0 100644 --- a/git/url_test.go +++ b/git/url_test.go @@ -28,11 +28,26 @@ func TestIsURL(t *testing.T) { url: "git://example.com/owner/repo", want: true, }, + { + name: "git with extension", + url: "git://example.com/owner/repo.git", + want: true, + }, + { + name: "git+ssh", + url: "git+ssh://git@example.com/owner/repo.git", + want: true, + }, { name: "https", url: "https://example.com/owner/repo.git", want: true, }, + { + name: "git+https", + url: "git+https://example.com/owner/repo.git", + want: true, + }, { name: "no protocol", url: "example.com/owner/repo", @@ -121,6 +136,16 @@ func TestParseURL(t *testing.T) { Path: "/owner/repo.git", }, }, + { + name: "git+https", + url: "git+https://example.com/owner/repo.git", + want: url{ + Scheme: "https", + User: "", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, { name: "scp-like", url: "git@example.com:owner/repo.git", From d14715f1e37e7db19408dea8cc3162f8b2c3fb9f Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 20 Sep 2021 11:29:37 -0700 Subject: [PATCH 18/29] Convert bool to string early for pr list draft flag --- pkg/cmd/pr/list/http.go | 6 ++--- pkg/cmd/pr/list/list.go | 12 +++++----- pkg/cmd/pr/list/list_test.go | 39 +++++++++++++++----------------- pkg/cmd/pr/shared/params.go | 10 ++++---- pkg/cmd/pr/shared/params_test.go | 6 ++--- pkg/githubsearch/query.go | 10 ++++---- 6 files changed, 38 insertions(+), 45 deletions(-) diff --git a/pkg/cmd/pr/list/http.go b/pkg/cmd/pr/list/http.go index 8f77b8023..43e87b7fd 100644 --- a/pkg/cmd/pr/list/http.go +++ b/pkg/cmd/pr/list/http.go @@ -11,7 +11,7 @@ import ( ) func shouldUseSearch(filters prShared.FilterOptions) bool { - return filters.Draft != nil || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 + return filters.Draft != "" || filters.Author != "" || filters.Assignee != "" || filters.Search != "" || len(filters.Labels) > 0 } func listPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters prShared.FilterOptions, limit int) (*api.PullRequestAndTotalCount, error) { @@ -181,8 +181,8 @@ func searchPullRequests(httpClient *http.Client, repo ghrepo.Interface, filters q.SetBaseBranch(filters.BaseBranch) } - if filters.Draft != nil { - q.SetDraft(*filters.Draft) + if filters.Draft != "" { + q.SetDraft(filters.Draft) } pageLimit := min(limit, 100) diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 2e9056e88..76013fd88 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -37,18 +37,18 @@ type ListOptions struct { Author string Assignee string Search string - Draft *bool + Draft string } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { - draft := false opts := &ListOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, Browser: f.Browser, - Draft: &draft, } + var draft bool + cmd := &cobra.Command{ Use: "list", Short: "List and filter pull requests in this repository", @@ -77,8 +77,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return &cmdutil.FlagError{Err: fmt.Errorf("invalid value for --limit: %v", opts.LimitResults)} } - if !cmd.Flags().Changed("draft") { - opts.Draft = nil + if cmd.Flags().Changed("draft") { + opts.Draft = strconv.FormatBool(draft) } if runF != nil { @@ -99,7 +99,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Author, "author", "A", "", "Filter by author") cmd.Flags().StringVarP(&opts.Assignee, "assignee", "a", "", "Filter by assignee") cmd.Flags().StringVarP(&opts.Search, "search", "S", "", "Search pull requests with `query`") - cmd.Flags().BoolVar(opts.Draft, "draft", false, "Filter by draft/non-draft") + cmd.Flags().BoolVarP(&draft, "draft", "d", false, "Filter by draft state") cmdutil.AddJSONFlags(cmd, &opts.Exporter, api.PullRequestFields) diff --git a/pkg/cmd/pr/list/list_test.go b/pkg/cmd/pr/list/list_test.go index 9043d9274..ff55089f0 100644 --- a/pkg/cmd/pr/list/list_test.go +++ b/pkg/cmd/pr/list/list_test.go @@ -183,15 +183,16 @@ func TestPRList_filteringDraft(t *testing.T) { expectedQuery string }{ { - "draft", - "--draft=1", - `repo:OWNER/REPO is:pr is:open draft:true`, + name: "draft", + cli: "--draft", + expectedQuery: `repo:OWNER/REPO is:pr is:open draft:true`, }, { - "non-draft", - "--draft=false", - `repo:OWNER/REPO is:pr is:open draft:false`, - }} + name: "non-draft", + cli: "--draft=false", + expectedQuery: `repo:OWNER/REPO is:pr is:open draft:false`, + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -210,16 +211,13 @@ func TestPRList_filteringDraft(t *testing.T) { } }) } - } func TestPRList_withInvalidLimitFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - _, err := runCommand(http, true, `--limit=0`) - assert.NotNil(t, err) - assert.Equal(t, err.Error(), "invalid value for --limit: 0") + assert.EqualError(t, err, "invalid value for --limit: 0") } func TestPRList_web(t *testing.T) { @@ -229,19 +227,19 @@ func TestPRList_web(t *testing.T) { expectedBrowserURL string }{ { - "test", - "-a peter -l bug -l docs -L 10 -s merged -B trunk", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", + name: "filters", + cli: "-a peter -l bug -l docs -L 10 -s merged -B trunk", + expectedBrowserURL: "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Amerged+assignee%3Apeter+label%3Abug+label%3Adocs+base%3Atrunk", }, { - "draft", - "--draft=true", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", + name: "draft", + cli: "--draft=true", + expectedBrowserURL: "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Atrue", }, { - "non-draft", - "--draft=0", - "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", + name: "non-draft", + cli: "--draft=0", + expectedBrowserURL: "https://github.com/OWNER/REPO/pulls?q=is%3Apr+is%3Aopen+draft%3Afalse", }, } @@ -263,5 +261,4 @@ func TestPRList_web(t *testing.T) { assert.Equal(t, test.expectedBrowserURL, output.BrowsedURL) }) } - } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 846a05022..2333bcfa3 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -157,10 +157,8 @@ type FilterOptions struct { Mention string Milestone string Search string - // filter by draft/non-draft, `nil` means "no filter" - Draft *bool - - Fields []string + Draft string + Fields []string } func (opts *FilterOptions) IsDefault() bool { @@ -243,8 +241,8 @@ func SearchQueryBuild(options FilterOptions) string { if options.Search != "" { q.AddQuery(options.Search) } - if options.Draft != nil { - q.SetDraft(*options.Draft) + if options.Draft != "" { + q.SetDraft(options.Draft) } return q.String() } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index a40916077..8f3e793e5 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -16,8 +16,6 @@ func Test_listURLWithQuery(t *testing.T) { listURL string options FilterOptions } - draft := true - noDraft := false tests := []struct { name string @@ -44,7 +42,7 @@ func Test_listURLWithQuery(t *testing.T) { options: FilterOptions{ Entity: "pr", State: "open", - Draft: &draft, + Draft: "true", }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Atrue", @@ -57,7 +55,7 @@ func Test_listURLWithQuery(t *testing.T) { options: FilterOptions{ Entity: "pr", State: "open", - Draft: &noDraft, + Draft: "false", }, }, want: "https://example.com/path?q=is%3Apr+is%3Aopen+draft%3Afalse", diff --git a/pkg/githubsearch/query.go b/pkg/githubsearch/query.go index b0938613a..a8f3005a9 100644 --- a/pkg/githubsearch/query.go +++ b/pkg/githubsearch/query.go @@ -54,7 +54,7 @@ type Query struct { forkState string visibility string isArchived *bool - isDraft *bool + draft string } func (q *Query) InRepository(nameWithOwner string) { @@ -140,8 +140,8 @@ func (q *Query) SetArchived(isArchived bool) { q.isArchived = &isArchived } -func (q *Query) SetDraft(isDraft bool) { - q.isDraft = &isDraft +func (q *Query) SetDraft(draft string) { + q.draft = draft } func (q *Query) String() string { @@ -203,8 +203,8 @@ func (q *Query) String() string { if q.headBranch != "" { qs += fmt.Sprintf("head:%s ", quote(q.headBranch)) } - if q.isDraft != nil { - qs += fmt.Sprintf("draft:%v ", *q.isDraft) + if q.draft != "" { + qs += fmt.Sprintf("draft:%v ", q.draft) } if q.sort != "" { From b00e8a5681d8934a9408e99c3e8ced693faefe84 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:02:20 -0500 Subject: [PATCH 19/29] more accurately check for binary extension --- pkg/cmd/extension/command.go | 68 +++++++++++++++++++++++++++---- pkg/cmd/extension/command_test.go | 10 +++-- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index f83637681..8153ef5e4 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -247,12 +247,8 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, } func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { - hs, err := hasScript(client, repo) - if err != nil || hs { - return - } - - _, err = fetchLatestRelease(client, repo) + var r *release + r, err = fetchLatestRelease(client, repo) if err != nil { httpErr, ok := err.(api.HTTPError) if ok && httpErr.StatusCode == 404 { @@ -262,7 +258,16 @@ func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err return } - isBin = true + for _, a := range r.Assets { + dists := possibleDists() + for _, d := range dists { + if strings.HasSuffix(a.Name, d) { + isBin = true + break + } + } + } + return } @@ -272,3 +277,52 @@ func normalizeExtensionSelector(n string) string { } return strings.TrimPrefix(n, "gh-") } + +func possibleDists() []string { + return []string{ + "aix-ppc64", + "android-386", + "android-amd64", + "android-arm", + "android-arm64", + "darwin-amd64", + "darwin-arm64", + "dragonfly-amd64", + "freebsd-386", + "freebsd-amd64", + "freebsd-arm", + "freebsd-arm64", + "illumos-amd64", + "ios-amd64", + "ios-arm64", + "js-wasm", + "linux-386", + "linux-amd64", + "linux-arm", + "linux-arm64", + "linux-mips", + "linux-mips64", + "linux-mips64le", + "linux-mipsle", + "linux-ppc64", + "linux-ppc64le", + "linux-riscv64", + "linux-s390x", + "netbsd-386", + "netbsd-amd64", + "netbsd-arm", + "netbsd-arm64", + "openbsd-386", + "openbsd-amd64", + "openbsd-arm", + "openbsd-arm64", + "openbsd-mips64", + "plan9-386", + "plan9-amd64", + "plan9-arm", + "solaris-amd64", + "windows-386", + "windows-amd64", + "windows-arm", + } +} diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 635fcba17..8c54b24da 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -40,6 +40,9 @@ func TestNewCmdExtension(t *testing.T) { name: "install a git extension", args: []string{"install", "owner/gh-some-ext"}, httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), + httpmock.StatusStringResponse(404, "nope")) reg.Register( httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), httpmock.StringResponse("a script")) @@ -64,12 +67,11 @@ func TestNewCmdExtension(t *testing.T) { name: "install a binary extension", args: []string{"install", "owner/gh-bin-ext"}, httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/owner/gh-bin-ext/contents/gh-bin-ext"), - httpmock.StatusStringResponse(404, "no")) reg.Register( httpmock.REST("GET", "repos/owner/gh-bin-ext/releases/latest"), - httpmock.StringResponse("{}")) + httpmock.JSONResponse(release{ + Assets: []releaseAsset{ + {Name: "gh-foo-windows-amd64"}}})) }, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { em.ListFunc = func(bool) []extensions.Extension { From ae38daf08a451bbccb9964fcd8f5a49190cce823 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:05:35 -0500 Subject: [PATCH 20/29] nit --- pkg/cmd/extension/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 8ffbc422e..393e8ef82 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -204,7 +204,7 @@ func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { } if asset == nil { - return fmt.Errorf("%s unsupported for %s. Open an issue: `gh issue create -R%s/%s -t'Support %s'`", + return fmt.Errorf("%s unsupported for %s. Open an issue: `gh issue create -R %s/%s -t'Support %s'`", repo.RepoName(), suffix, repo.RepoOwner(), repo.RepoName(), suffix) } From f4d97dcedd4b4b4f31ffdf117e357a6fbac5ccae Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:25:26 -0500 Subject: [PATCH 21/29] WIP refactoring --- pkg/cmd/extension/command.go | 22 ++-------------------- pkg/cmd/extension/manager.go | 23 +++++++++++++++++++++++ pkg/cmd/extension/manager_test.go | 4 ++-- pkg/extensions/extension.go | 7 +++++-- 4 files changed, 32 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 8153ef5e4..9ac5e5f21 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -105,38 +105,20 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { return err } if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { - // TODO i feel like this should check for a gh-foo script return err } - client, err := f.HttpClient() if err != nil { return fmt.Errorf("could not make http client: %w", err) } client = api.NewCachedClient(client, time.Second*30) - isBin, err := isBinExtension(client, repo) - if err != nil { - return fmt.Errorf("could not check for binary extension: %w", err) - } - if isBin { - return m.InstallBin(client, repo) - } - - hs, err := hasScript(client, repo) - if err != nil { - return err - } - if !hs { - return errors.New("extension is uninstallable: missing executable") - } - cfg, err := f.Config() if err != nil { return err } - protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + + return m.Install(client, repo, io, cfg) }, }, func() *cobra.Command { diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 393e8ef82..ddfbde3fc 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -19,6 +19,7 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/extensions" "github.com/cli/cli/v2/pkg/findsh" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/safeexec" "gopkg.in/yaml.v3" ) @@ -187,6 +188,28 @@ type BinManifest struct { Path string } +func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostreams.IOStreams, cfg config.Config) error { + isBin, err := isBinExtension(client, repo) + if err != nil { + return fmt.Errorf("could not check for binary extension: %w", err) + } + if isBin { + return m.InstallBin(client, repo) + } + + hs, err := hasScript(client, repo) + if err != nil { + return err + } + if !hs { + // TODO open an issue hint, here? + return errors.New("extension is uninstallable: missing executable") + } + + protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") + return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) +} + func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { var r *release r, err := fetchLatestRelease(client, repo) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index c03d3fa62..80a535c40 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -44,7 +44,7 @@ func newTestManager(dir string) *Manager { return cmd }, platform: func() string { - return "amiga-arm64" + return "windows-amd64" }, } } @@ -222,7 +222,7 @@ func TestManager_InstallBin(t *testing.T) { release{ Assets: []releaseAsset{ { - Name: "gh-bin-ext-amiga-arm64", + Name: "gh-bin-ext-windows-amd64", APIURL: "https://example.com/release/cool", }, }, diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 6102c1e9a..99087fe04 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -4,7 +4,9 @@ import ( "io" "net/http" + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/iostreams" ) //go:generate moq -rm -out extension_mock.go . Extension @@ -19,8 +21,9 @@ type Extension interface { //go:generate moq -rm -out manager_mock.go . ExtensionManager type ExtensionManager interface { List(includeMetadata bool) []Extension - InstallGit(url string, stdout, stderr io.Writer) error - InstallBin(client *http.Client, repo ghrepo.Interface) error + Install(*http.Client, ghrepo.Interface, *iostreams.IOStreams, config.Config) error + InstallBin(*http.Client, ghrepo.Interface) error + InstallGit(string, io.Writer, io.Writer) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error From af7805af53384839e3b421d356cf7067d88a09ae Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 16:46:54 -0500 Subject: [PATCH 22/29] WIP refactoring --- pkg/cmd/extension/command_test.go | 47 +---------- pkg/extensions/manager_mock.go | 135 ++++++++++++++++++++++-------- 2 files changed, 103 insertions(+), 79 deletions(-) diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 8c54b24da..9b363fbb7 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -28,7 +28,6 @@ func TestNewCmdExtension(t *testing.T) { tests := []struct { name string args []string - httpStubs func(*httpmock.Registry) managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T) isTTY bool wantErr bool @@ -37,53 +36,19 @@ func TestNewCmdExtension(t *testing.T) { wantStderr string }{ { - name: "install a git extension", + name: "install an extension", args: []string{"install", "owner/gh-some-ext"}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), - httpmock.StatusStringResponse(404, "nope")) - reg.Register( - httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), - httpmock.StringResponse("a script")) - }, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { em.ListFunc = func(bool) []extensions.Extension { return []extensions.Extension{} } - em.InstallGitFunc = func(s string, out, errOut io.Writer) error { + em.InstallFunc = func(_ *http.Client, _ ghrepo.Interface, _ *iostreams.IOStreams, _ config.Config) error { return nil } return func(t *testing.T) { - installCalls := em.InstallGitCalls() + installCalls := em.InstallCalls() assert.Equal(t, 1, len(installCalls)) - assert.Equal(t, "https://github.com/owner/gh-some-ext.git", installCalls[0].URL) - listCalls := em.ListCalls() - assert.Equal(t, 1, len(listCalls)) - } - }, - }, - { - name: "install a binary extension", - args: []string{"install", "owner/gh-bin-ext"}, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/owner/gh-bin-ext/releases/latest"), - httpmock.JSONResponse(release{ - Assets: []releaseAsset{ - {Name: "gh-foo-windows-amd64"}}})) - }, - managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { - em.ListFunc = func(bool) []extensions.Extension { - return []extensions.Extension{} - } - em.InstallBinFunc = func(_ *http.Client, _ ghrepo.Interface) error { - return nil - } - return func(t *testing.T) { - installCalls := em.InstallBinCalls() - assert.Equal(t, 1, len(installCalls)) - assert.Equal(t, "gh-bin-ext", installCalls[0].Repo.RepoName()) + assert.Equal(t, "gh-some-ext", installCalls[0].InterfaceMoqParam.RepoName()) listCalls := em.ListCalls() assert.Equal(t, 1, len(listCalls)) } @@ -323,10 +288,6 @@ func TestNewCmdExtension(t *testing.T) { defer reg.Verify(t) client := http.Client{Transport: ®} - if tt.httpStubs != nil { - tt.httpStubs(®) - } - f := cmdutil.Factory{ Config: func() (config.Config, error) { return config.NewBlankConfig(), nil diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index 97596e70d..b8d18dc6d 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -4,7 +4,9 @@ package extensions import ( + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/iostreams" "io" "net/http" "sync" @@ -26,10 +28,13 @@ var _ ExtensionManager = &ExtensionManagerMock{} // DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // panic("mock out the Dispatch method") // }, -// InstallBinFunc: func(client *http.Client, repo ghrepo.Interface) error { +// InstallFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { +// panic("mock out the Install method") +// }, +// InstallBinFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface) error { // panic("mock out the InstallBin method") // }, -// InstallGitFunc: func(url string, stdout io.Writer, stderr io.Writer) error { +// InstallGitFunc: func(s string, writer1 io.Writer, writer2 io.Writer) error { // panic("mock out the InstallGit method") // }, // InstallLocalFunc: func(dir string) error { @@ -57,11 +62,14 @@ type ExtensionManagerMock struct { // DispatchFunc mocks the Dispatch method. DispatchFunc func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) + // InstallFunc mocks the Install method. + InstallFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error + // InstallBinFunc mocks the InstallBin method. - InstallBinFunc func(client *http.Client, repo ghrepo.Interface) error + InstallBinFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface) error // InstallGitFunc mocks the InstallGit method. - InstallGitFunc func(url string, stdout io.Writer, stderr io.Writer) error + InstallGitFunc func(s string, writer1 io.Writer, writer2 io.Writer) error // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -93,21 +101,32 @@ type ExtensionManagerMock struct { // Stderr is the stderr argument value. Stderr io.Writer } + // Install holds details about calls to the Install method. + Install []struct { + // Client is the client argument value. + Client *http.Client + // InterfaceMoqParam is the interfaceMoqParam argument value. + InterfaceMoqParam ghrepo.Interface + // IOStreams is the iOStreams argument value. + IOStreams *iostreams.IOStreams + // ConfigMoqParam is the configMoqParam argument value. + ConfigMoqParam config.Config + } // InstallBin holds details about calls to the InstallBin method. InstallBin []struct { // Client is the client argument value. Client *http.Client - // Repo is the repo argument value. - Repo ghrepo.Interface + // InterfaceMoqParam is the interfaceMoqParam argument value. + InterfaceMoqParam ghrepo.Interface } // InstallGit holds details about calls to the InstallGit method. InstallGit []struct { - // URL is the url argument value. - URL string - // Stdout is the stdout argument value. - Stdout io.Writer - // Stderr is the stderr argument value. - Stderr io.Writer + // S is the s argument value. + S string + // Writer1 is the writer1 argument value. + Writer1 io.Writer + // Writer2 is the writer2 argument value. + Writer2 io.Writer } // InstallLocal holds details about calls to the InstallLocal method. InstallLocal []struct { @@ -138,6 +157,7 @@ type ExtensionManagerMock struct { } lockCreate sync.RWMutex lockDispatch sync.RWMutex + lockInstall sync.RWMutex lockInstallBin sync.RWMutex lockInstallGit sync.RWMutex lockInstallLocal sync.RWMutex @@ -220,34 +240,77 @@ func (mock *ExtensionManagerMock) DispatchCalls() []struct { return calls } +// Install calls InstallFunc. +func (mock *ExtensionManagerMock) Install(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { + if mock.InstallFunc == nil { + panic("ExtensionManagerMock.InstallFunc: method is nil but ExtensionManager.Install was just called") + } + callInfo := struct { + Client *http.Client + InterfaceMoqParam ghrepo.Interface + IOStreams *iostreams.IOStreams + ConfigMoqParam config.Config + }{ + Client: client, + InterfaceMoqParam: interfaceMoqParam, + IOStreams: iOStreams, + ConfigMoqParam: configMoqParam, + } + mock.lockInstall.Lock() + mock.calls.Install = append(mock.calls.Install, callInfo) + mock.lockInstall.Unlock() + return mock.InstallFunc(client, interfaceMoqParam, iOStreams, configMoqParam) +} + +// InstallCalls gets all the calls that were made to Install. +// Check the length with: +// len(mockedExtensionManager.InstallCalls()) +func (mock *ExtensionManagerMock) InstallCalls() []struct { + Client *http.Client + InterfaceMoqParam ghrepo.Interface + IOStreams *iostreams.IOStreams + ConfigMoqParam config.Config +} { + var calls []struct { + Client *http.Client + InterfaceMoqParam ghrepo.Interface + IOStreams *iostreams.IOStreams + ConfigMoqParam config.Config + } + mock.lockInstall.RLock() + calls = mock.calls.Install + mock.lockInstall.RUnlock() + return calls +} + // InstallBin calls InstallBinFunc. -func (mock *ExtensionManagerMock) InstallBin(client *http.Client, repo ghrepo.Interface) error { +func (mock *ExtensionManagerMock) InstallBin(client *http.Client, interfaceMoqParam ghrepo.Interface) error { if mock.InstallBinFunc == nil { panic("ExtensionManagerMock.InstallBinFunc: method is nil but ExtensionManager.InstallBin was just called") } callInfo := struct { - Client *http.Client - Repo ghrepo.Interface + Client *http.Client + InterfaceMoqParam ghrepo.Interface }{ - Client: client, - Repo: repo, + Client: client, + InterfaceMoqParam: interfaceMoqParam, } mock.lockInstallBin.Lock() mock.calls.InstallBin = append(mock.calls.InstallBin, callInfo) mock.lockInstallBin.Unlock() - return mock.InstallBinFunc(client, repo) + return mock.InstallBinFunc(client, interfaceMoqParam) } // InstallBinCalls gets all the calls that were made to InstallBin. // Check the length with: // len(mockedExtensionManager.InstallBinCalls()) func (mock *ExtensionManagerMock) InstallBinCalls() []struct { - Client *http.Client - Repo ghrepo.Interface + Client *http.Client + InterfaceMoqParam ghrepo.Interface } { var calls []struct { - Client *http.Client - Repo ghrepo.Interface + Client *http.Client + InterfaceMoqParam ghrepo.Interface } mock.lockInstallBin.RLock() calls = mock.calls.InstallBin @@ -256,37 +319,37 @@ func (mock *ExtensionManagerMock) InstallBinCalls() []struct { } // InstallGit calls InstallGitFunc. -func (mock *ExtensionManagerMock) InstallGit(url string, stdout io.Writer, stderr io.Writer) error { +func (mock *ExtensionManagerMock) InstallGit(s string, writer1 io.Writer, writer2 io.Writer) error { if mock.InstallGitFunc == nil { panic("ExtensionManagerMock.InstallGitFunc: method is nil but ExtensionManager.InstallGit was just called") } callInfo := struct { - URL string - Stdout io.Writer - Stderr io.Writer + S string + Writer1 io.Writer + Writer2 io.Writer }{ - URL: url, - Stdout: stdout, - Stderr: stderr, + S: s, + Writer1: writer1, + Writer2: writer2, } mock.lockInstallGit.Lock() mock.calls.InstallGit = append(mock.calls.InstallGit, callInfo) mock.lockInstallGit.Unlock() - return mock.InstallGitFunc(url, stdout, stderr) + return mock.InstallGitFunc(s, writer1, writer2) } // InstallGitCalls gets all the calls that were made to InstallGit. // Check the length with: // len(mockedExtensionManager.InstallGitCalls()) func (mock *ExtensionManagerMock) InstallGitCalls() []struct { - URL string - Stdout io.Writer - Stderr io.Writer + S string + Writer1 io.Writer + Writer2 io.Writer } { var calls []struct { - URL string - Stdout io.Writer - Stderr io.Writer + S string + Writer1 io.Writer + Writer2 io.Writer } mock.lockInstallGit.RLock() calls = mock.calls.InstallGit From f5d269ebad0be6fcf42a0e76a87cc887f3ab37bd Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:02:34 -0500 Subject: [PATCH 23/29] WIP refactoring --- pkg/cmd/extension/manager.go | 1 + pkg/cmd/extension/manager_test.go | 48 +++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index ddfbde3fc..75cb4cfd9 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -185,6 +185,7 @@ type BinManifest struct { Name string Host string // TODO I may end up not using this; just thinking ahead to local installs + // TODO track version Path string } diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 80a535c40..f0a930dcd 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -12,8 +12,10 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/assert" "gopkg.in/yaml.v3" ) @@ -197,25 +199,57 @@ func TestManager_Upgrade_NoExtensions(t *testing.T) { assert.Equal(t, "", stderr.String()) } -func TestManager_InstallGit(t *testing.T) { +func TestManager_Install_git(t *testing.T) { tempDir := t.TempDir() m := newTestManager(tempDir) - stdout := &bytes.Buffer{} - stderr := &bytes.Buffer{} - err := m.InstallGit("https://github.com/owner/gh-some-ext.git", stdout, stderr) + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "not-a-binary", + APIURL: "https://example.com/release/cool", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), + httpmock.StringResponse("script")) + + io, _, stdout, stderr := iostreams.Test() + + repo := ghrepo.New("owner", "gh-some-ext") + + err := m.Install(&client, repo, io, config.NewBlankConfig()) assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("[git clone https://github.com/owner/gh-some-ext.git %s]\n", filepath.Join(tempDir, "extensions", "gh-some-ext")), stdout.String()) assert.Equal(t, "", stderr.String()) } -func TestManager_InstallBin(t *testing.T) { +func TestManager_Install_binary(t *testing.T) { repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") reg := httpmock.Registry{} defer reg.Verify(t) client := http.Client{Transport: ®} + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-windows-amd64", + APIURL: "https://example.com/release/cool", + }, + }, + })) reg.Register( httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), httpmock.JSONResponse( @@ -234,7 +268,9 @@ func TestManager_InstallBin(t *testing.T) { tempDir := t.TempDir() m := newTestManager(tempDir) - err := m.InstallBin(&client, repo) + io, _, _, _ := iostreams.Test() + + err := m.Install(&client, repo, io, config.NewBlankConfig()) assert.NoError(t, err) manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) From 0e2861a507cc079bed0f5883bd98f38f9cacee92 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:05:19 -0500 Subject: [PATCH 24/29] WIP refactoring --- pkg/cmd/extension/manager.go | 8 +-- pkg/extensions/extension.go | 2 - pkg/extensions/manager_mock.go | 104 --------------------------------- 3 files changed, 4 insertions(+), 110 deletions(-) diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 75cb4cfd9..db6ec76ba 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -195,7 +195,7 @@ func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostre return fmt.Errorf("could not check for binary extension: %w", err) } if isBin { - return m.InstallBin(client, repo) + return m.installBin(client, repo) } hs, err := hasScript(client, repo) @@ -208,10 +208,10 @@ func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostre } protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.InstallGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) } -func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { +func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { var r *release r, err := fetchLatestRelease(client, repo) if err != nil { @@ -276,7 +276,7 @@ func (m *Manager) InstallBin(client *http.Client, repo ghrepo.Interface) error { return nil } -func (m *Manager) InstallGit(cloneURL string, stdout, stderr io.Writer) error { +func (m *Manager) installGit(cloneURL string, stdout, stderr io.Writer) error { exe, err := m.lookPath("git") if err != nil { return err diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 99087fe04..16a3c7494 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -22,8 +22,6 @@ type Extension interface { type ExtensionManager interface { List(includeMetadata bool) []Extension Install(*http.Client, ghrepo.Interface, *iostreams.IOStreams, config.Config) error - InstallBin(*http.Client, ghrepo.Interface) error - InstallGit(string, io.Writer, io.Writer) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index b8d18dc6d..ce7a71ce8 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -31,12 +31,6 @@ var _ ExtensionManager = &ExtensionManagerMock{} // InstallFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { // panic("mock out the Install method") // }, -// InstallBinFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface) error { -// panic("mock out the InstallBin method") -// }, -// InstallGitFunc: func(s string, writer1 io.Writer, writer2 io.Writer) error { -// panic("mock out the InstallGit method") -// }, // InstallLocalFunc: func(dir string) error { // panic("mock out the InstallLocal method") // }, @@ -65,12 +59,6 @@ type ExtensionManagerMock struct { // InstallFunc mocks the Install method. InstallFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error - // InstallBinFunc mocks the InstallBin method. - InstallBinFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface) error - - // InstallGitFunc mocks the InstallGit method. - InstallGitFunc func(s string, writer1 io.Writer, writer2 io.Writer) error - // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -112,22 +100,6 @@ type ExtensionManagerMock struct { // ConfigMoqParam is the configMoqParam argument value. ConfigMoqParam config.Config } - // InstallBin holds details about calls to the InstallBin method. - InstallBin []struct { - // Client is the client argument value. - Client *http.Client - // InterfaceMoqParam is the interfaceMoqParam argument value. - InterfaceMoqParam ghrepo.Interface - } - // InstallGit holds details about calls to the InstallGit method. - InstallGit []struct { - // S is the s argument value. - S string - // Writer1 is the writer1 argument value. - Writer1 io.Writer - // Writer2 is the writer2 argument value. - Writer2 io.Writer - } // InstallLocal holds details about calls to the InstallLocal method. InstallLocal []struct { // Dir is the dir argument value. @@ -158,8 +130,6 @@ type ExtensionManagerMock struct { lockCreate sync.RWMutex lockDispatch sync.RWMutex lockInstall sync.RWMutex - lockInstallBin sync.RWMutex - lockInstallGit sync.RWMutex lockInstallLocal sync.RWMutex lockList sync.RWMutex lockRemove sync.RWMutex @@ -283,80 +253,6 @@ func (mock *ExtensionManagerMock) InstallCalls() []struct { return calls } -// InstallBin calls InstallBinFunc. -func (mock *ExtensionManagerMock) InstallBin(client *http.Client, interfaceMoqParam ghrepo.Interface) error { - if mock.InstallBinFunc == nil { - panic("ExtensionManagerMock.InstallBinFunc: method is nil but ExtensionManager.InstallBin was just called") - } - callInfo := struct { - Client *http.Client - InterfaceMoqParam ghrepo.Interface - }{ - Client: client, - InterfaceMoqParam: interfaceMoqParam, - } - mock.lockInstallBin.Lock() - mock.calls.InstallBin = append(mock.calls.InstallBin, callInfo) - mock.lockInstallBin.Unlock() - return mock.InstallBinFunc(client, interfaceMoqParam) -} - -// InstallBinCalls gets all the calls that were made to InstallBin. -// Check the length with: -// len(mockedExtensionManager.InstallBinCalls()) -func (mock *ExtensionManagerMock) InstallBinCalls() []struct { - Client *http.Client - InterfaceMoqParam ghrepo.Interface -} { - var calls []struct { - Client *http.Client - InterfaceMoqParam ghrepo.Interface - } - mock.lockInstallBin.RLock() - calls = mock.calls.InstallBin - mock.lockInstallBin.RUnlock() - return calls -} - -// InstallGit calls InstallGitFunc. -func (mock *ExtensionManagerMock) InstallGit(s string, writer1 io.Writer, writer2 io.Writer) error { - if mock.InstallGitFunc == nil { - panic("ExtensionManagerMock.InstallGitFunc: method is nil but ExtensionManager.InstallGit was just called") - } - callInfo := struct { - S string - Writer1 io.Writer - Writer2 io.Writer - }{ - S: s, - Writer1: writer1, - Writer2: writer2, - } - mock.lockInstallGit.Lock() - mock.calls.InstallGit = append(mock.calls.InstallGit, callInfo) - mock.lockInstallGit.Unlock() - return mock.InstallGitFunc(s, writer1, writer2) -} - -// InstallGitCalls gets all the calls that were made to InstallGit. -// Check the length with: -// len(mockedExtensionManager.InstallGitCalls()) -func (mock *ExtensionManagerMock) InstallGitCalls() []struct { - S string - Writer1 io.Writer - Writer2 io.Writer -} { - var calls []struct { - S string - Writer1 io.Writer - Writer2 io.Writer - } - mock.lockInstallGit.RLock() - calls = mock.calls.InstallGit - mock.lockInstallGit.RUnlock() - return calls -} - // InstallLocal calls InstallLocalFunc. func (mock *ExtensionManagerMock) InstallLocal(dir string) error { if mock.InstallLocalFunc == nil { From e85b0480e90820a52dbbf8748602d67eced01524 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:10:18 -0500 Subject: [PATCH 25/29] track installed tag name --- pkg/cmd/extension/http.go | 1 + pkg/cmd/extension/manager.go | 7 ++++--- pkg/cmd/extension/manager_test.go | 6 ++++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/extension/http.go b/pkg/cmd/extension/http.go index de6f61e4b..6f93f2303 100644 --- a/pkg/cmd/extension/http.go +++ b/pkg/cmd/extension/http.go @@ -47,6 +47,7 @@ type releaseAsset struct { } type release struct { + Tag string `json:"tag_name"` Assets []releaseAsset } diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index db6ec76ba..05e048588 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -180,12 +180,12 @@ func (m *Manager) InstallLocal(dir string) error { return makeSymlink(dir, targetLink) } -type BinManifest struct { +type binManifest struct { Owner string Name string Host string + Tag string // TODO I may end up not using this; just thinking ahead to local installs - // TODO track version Path string } @@ -248,11 +248,12 @@ func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) } - manifest := BinManifest{ + manifest := binManifest{ Name: name, Owner: repo.RepoOwner(), Host: repo.RepoHost(), Path: binPath, + Tag: r.Tag, } bs, err := yaml.Marshal(manifest) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index f0a930dcd..8c4ca604e 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -254,6 +254,7 @@ func TestManager_Install_binary(t *testing.T) { httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), httpmock.JSONResponse( release{ + Tag: "v1.0.1", Assets: []releaseAsset{ { Name: "gh-bin-ext-windows-amd64", @@ -276,14 +277,15 @@ func TestManager_Install_binary(t *testing.T) { manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) assert.NoError(t, err) - var bm BinManifest + var bm binManifest err = yaml.Unmarshal(manifest, &bm) assert.NoError(t, err) - assert.Equal(t, BinManifest{ + assert.Equal(t, binManifest{ Name: "gh-bin-ext", Owner: "owner", Host: "example.com", + Tag: "v1.0.1", Path: filepath.Join(tempDir, "extensions/gh-bin-ext/gh-bin-ext"), }, bm) From 1f3b872859d33a5e5d1a4445c66f10aa6899b88f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 20 Sep 2021 17:17:18 -0500 Subject: [PATCH 26/29] test for unsupported platform --- pkg/cmd/extension/manager_test.go | 44 +++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 8c4ca604e..3528d9920 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -232,6 +232,50 @@ func TestManager_Install_git(t *testing.T) { assert.Equal(t, "", stderr.String()) } +func TestManager_Install_binary_unsupported(t *testing.T) { + repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") + + reg := httpmock.Registry{} + defer reg.Verify(t) + client := http.Client{Transport: ®} + + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-linux-amd64", + APIURL: "https://example.com/release/cool", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "api/v3/repos/owner/gh-bin-ext/releases/latest"), + httpmock.JSONResponse( + release{ + Tag: "v1.0.1", + Assets: []releaseAsset{ + { + Name: "gh-bin-ext-linux-amd64", + APIURL: "https://example.com/release/cool", + }, + }, + })) + + tempDir := t.TempDir() + m := newTestManager(tempDir) + + io, _, _, _ := iostreams.Test() + + err := m.Install(&client, repo, io, config.NewBlankConfig()) + assert.Error(t, err) + + errText := "gh-bin-ext unsupported for windows-amd64. Open an issue: `gh issue create -R owner/gh-bin-ext -t'Support windows-amd64'`" + + assert.Equal(t, errText, err.Error()) +} + func TestManager_Install_binary(t *testing.T) { repo := ghrepo.NewWithHost("owner", "gh-bin-ext", "example.com") From 514d4d992ce4900f18f286b241d788a01c721d0e Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Sep 2021 15:55:31 -0500 Subject: [PATCH 27/29] refactor dependencies of ext manager --- pkg/cmd/extension/command.go | 82 +---------------------- pkg/cmd/extension/command_test.go | 2 +- pkg/cmd/extension/manager.go | 106 +++++++++++++++++++++++++++--- pkg/cmd/extension/manager_test.go | 48 +++++++------- pkg/cmd/factory/default.go | 20 +++++- pkg/extensions/extension.go | 5 +- pkg/extensions/manager_mock.go | 29 ++------ 7 files changed, 148 insertions(+), 144 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 9ac5e5f21..03466b84d 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -3,7 +3,6 @@ package extension import ( "errors" "fmt" - "net/http" "os" "strings" "time" @@ -113,12 +112,7 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { } client = api.NewCachedClient(client, time.Second*30) - cfg, err := f.Config() - if err != nil { - return err - } - - return m.Install(client, repo, io, cfg) + return m.Install(repo) }, }, func() *cobra.Command { @@ -228,83 +222,9 @@ func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, return nil } -func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { - var r *release - r, err = fetchLatestRelease(client, repo) - if err != nil { - httpErr, ok := err.(api.HTTPError) - if ok && httpErr.StatusCode == 404 { - err = nil - return - } - return - } - - for _, a := range r.Assets { - dists := possibleDists() - for _, d := range dists { - if strings.HasSuffix(a.Name, d) { - isBin = true - break - } - } - } - - return -} - func normalizeExtensionSelector(n string) string { if idx := strings.IndexRune(n, '/'); idx >= 0 { n = n[idx+1:] } return strings.TrimPrefix(n, "gh-") } - -func possibleDists() []string { - return []string{ - "aix-ppc64", - "android-386", - "android-amd64", - "android-arm", - "android-arm64", - "darwin-amd64", - "darwin-arm64", - "dragonfly-amd64", - "freebsd-386", - "freebsd-amd64", - "freebsd-arm", - "freebsd-arm64", - "illumos-amd64", - "ios-amd64", - "ios-arm64", - "js-wasm", - "linux-386", - "linux-amd64", - "linux-arm", - "linux-arm64", - "linux-mips", - "linux-mips64", - "linux-mips64le", - "linux-mipsle", - "linux-ppc64", - "linux-ppc64le", - "linux-riscv64", - "linux-s390x", - "netbsd-386", - "netbsd-amd64", - "netbsd-arm", - "netbsd-arm64", - "openbsd-386", - "openbsd-amd64", - "openbsd-arm", - "openbsd-arm64", - "openbsd-mips64", - "plan9-386", - "plan9-amd64", - "plan9-arm", - "solaris-amd64", - "windows-386", - "windows-amd64", - "windows-arm", - } -} diff --git a/pkg/cmd/extension/command_test.go b/pkg/cmd/extension/command_test.go index 9b363fbb7..fac5d0e9a 100644 --- a/pkg/cmd/extension/command_test.go +++ b/pkg/cmd/extension/command_test.go @@ -42,7 +42,7 @@ func TestNewCmdExtension(t *testing.T) { em.ListFunc = func(bool) []extensions.Extension { return []extensions.Extension{} } - em.InstallFunc = func(_ *http.Client, _ ghrepo.Interface, _ *iostreams.IOStreams, _ config.Config) error { + em.InstallFunc = func(_ ghrepo.Interface) error { return nil } return func(t *testing.T) { diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 05e048588..7e1f403e2 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -15,6 +15,7 @@ import ( "strings" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/extensions" @@ -30,9 +31,12 @@ type Manager struct { findSh func() (string, error) newCommand func(string, ...string) *exec.Cmd platform func() string + client *http.Client + config config.Config + io *iostreams.IOStreams } -func NewManager() *Manager { +func NewManager(io *iostreams.IOStreams) *Manager { return &Manager{ dataDir: config.DataDir, lookPath: safeexec.LookPath, @@ -44,6 +48,14 @@ func NewManager() *Manager { } } +func (m *Manager) SetConfig(cfg config.Config) { + m.config = cfg +} + +func (m *Manager) SetClient(client *http.Client) { + m.client = client +} + func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Writer) (bool, error) { if len(args) == 0 { return false, errors.New("too few arguments in list") @@ -189,16 +201,16 @@ type binManifest struct { Path string } -func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostreams.IOStreams, cfg config.Config) error { - isBin, err := isBinExtension(client, repo) +func (m *Manager) Install(repo ghrepo.Interface) error { + isBin, err := isBinExtension(m.client, repo) if err != nil { return fmt.Errorf("could not check for binary extension: %w", err) } if isBin { - return m.installBin(client, repo) + return m.installBin(repo) } - hs, err := hasScript(client, repo) + hs, err := hasScript(m.client, repo) if err != nil { return err } @@ -207,13 +219,13 @@ func (m *Manager) Install(client *http.Client, repo ghrepo.Interface, io *iostre return errors.New("extension is uninstallable: missing executable") } - protocol, _ := cfg.Get(repo.RepoHost(), "git_protocol") - return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), io.Out, io.ErrOut) + protocol, _ := m.config.Get(repo.RepoHost(), "git_protocol") + return m.installGit(ghrepo.FormatRemoteURL(repo, protocol), m.io.Out, m.io.ErrOut) } -func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { +func (m *Manager) installBin(repo ghrepo.Interface) error { var r *release - r, err := fetchLatestRelease(client, repo) + r, err := fetchLatestRelease(m.client, repo) if err != nil { return err } @@ -243,7 +255,7 @@ func (m *Manager) installBin(client *http.Client, repo ghrepo.Interface) error { binPath := filepath.Join(targetDir, name) - err = downloadAsset(client, *asset, binPath) + err = downloadAsset(m.client, *asset, binPath) if err != nil { return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) } @@ -451,3 +463,77 @@ func readPathFromFile(path string) (string, error) { n, err := f.Read(b) return strings.TrimSpace(string(b[:n])), err } + +func isBinExtension(client *http.Client, repo ghrepo.Interface) (isBin bool, err error) { + var r *release + r, err = fetchLatestRelease(client, repo) + if err != nil { + httpErr, ok := err.(api.HTTPError) + if ok && httpErr.StatusCode == 404 { + err = nil + return + } + return + } + + for _, a := range r.Assets { + dists := possibleDists() + for _, d := range dists { + if strings.HasSuffix(a.Name, d) { + isBin = true + break + } + } + } + + return +} + +func possibleDists() []string { + return []string{ + "aix-ppc64", + "android-386", + "android-amd64", + "android-arm", + "android-arm64", + "darwin-amd64", + "darwin-arm64", + "dragonfly-amd64", + "freebsd-386", + "freebsd-amd64", + "freebsd-arm", + "freebsd-arm64", + "illumos-amd64", + "ios-amd64", + "ios-arm64", + "js-wasm", + "linux-386", + "linux-amd64", + "linux-arm", + "linux-arm64", + "linux-mips", + "linux-mips64", + "linux-mips64le", + "linux-mipsle", + "linux-ppc64", + "linux-ppc64le", + "linux-riscv64", + "linux-s390x", + "netbsd-386", + "netbsd-amd64", + "netbsd-arm", + "netbsd-arm64", + "openbsd-386", + "openbsd-amd64", + "openbsd-arm", + "openbsd-arm64", + "openbsd-mips64", + "plan9-386", + "plan9-amd64", + "plan9-arm", + "solaris-amd64", + "windows-386", + "windows-amd64", + "windows-arm", + } +} diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 3528d9920..d78f4c2e6 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -34,7 +34,7 @@ func TestHelperProcess(t *testing.T) { os.Exit(0) } -func newTestManager(dir string) *Manager { +func newTestManager(dir string, client *http.Client, io *iostreams.IOStreams) *Manager { return &Manager{ dataDir: func() string { return dir }, lookPath: func(exe string) (string, error) { return exe, nil }, @@ -45,6 +45,9 @@ func newTestManager(dir string) *Manager { cmd.Env = []string{"GH_WANT_HELPER_PROCESS=1"} return cmd }, + config: config.NewBlankConfig(), + io: io, + client: client, platform: func() string { return "windows-amd64" }, @@ -56,7 +59,7 @@ func TestManager_List(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-two", "gh-two"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) exts := m.List(false) assert.Equal(t, 2, len(exts)) assert.Equal(t, "hello", exts[0].Name()) @@ -68,7 +71,7 @@ func TestManager_Dispatch(t *testing.T) { extPath := filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello") assert.NoError(t, stubExtension(extPath)) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -89,7 +92,7 @@ func TestManager_Remove(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-hello", "gh-hello"))) assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-two", "gh-two"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) err := m.Remove("hello") assert.NoError(t, err) @@ -105,7 +108,7 @@ func TestManager_Upgrade_AllExtensions(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-two", "gh-two"))) assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -130,7 +133,7 @@ func TestManager_Upgrade_RemoteExtension(t *testing.T) { tempDir := t.TempDir() assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -150,7 +153,7 @@ func TestManager_Upgrade_LocalExtension(t *testing.T) { tempDir := t.TempDir() assert.NoError(t, stubLocalExtension(tempDir, filepath.Join(tempDir, "extensions", "gh-local", "gh-local"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -167,7 +170,7 @@ func TestManager_Upgrade_Force(t *testing.T) { assert.NoError(t, stubExtension(filepath.Join(tempDir, "extensions", "gh-remote", "gh-remote"))) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -189,7 +192,7 @@ func TestManager_Upgrade_Force(t *testing.T) { func TestManager_Upgrade_NoExtensions(t *testing.T) { tempDir := t.TempDir() - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} @@ -201,12 +204,15 @@ func TestManager_Upgrade_NoExtensions(t *testing.T) { func TestManager_Install_git(t *testing.T) { tempDir := t.TempDir() - m := newTestManager(tempDir) reg := httpmock.Registry{} defer reg.Verify(t) client := http.Client{Transport: ®} + io, _, stdout, stderr := iostreams.Test() + + m := newTestManager(tempDir, &client, io) + reg.Register( httpmock.REST("GET", "repos/owner/gh-some-ext/releases/latest"), httpmock.JSONResponse( @@ -222,11 +228,9 @@ func TestManager_Install_git(t *testing.T) { httpmock.REST("GET", "repos/owner/gh-some-ext/contents/gh-some-ext"), httpmock.StringResponse("script")) - io, _, stdout, stderr := iostreams.Test() - repo := ghrepo.New("owner", "gh-some-ext") - err := m.Install(&client, repo, io, config.NewBlankConfig()) + err := m.Install(repo) assert.NoError(t, err) assert.Equal(t, fmt.Sprintf("[git clone https://github.com/owner/gh-some-ext.git %s]\n", filepath.Join(tempDir, "extensions", "gh-some-ext")), stdout.String()) assert.Equal(t, "", stderr.String()) @@ -263,12 +267,12 @@ func TestManager_Install_binary_unsupported(t *testing.T) { }, })) - tempDir := t.TempDir() - m := newTestManager(tempDir) - io, _, _, _ := iostreams.Test() + tempDir := t.TempDir() - err := m.Install(&client, repo, io, config.NewBlankConfig()) + m := newTestManager(tempDir, &client, io) + + err := m.Install(repo) assert.Error(t, err) errText := "gh-bin-ext unsupported for windows-amd64. Open an issue: `gh issue create -R owner/gh-bin-ext -t'Support windows-amd64'`" @@ -310,12 +314,12 @@ func TestManager_Install_binary(t *testing.T) { httpmock.REST("GET", "release/cool"), httpmock.StringResponse("FAKE BINARY")) - tempDir := t.TempDir() - m := newTestManager(tempDir) - io, _, _, _ := iostreams.Test() + tempDir := t.TempDir() - err := m.Install(&client, repo, io, config.NewBlankConfig()) + m := newTestManager(tempDir, &client, io) + + err := m.Install(repo) assert.NoError(t, err) manifest, err := os.ReadFile(filepath.Join(tempDir, "extensions/gh-bin-ext/manifest.yml")) @@ -344,7 +348,7 @@ func TestManager_Create(t *testing.T) { oldWd, _ := os.Getwd() assert.NoError(t, os.Chdir(tempDir)) t.Cleanup(func() { _ = os.Chdir(oldWd) }) - m := newTestManager(tempDir) + m := newTestManager(tempDir, nil, nil) err := m.Create("gh-test") assert.NoError(t, err) files, err := ioutil.ReadDir(filepath.Join(tempDir, "gh-test")) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 961883b12..2ba926033 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -22,7 +22,6 @@ func New(appVersion string) *cmdutil.Factory { Branch: branchFunc(), // No factory dependencies Executable: executable(), // No factory dependencies - ExtensionManager: extension.NewManager(), } f.IOStreams = ioStreams(f) // Depends on Config @@ -30,6 +29,7 @@ func New(appVersion string) *cmdutil.Factory { f.Remotes = remotesFunc(f) // Depends on Config f.BaseRepo = BaseRepoFunc(f) // Depends on Remotes f.Browser = browser(f) // Depends on Config, and IOStreams + f.ExtensionManager = extensionManager(f) // Depends on Config, HttpClient, and IOStreams return f } @@ -148,6 +148,24 @@ func branchFunc() func() (string, error) { } } +func extensionManager(f *cmdutil.Factory) *extension.Manager { + em := extension.NewManager(f.IOStreams) + + cfg, err := f.Config() + if err != nil { + return em + } + em.SetConfig(cfg) + + client, err := f.HttpClient() + if err != nil { + return em + } + em.SetClient(client) + + return em +} + func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io := iostreams.System() cfg, err := f.Config() diff --git a/pkg/extensions/extension.go b/pkg/extensions/extension.go index 16a3c7494..4e9ce89b5 100644 --- a/pkg/extensions/extension.go +++ b/pkg/extensions/extension.go @@ -2,11 +2,8 @@ package extensions import ( "io" - "net/http" - "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/pkg/iostreams" ) //go:generate moq -rm -out extension_mock.go . Extension @@ -21,7 +18,7 @@ type Extension interface { //go:generate moq -rm -out manager_mock.go . ExtensionManager type ExtensionManager interface { List(includeMetadata bool) []Extension - Install(*http.Client, ghrepo.Interface, *iostreams.IOStreams, config.Config) error + Install(ghrepo.Interface) error InstallLocal(dir string) error Upgrade(name string, force bool, stdout, stderr io.Writer) error Remove(name string) error diff --git a/pkg/extensions/manager_mock.go b/pkg/extensions/manager_mock.go index ce7a71ce8..96d76cdf7 100644 --- a/pkg/extensions/manager_mock.go +++ b/pkg/extensions/manager_mock.go @@ -4,11 +4,8 @@ package extensions import ( - "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/pkg/iostreams" "io" - "net/http" "sync" ) @@ -28,7 +25,7 @@ var _ ExtensionManager = &ExtensionManagerMock{} // DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) { // panic("mock out the Dispatch method") // }, -// InstallFunc: func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { +// InstallFunc: func(interfaceMoqParam ghrepo.Interface) error { // panic("mock out the Install method") // }, // InstallLocalFunc: func(dir string) error { @@ -57,7 +54,7 @@ type ExtensionManagerMock struct { DispatchFunc func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) // InstallFunc mocks the Install method. - InstallFunc func(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error + InstallFunc func(interfaceMoqParam ghrepo.Interface) error // InstallLocalFunc mocks the InstallLocal method. InstallLocalFunc func(dir string) error @@ -91,14 +88,8 @@ type ExtensionManagerMock struct { } // Install holds details about calls to the Install method. Install []struct { - // Client is the client argument value. - Client *http.Client // InterfaceMoqParam is the interfaceMoqParam argument value. InterfaceMoqParam ghrepo.Interface - // IOStreams is the iOStreams argument value. - IOStreams *iostreams.IOStreams - // ConfigMoqParam is the configMoqParam argument value. - ConfigMoqParam config.Config } // InstallLocal holds details about calls to the InstallLocal method. InstallLocal []struct { @@ -211,41 +202,29 @@ func (mock *ExtensionManagerMock) DispatchCalls() []struct { } // Install calls InstallFunc. -func (mock *ExtensionManagerMock) Install(client *http.Client, interfaceMoqParam ghrepo.Interface, iOStreams *iostreams.IOStreams, configMoqParam config.Config) error { +func (mock *ExtensionManagerMock) Install(interfaceMoqParam ghrepo.Interface) error { if mock.InstallFunc == nil { panic("ExtensionManagerMock.InstallFunc: method is nil but ExtensionManager.Install was just called") } callInfo := struct { - Client *http.Client InterfaceMoqParam ghrepo.Interface - IOStreams *iostreams.IOStreams - ConfigMoqParam config.Config }{ - Client: client, InterfaceMoqParam: interfaceMoqParam, - IOStreams: iOStreams, - ConfigMoqParam: configMoqParam, } mock.lockInstall.Lock() mock.calls.Install = append(mock.calls.Install, callInfo) mock.lockInstall.Unlock() - return mock.InstallFunc(client, interfaceMoqParam, iOStreams, configMoqParam) + return mock.InstallFunc(interfaceMoqParam) } // InstallCalls gets all the calls that were made to Install. // Check the length with: // len(mockedExtensionManager.InstallCalls()) func (mock *ExtensionManagerMock) InstallCalls() []struct { - Client *http.Client InterfaceMoqParam ghrepo.Interface - IOStreams *iostreams.IOStreams - ConfigMoqParam config.Config } { var calls []struct { - Client *http.Client InterfaceMoqParam ghrepo.Interface - IOStreams *iostreams.IOStreams - ConfigMoqParam config.Config } mock.lockInstall.RLock() calls = mock.calls.Install From 5f02ed2656b5c2846505cb4c6a570f508a9bb7ad Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Sep 2021 15:59:50 -0500 Subject: [PATCH 28/29] linter appeasement --- pkg/cmd/extension/command.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/pkg/cmd/extension/command.go b/pkg/cmd/extension/command.go index 03466b84d..ad92a8f14 100644 --- a/pkg/cmd/extension/command.go +++ b/pkg/cmd/extension/command.go @@ -5,10 +5,8 @@ import ( "fmt" "os" "strings" - "time" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -103,14 +101,10 @@ func NewCmdExtension(f *cmdutil.Factory) *cobra.Command { if err != nil { return err } + if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { return err } - client, err := f.HttpClient() - if err != nil { - return fmt.Errorf("could not make http client: %w", err) - } - client = api.NewCachedClient(client, time.Second*30) return m.Install(repo) }, From 7bf85355a92b80437df7470b53e12a554238d0fc Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Sep 2021 15:59:57 -0500 Subject: [PATCH 29/29] restore cached client --- pkg/cmd/factory/default.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 2ba926033..61bbe2c54 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "os" + "time" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/context" @@ -161,7 +162,8 @@ func extensionManager(f *cmdutil.Factory) *extension.Manager { if err != nil { return em } - em.SetClient(client) + + em.SetClient(api.NewCachedClient(client, time.Second*30)) return em }