From 3bb6983b3503686250792f85a18651a806b8d478 Mon Sep 17 00:00:00 2001 From: naman <1977419+metalogical@users.noreply.github.com> Date: Wed, 10 Jun 2020 11:41:44 -0700 Subject: [PATCH 01/35] fix regression in support for detached HEAD state for gh pr status --- command/pr.go | 2 +- context/context.go | 6 +++++- git/git.go | 5 ++++- git/git_test.go | 5 ++--- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/command/pr.go b/command/pr.go index e92fa7cf0..f1c1eb101 100644 --- a/command/pr.go +++ b/command/pr.go @@ -116,7 +116,7 @@ func prStatus(cmd *cobra.Command, args []string) error { repoOverride, _ := cmd.Flags().GetString("repo") currentPRNumber, currentPRHeadRef, err := prSelectorForCurrentBranch(ctx, baseRepo) - if err != nil && repoOverride == "" && err.Error() != "git: not on any branch" { + if err != nil && repoOverride == "" && err != git.ErrNotOnAnyBranch { return fmt.Errorf("could not query for pull request for current branch: %w", err) } diff --git a/context/context.go b/context/context.go index 236f9e722..4f1aa10ba 100644 --- a/context/context.go +++ b/context/context.go @@ -207,7 +207,11 @@ func (c *fsContext) Branch() (string, error) { } currentBranch, err := git.CurrentBranch() - if err != nil { + switch err { + case nil: + case git.ErrNotOnAnyBranch: + return "", err + default: return "", fmt.Errorf("could not determine current branch: %w", err) } diff --git a/git/git.go b/git/git.go index 355f905f0..ff40adde3 100644 --- a/git/git.go +++ b/git/git.go @@ -13,6 +13,9 @@ import ( "github.com/cli/cli/internal/run" ) +// ErrNotOnAnyBranch indicates that the users is in detached HEAD state +var ErrNotOnAnyBranch = errors.New("git: not on any branch") + // Ref represents a git commit reference type Ref struct { Hash string @@ -64,7 +67,7 @@ func CurrentBranch() (string, error) { if errors.As(err, &cmdErr) { if cmdErr.Stderr.Len() == 0 { // Detached head - return "", errors.New("git: not on any branch") + return "", ErrNotOnAnyBranch } } diff --git a/git/git_test.go b/git/git_test.go index c84023e40..1a8f32677 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -67,9 +67,8 @@ func Test_CurrentBranch_detached_head(t *testing.T) { if err == nil { t.Errorf("expected an error") } - expectedError := "git: not on any branch" - if err.Error() != expectedError { - t.Errorf("got unexpected error: %s instead of %s", err.Error(), expectedError) + if err != ErrNotOnAnyBranch { + t.Errorf("got unexpected error: %s instead of %s", err, ErrNotOnAnyBranch) } if len(cs.Calls) != 1 { t.Errorf("expected 1 git call, saw %d", len(cs.Calls)) From fb7b01b40c3353015ec9b706d68b8626b356ec6d Mon Sep 17 00:00:00 2001 From: naman <1977419+metalogical@users.noreply.github.com> Date: Thu, 11 Jun 2020 17:05:25 -0700 Subject: [PATCH 02/35] resolve PR review --- command/pr.go | 2 +- command/pr_test.go | 32 ++++++++++++++++++++++++++++++++ context/blank_context.go | 3 ++- context/context.go | 6 +----- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/command/pr.go b/command/pr.go index f1c1eb101..10a41246d 100644 --- a/command/pr.go +++ b/command/pr.go @@ -116,7 +116,7 @@ func prStatus(cmd *cobra.Command, args []string) error { repoOverride, _ := cmd.Flags().GetString("repo") currentPRNumber, currentPRHeadRef, err := prSelectorForCurrentBranch(ctx, baseRepo) - if err != nil && repoOverride == "" && err != git.ErrNotOnAnyBranch { + if err != nil && repoOverride == "" && !errors.Is(err, git.ErrNotOnAnyBranch) { return fmt.Errorf("could not query for pull request for current branch: %w", err) } diff --git a/command/pr_test.go b/command/pr_test.go index a11c60820..ee90a66b9 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -299,6 +299,38 @@ Requesting a code review from you } } +func TestPRStatus_detachedHead(t *testing.T) { + initBlankContext("", "OWNER/REPO", "") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": {} } + `)) + + output, err := RunCommand("pr status") + if err != nil { + t.Errorf("error running command `pr status`: %v", err) + } + + expected := ` +Relevant pull requests in OWNER/REPO + +Current branch + There is no current branch + +Created by you + You have no open pull requests + +Requesting a code review from you + You have no pull requests to review + +` + if output.String() != expected { + t.Errorf("expected %q, got %q", expected, output.String()) + } +} + func TestPRList(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() diff --git a/context/blank_context.go b/context/blank_context.go index 3ea657abe..7bb5282ec 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -18,6 +18,7 @@ func NewBlank() *blankContext { type blankContext struct { authToken string branch string + branchErr error baseRepo ghrepo.Interface remotes Remotes } @@ -40,7 +41,7 @@ func (c *blankContext) SetAuthToken(t string) { func (c *blankContext) Branch() (string, error) { if c.branch == "" { - return "", fmt.Errorf("branch was not initialized") + return "", fmt.Errorf("branch was not initialized: %w", git.ErrNotOnAnyBranch) } return c.branch, nil } diff --git a/context/context.go b/context/context.go index 4f1aa10ba..236f9e722 100644 --- a/context/context.go +++ b/context/context.go @@ -207,11 +207,7 @@ func (c *fsContext) Branch() (string, error) { } currentBranch, err := git.CurrentBranch() - switch err { - case nil: - case git.ErrNotOnAnyBranch: - return "", err - default: + if err != nil { return "", fmt.Errorf("could not determine current branch: %w", err) } From fffa39368356e870f70ff051518c1814b9df7f6a Mon Sep 17 00:00:00 2001 From: naman <1977419+metalogical@users.noreply.github.com> Date: Thu, 11 Jun 2020 17:08:18 -0700 Subject: [PATCH 03/35] fix lint --- context/blank_context.go | 1 - 1 file changed, 1 deletion(-) diff --git a/context/blank_context.go b/context/blank_context.go index 7bb5282ec..80a2cd298 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -18,7 +18,6 @@ func NewBlank() *blankContext { type blankContext struct { authToken string branch string - branchErr error baseRepo ghrepo.Interface remotes Remotes } From e6e8f701a2b3832806649d56490115c447ded754 Mon Sep 17 00:00:00 2001 From: gabgodBB Date: Fri, 12 Jun 2020 14:26:38 -0300 Subject: [PATCH 04/35] Issue #930 - Removing color for output files --- utils/color.go | 4 ++++ utils/utils.go | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/utils/color.go b/utils/color.go index 4940fe26b..e88549667 100644 --- a/utils/color.go +++ b/utils/color.go @@ -53,6 +53,10 @@ func makeColorFunc(color string) func(string) string { } func isColorEnabled() bool { + if !isStdoutTerminal() { + return false + } + if !checkedNoColor { _isColorEnabled = os.Getenv("NO_COLOR") == "" checkedNoColor = true diff --git a/utils/utils.go b/utils/utils.go index c84860d9e..9441bc2bc 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -23,6 +23,10 @@ func OpenInBrowser(url string) error { func RenderMarkdown(text string) (string, error) { style := "notty" + if !isColorEnabled() { + return text, nil + } + if isColorEnabled() { style = "dark" } From 2943703d4a0a35855476ee8d884b6ef6bbab3cce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edd=C3=BA=20Mel=C3=A9ndez?= Date: Thu, 12 Mar 2020 21:40:11 -0600 Subject: [PATCH 05/35] Add mentioned flag --- api/queries_issue.go | 7 +++++-- api/queries_issue_test.go | 2 +- command/issue.go | 8 +++++++- command/issue_test.go | 12 +++++++----- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 56a04edd9..3da827ae1 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -198,7 +198,7 @@ func IssueStatus(client *Client, repo ghrepo.Interface, currentUsername string) return &payload, nil } -func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string) (*IssuesAndTotalCount, error) { +func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionedString string) (*IssuesAndTotalCount, error) { var states []string switch state { case "open", "": @@ -215,7 +215,7 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String) { repository(owner: $owner, name: $repo) { hasIssuesEnabled - issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author}) { + issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mentioned}) { totalCount nodes { ...issue @@ -243,6 +243,9 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str if authorString != "" { variables["author"] = authorString } + if mentionedString != "" { + variables["mentioned"] = mentionedString + } var response struct { Repository struct { diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index ffe4aaad1..4eff67804 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -40,7 +40,7 @@ func TestIssueList(t *testing.T) { `)) repo, _ := ghrepo.FromFullName("OWNER/REPO") - _, err := IssueList(client, repo, "open", []string{}, "", 251, "") + _, err := IssueList(client, repo, "open", []string{}, "", 251, "", "") if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/command/issue.go b/command/issue.go index 526d02871..85e0696a6 100644 --- a/command/issue.go +++ b/command/issue.go @@ -41,6 +41,7 @@ func init() { issueListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|all}") issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch") issueListCmd.Flags().StringP("author", "A", "", "Filter by author") + issueListCmd.Flags().StringP("mentioned", "", "", "Filter by mention") issueCmd.AddCommand(issueViewCmd) issueViewCmd.Flags().BoolP("web", "w", false, "Open an issue in the browser") @@ -141,7 +142,12 @@ func issueList(cmd *cobra.Command, args []string) error { return err } - listResult, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit, author) + mentioned, err := cmd.Flags().GetString("mentioned") + if err != nil { + return err + } + + listResult, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit, author, mentioned) if err != nil { return err } diff --git a/command/issue_test.go b/command/issue_test.go index 418de25d3..ce91c98e2 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -153,7 +153,7 @@ func TestIssueList_withFlags(t *testing.T) { } } } `)) - output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo") + output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mentioned me") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -167,10 +167,11 @@ No issues match your search in OWNER/REPO bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { - Assignee string - Labels []string - States []string - Author string + Assignee string + Labels []string + States []string + Author string + Mentioned string } }{} _ = json.Unmarshal(bodyBytes, &reqBody) @@ -179,6 +180,7 @@ No issues match your search in OWNER/REPO eq(t, reqBody.Variables.Labels, []string{"web", "bug"}) eq(t, reqBody.Variables.States, []string{"OPEN"}) eq(t, reqBody.Variables.Author, "foo") + eq(t, reqBody.Variables.Mentioned, "me") } func TestIssueList_withInvalidLimitFlag(t *testing.T) { From 9aebb66a3c00ccec2d0af6db968a3ddf37e3df94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edd=C3=BA=20Mel=C3=A9ndez?= Date: Thu, 12 Mar 2020 21:53:45 -0600 Subject: [PATCH 06/35] Add milestone filter --- api/queries_issue.go | 7 +++++-- api/queries_issue_test.go | 2 +- command/issue.go | 8 +++++++- command/issue_test.go | 4 +++- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 3da827ae1..e43fb8ea3 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -198,7 +198,7 @@ func IssueStatus(client *Client, repo ghrepo.Interface, currentUsername string) return &payload, nil } -func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionedString string) (*IssuesAndTotalCount, error) { +func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionedString string, milestoneString string) (*IssuesAndTotalCount, error) { var states []string switch state { case "open", "": @@ -215,7 +215,7 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String) { repository(owner: $owner, name: $repo) { hasIssuesEnabled - issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mentioned}) { + issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mentioned, milestone: $milestone}) { totalCount nodes { ...issue @@ -246,6 +246,9 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str if mentionedString != "" { variables["mentioned"] = mentionedString } + if milestoneString != "" { + variables["milestone"] = milestoneString + } var response struct { Repository struct { diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index 4eff67804..2ea64f0c6 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -40,7 +40,7 @@ func TestIssueList(t *testing.T) { `)) repo, _ := ghrepo.FromFullName("OWNER/REPO") - _, err := IssueList(client, repo, "open", []string{}, "", 251, "", "") + _, err := IssueList(client, repo, "open", []string{}, "", 251, "", "", "") if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/command/issue.go b/command/issue.go index 85e0696a6..1b4befd67 100644 --- a/command/issue.go +++ b/command/issue.go @@ -42,6 +42,7 @@ func init() { issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch") issueListCmd.Flags().StringP("author", "A", "", "Filter by author") issueListCmd.Flags().StringP("mentioned", "", "", "Filter by mention") + issueListCmd.Flags().StringP("milestone", "", "", "Filter by milestone") issueCmd.AddCommand(issueViewCmd) issueViewCmd.Flags().BoolP("web", "w", false, "Open an issue in the browser") @@ -147,7 +148,12 @@ func issueList(cmd *cobra.Command, args []string) error { return err } - listResult, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit, author, mentioned) + milestone, err := cmd.Flags().GetString("milestone") + if err != nil { + return err + } + + listResult, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit, author, mentioned, milestone) if err != nil { return err } diff --git a/command/issue_test.go b/command/issue_test.go index ce91c98e2..b834f26c7 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -153,7 +153,7 @@ func TestIssueList_withFlags(t *testing.T) { } } } `)) - output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mentioned me") + output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mentioned me --milestone 1.x") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -172,6 +172,7 @@ No issues match your search in OWNER/REPO States []string Author string Mentioned string + Milestone string } }{} _ = json.Unmarshal(bodyBytes, &reqBody) @@ -181,6 +182,7 @@ No issues match your search in OWNER/REPO eq(t, reqBody.Variables.States, []string{"OPEN"}) eq(t, reqBody.Variables.Author, "foo") eq(t, reqBody.Variables.Mentioned, "me") + eq(t, reqBody.Variables.Milestone, "1.x") } func TestIssueList_withInvalidLimitFlag(t *testing.T) { From a16405650f7b99e42e99938ced5d81247edd1a49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edd=C3=BA=20Mel=C3=A9ndez?= Date: Sat, 13 Jun 2020 20:55:55 -0500 Subject: [PATCH 07/35] Define query variables --- api/queries_issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index e43fb8ea3..55d7515f0 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -212,7 +212,7 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str } query := fragments + ` - query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String) { + query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String, $mentioned: String, $milestone: String) { repository(owner: $owner, name: $repo) { hasIssuesEnabled issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mentioned, milestone: $milestone}) { From 8a96299735711353a7ae9da9882123650a82341f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edd=C3=BA=20Mel=C3=A9ndez?= Date: Sat, 13 Jun 2020 21:07:12 -0500 Subject: [PATCH 08/35] Fix lint --- command/issue_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/command/issue_test.go b/command/issue_test.go index b834f26c7..19ac94d01 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -167,12 +167,12 @@ No issues match your search in OWNER/REPO bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { - Assignee string - Labels []string - States []string - Author string - Mentioned string - Milestone string + Assignee string + Labels []string + States []string + Author string + Mentioned string + Milestone string } }{} _ = json.Unmarshal(bodyBytes, &reqBody) From cffd56f717570a33ca61e44348ae3319ebbfe91e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edd=C3=BA=20Mel=C3=A9ndez?= Date: Mon, 15 Jun 2020 15:58:07 -0500 Subject: [PATCH 09/35] Rename to mention --- api/queries_issue.go | 10 +++++----- command/issue.go | 6 +++--- command/issue_test.go | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 55d7515f0..76981f8d7 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -198,7 +198,7 @@ func IssueStatus(client *Client, repo ghrepo.Interface, currentUsername string) return &payload, nil } -func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionedString string, milestoneString string) (*IssuesAndTotalCount, error) { +func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int, authorString string, mentionString string, milestoneString string) (*IssuesAndTotalCount, error) { var states []string switch state { case "open", "": @@ -212,10 +212,10 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str } query := fragments + ` - query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String, $mentioned: String, $milestone: String) { + query($owner: String!, $repo: String!, $limit: Int, $endCursor: String, $states: [IssueState!] = OPEN, $labels: [String!], $assignee: String, $author: String, $mention: String, $milestone: String) { repository(owner: $owner, name: $repo) { hasIssuesEnabled - issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mentioned, milestone: $milestone}) { + issues(first: $limit, after: $endCursor, orderBy: {field: CREATED_AT, direction: DESC}, states: $states, labels: $labels, filterBy: {assignee: $assignee, createdBy: $author, mentioned: $mention, milestone: $milestone}) { totalCount nodes { ...issue @@ -243,8 +243,8 @@ func IssueList(client *Client, repo ghrepo.Interface, state string, labels []str if authorString != "" { variables["author"] = authorString } - if mentionedString != "" { - variables["mentioned"] = mentionedString + if mentionString != "" { + variables["mention"] = mentionString } if milestoneString != "" { variables["milestone"] = milestoneString diff --git a/command/issue.go b/command/issue.go index 1b4befd67..9dd031ee5 100644 --- a/command/issue.go +++ b/command/issue.go @@ -41,7 +41,7 @@ func init() { issueListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|all}") issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch") issueListCmd.Flags().StringP("author", "A", "", "Filter by author") - issueListCmd.Flags().StringP("mentioned", "", "", "Filter by mention") + issueListCmd.Flags().StringP("mention", "", "", "Filter by mention") issueListCmd.Flags().StringP("milestone", "", "", "Filter by milestone") issueCmd.AddCommand(issueViewCmd) @@ -143,7 +143,7 @@ func issueList(cmd *cobra.Command, args []string) error { return err } - mentioned, err := cmd.Flags().GetString("mentioned") + mention, err := cmd.Flags().GetString("mention") if err != nil { return err } @@ -153,7 +153,7 @@ func issueList(cmd *cobra.Command, args []string) error { return err } - listResult, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit, author, mentioned, milestone) + listResult, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit, author, mention, milestone) if err != nil { return err } diff --git a/command/issue_test.go b/command/issue_test.go index 19ac94d01..b3cb1e5a4 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -153,7 +153,7 @@ func TestIssueList_withFlags(t *testing.T) { } } } `)) - output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mentioned me --milestone 1.x") + output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo --mention me --milestone 1.x") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -171,7 +171,7 @@ No issues match your search in OWNER/REPO Labels []string States []string Author string - Mentioned string + Mention string Milestone string } }{} @@ -181,7 +181,7 @@ No issues match your search in OWNER/REPO eq(t, reqBody.Variables.Labels, []string{"web", "bug"}) eq(t, reqBody.Variables.States, []string{"OPEN"}) eq(t, reqBody.Variables.Author, "foo") - eq(t, reqBody.Variables.Mentioned, "me") + eq(t, reqBody.Variables.Mention, "me") eq(t, reqBody.Variables.Milestone, "1.x") } From 7907def88081f519bf3266ca975e80e5d519e434 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 16 Jun 2020 18:16:49 +0200 Subject: [PATCH 10/35] api command: add support for REST pagination --- pkg/cmd/api/api.go | 52 ++++++++++++++++++++-- pkg/cmd/api/api_test.go | 98 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index a5540cf2d..dff60245c 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -3,6 +3,7 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -32,6 +33,7 @@ type ApiOptions struct { RawFields []string RequestHeaders []string ShowResponseHeaders bool + Paginate bool HttpClient func() (*http.Client, error) BaseRepo func() (ghrepo.Interface, error) @@ -93,6 +95,13 @@ Pass "-" to read from standard input. In this mode, parameters specified via opts.RequestPath = args[0] opts.RequestMethodPassed = c.Flags().Changed("method") + if opts.Paginate && !strings.EqualFold(opts.RequestMethod, "GET") && opts.RequestPath != "graphql" { + return &cmdutil.FlagError{Err: errors.New(`the '--paginate' option is not supported for non-GET requests`)} + } + if opts.Paginate && opts.RequestInputFile != "" { + return &cmdutil.FlagError{Err: errors.New(`the '--paginate' option is not supported with '--input'`)} + } + if runF != nil { return runF(&opts) } @@ -105,6 +114,7 @@ Pass "-" to read from standard input. In this mode, parameters specified via cmd.Flags().StringArrayVarP(&opts.RawFields, "raw-field", "f", nil, "Add a string parameter") cmd.Flags().StringArrayVarP(&opts.RequestHeaders, "header", "H", nil, "Add an additional HTTP request header") cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response headers in the output") + cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The file to use as body for the HTTP request") return cmd } @@ -145,11 +155,46 @@ func apiRun(opts *ApiOptions) error { return err } - resp, err := httpRequest(httpClient, method, requestPath, requestBody, requestHeaders) - if err != nil { - return err + hasNextPage := true + for hasNextPage { + resp, err := httpRequest(httpClient, method, requestPath, requestBody, requestHeaders) + if err != nil { + return err + } + + err = processResponse(resp, opts) + if err != nil { + return err + } + + if !opts.Paginate { + break + } + requestPath, hasNextPage = findNextPage(resp) + + if hasNextPage && opts.ShowResponseHeaders { + fmt.Fprint(opts.IO.Out, "\n") + } } + return nil +} + +var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) + +func findNextPage(resp *http.Response) (string, bool) { + for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { + if len(m) < 2 { + continue + } + if m[2] == "next" { + return m[1], true + } + } + return "", false +} + +func processResponse(resp *http.Response, opts *ApiOptions) error { if opts.ShowResponseHeaders { fmt.Fprintln(opts.IO.Out, resp.Proto, resp.Status) printHeaders(opts.IO.Out, resp.Header, opts.IO.ColorEnabled()) @@ -164,6 +209,7 @@ func apiRun(opts *ApiOptions) error { isJSON, _ := regexp.MatchString(`[/+]json(;|$)`, resp.Header.Get("Content-Type")) + var err error var serverError string if isJSON && (opts.RequestPath == "graphql" || resp.StatusCode >= 400) { responseBody, serverError, err = parseErrorResponse(responseBody, resp.StatusCode) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 605e4bf9e..c83cf3b5a 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -36,6 +36,7 @@ func Test_NewCmdApi(t *testing.T) { MagicFields: []string(nil), RequestHeaders: []string(nil), ShowResponseHeaders: false, + Paginate: false, }, wantsErr: false, }, @@ -51,6 +52,7 @@ func Test_NewCmdApi(t *testing.T) { MagicFields: []string(nil), RequestHeaders: []string(nil), ShowResponseHeaders: false, + Paginate: false, }, wantsErr: false, }, @@ -66,6 +68,7 @@ func Test_NewCmdApi(t *testing.T) { MagicFields: []string{"body=@file.txt"}, RequestHeaders: []string(nil), ShowResponseHeaders: false, + Paginate: false, }, wantsErr: false, }, @@ -81,9 +84,52 @@ func Test_NewCmdApi(t *testing.T) { MagicFields: []string(nil), RequestHeaders: []string{"accept: text/plain"}, ShowResponseHeaders: true, + Paginate: false, }, wantsErr: false, }, + { + name: "with pagination", + cli: "repos/OWNER/REPO/issues --paginate", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "repos/OWNER/REPO/issues", + RequestInputFile: "", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + Paginate: true, + }, + wantsErr: false, + }, + { + name: "POST pagination", + cli: "-XPOST repos/OWNER/REPO/issues --paginate", + wantsErr: true, + }, + { + name: "GraphQL pagination", + cli: "-XPOST graphql --paginate", + wants: ApiOptions{ + RequestMethod: "POST", + RequestMethodPassed: true, + RequestPath: "graphql", + RequestInputFile: "", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + Paginate: true, + }, + wantsErr: false, + }, + { + name: "input pagination", + cli: "--input repos/OWNER/REPO/issues --paginate", + wantsErr: true, + }, { name: "with request body from file", cli: "user --input myfile", @@ -96,6 +142,7 @@ func Test_NewCmdApi(t *testing.T) { MagicFields: []string(nil), RequestHeaders: []string(nil), ShowResponseHeaders: false, + Paginate: false, }, wantsErr: false, }, @@ -246,6 +293,57 @@ func Test_apiRun(t *testing.T) { } } +func Test_apiRun_pagination(t *testing.T) { + io, _, stdout, stderr := iostreams.Test() + + requestCount := 0 + responses := []*http.Response{ + { + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"page":1}`)), + Header: http.Header{ + "Link": []string{`; rel="next", ; rel="last"`}, + }, + }, + { + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"page":2}`)), + Header: http.Header{ + "Link": []string{`; rel="next", ; rel="last"`}, + }, + }, + { + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"page":3}`)), + Header: http.Header{}, + }, + } + + options := ApiOptions{ + IO: io, + HttpClient: func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + resp := responses[requestCount] + resp.Request = req + requestCount++ + return resp, nil + } + return &http.Client{Transport: tr}, nil + }, + + Paginate: true, + } + + err := apiRun(&options) + assert.NoError(t, err) + + assert.Equal(t, `{"page":1}{"page":2}{"page":3}`, stdout.String(), "stdout") + assert.Equal(t, "", stderr.String(), "stderr") + + assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=2", responses[1].Request.URL.String()) + assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) +} + func Test_apiRun_inputFile(t *testing.T) { tests := []struct { name string From 3f940c98f1872d439109b4363139fe59b6097ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 16 Jun 2020 18:19:39 +0200 Subject: [PATCH 11/35] Add assertion for 1st api request before pagination --- pkg/cmd/api/api_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index c83cf3b5a..9debf7896 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -331,7 +331,8 @@ func Test_apiRun_pagination(t *testing.T) { return &http.Client{Transport: tr}, nil }, - Paginate: true, + RequestPath: "issues", + Paginate: true, } err := apiRun(&options) @@ -340,6 +341,7 @@ func Test_apiRun_pagination(t *testing.T) { assert.Equal(t, `{"page":1}{"page":2}{"page":3}`, stdout.String(), "stdout") assert.Equal(t, "", stderr.String(), "stderr") + assert.Equal(t, "https://api.github.com/issues", responses[0].Request.URL.String()) assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=2", responses[1].Request.URL.String()) assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) } From f4ecd365a69d3491e82d52eb16b10aee66d75166 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 17 Jun 2020 18:02:20 +0200 Subject: [PATCH 12/35] api command: add GraphQL support for `--paginate` --- pkg/cmd/api/api.go | 76 +++++++++++++-------- pkg/cmd/api/api_test.go | 81 +++++++++++++++++++++- pkg/cmd/api/pagination.go | 87 ++++++++++++++++++++++++ pkg/cmd/api/pagination_test.go | 118 +++++++++++++++++++++++++++++++++ 4 files changed, 335 insertions(+), 27 deletions(-) create mode 100644 pkg/cmd/api/pagination.go create mode 100644 pkg/cmd/api/pagination_test.go diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index dff60245c..067ef6bc8 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -76,7 +76,11 @@ on the format of the value: Raw request body may be passed from the outside via a file specified by '--input'. Pass "-" to read from standard input. In this mode, parameters specified via '--field' flags are serialized into URL query parameters. -`, + +In '--paginate' mode, all pages of results will sequentially be requested until +there are no more pages of results. For GraphQL requests, this requires that the +original query accepts an '$endCursor: String' variable and that it fetches the +'pageInfo{ hasNextPage, endCursor }' set of fields from a collection.`, Example: heredoc.Doc(` $ gh api repos/:owner/:repo/releases @@ -89,6 +93,20 @@ Pass "-" to read from standard input. In this mode, parameters specified via } } ' + + $ gh api graphql --paginate -f query=' + query($endCursor: String) { + viewer { + repositories(first: 100, after: $endCursor) { + nodes { nameWithOwner } + pageInfo { + hasNextPage + endCursor + } + } + } + } + ' `), Args: cobra.ExactArgs(1), RunE: func(c *cobra.Command, args []string) error { @@ -162,7 +180,7 @@ func apiRun(opts *ApiOptions) error { return err } - err = processResponse(resp, opts) + endCursor, err := processResponse(resp, opts) if err != nil { return err } @@ -170,7 +188,15 @@ func apiRun(opts *ApiOptions) error { if !opts.Paginate { break } - requestPath, hasNextPage = findNextPage(resp) + + if opts.RequestPath == "graphql" { + hasNextPage = endCursor != "" + if hasNextPage { + params["endCursor"] = endCursor + } + } else { + requestPath, hasNextPage = findNextPage(resp) + } if hasNextPage && opts.ShowResponseHeaders { fmt.Fprint(opts.IO.Out, "\n") @@ -180,21 +206,7 @@ func apiRun(opts *ApiOptions) error { return nil } -var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) - -func findNextPage(resp *http.Response) (string, bool) { - for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { - if len(m) < 2 { - continue - } - if m[2] == "next" { - return m[1], true - } - } - return "", false -} - -func processResponse(resp *http.Response, opts *ApiOptions) error { +func processResponse(resp *http.Response, opts *ApiOptions) (endCursor string, err error) { if opts.ShowResponseHeaders { fmt.Fprintln(opts.IO.Out, resp.Proto, resp.Status) printHeaders(opts.IO.Out, resp.Header, opts.IO.ColorEnabled()) @@ -202,43 +214,55 @@ func processResponse(resp *http.Response, opts *ApiOptions) error { } if resp.StatusCode == 204 { - return nil + return } var responseBody io.Reader = resp.Body defer resp.Body.Close() isJSON, _ := regexp.MatchString(`[/+]json(;|$)`, resp.Header.Get("Content-Type")) - var err error var serverError string if isJSON && (opts.RequestPath == "graphql" || resp.StatusCode >= 400) { responseBody, serverError, err = parseErrorResponse(responseBody, resp.StatusCode) if err != nil { - return err + return } } + var bodyCopy *bytes.Buffer + isGraphQLPaginate := isJSON && resp.StatusCode == 200 && opts.Paginate && opts.RequestPath == "graphql" + if isGraphQLPaginate { + bodyCopy = &bytes.Buffer{} + responseBody = io.TeeReader(responseBody, bodyCopy) + } + if isJSON && opts.IO.ColorEnabled() { err = jsoncolor.Write(opts.IO.Out, responseBody, " ") if err != nil { - return err + return } } else { _, err = io.Copy(opts.IO.Out, responseBody) if err != nil { - return err + return } } if serverError != "" { fmt.Fprintf(opts.IO.ErrOut, "gh: %s\n", serverError) - return cmdutil.SilentError + err = cmdutil.SilentError + return } else if resp.StatusCode > 299 { fmt.Fprintf(opts.IO.ErrOut, "gh: HTTP %d\n", resp.StatusCode) - return cmdutil.SilentError + err = cmdutil.SilentError + return } - return nil + if isGraphQLPaginate { + endCursor = findEndCursor(bodyCopy) + } + + return } var placeholderRE = regexp.MustCompile(`\:(owner|repo)\b`) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 9debf7896..dc7a44e41 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -2,6 +2,7 @@ package api import ( "bytes" + "encoding/json" "fmt" "io/ioutil" "net/http" @@ -13,6 +14,7 @@ import ( "github.com/cli/cli/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NewCmdApi(t *testing.T) { @@ -293,7 +295,7 @@ func Test_apiRun(t *testing.T) { } } -func Test_apiRun_pagination(t *testing.T) { +func Test_apiRun_paginationREST(t *testing.T) { io, _, stdout, stderr := iostreams.Test() requestCount := 0 @@ -346,6 +348,83 @@ func Test_apiRun_pagination(t *testing.T) { assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) } +func Test_apiRun_paginationGraphQL(t *testing.T) { + io, _, stdout, stderr := iostreams.Test() + + requestCount := 0 + responses := []*http.Response{ + { + StatusCode: 200, + Header: http.Header{"Content-Type": []string{`application/json`}}, + Body: ioutil.NopCloser(bytes.NewBufferString(`{ + "data": { + "nodes": ["page one"], + "pageInfo": { + "endCursor": "PAGE1_END", + "hasNextPage": true + } + } + }`)), + }, + { + StatusCode: 200, + Header: http.Header{"Content-Type": []string{`application/json`}}, + Body: ioutil.NopCloser(bytes.NewBufferString(`{ + "data": { + "nodes": ["page two"], + "pageInfo": { + "endCursor": "PAGE2_END", + "hasNextPage": false + } + } + }`)), + }, + } + + options := ApiOptions{ + IO: io, + HttpClient: func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + resp := responses[requestCount] + resp.Request = req + requestCount++ + return resp, nil + } + return &http.Client{Transport: tr}, nil + }, + + RequestMethod: "POST", + RequestPath: "graphql", + Paginate: true, + } + + err := apiRun(&options) + require.NoError(t, err) + + assert.Contains(t, stdout.String(), `"page one"`) + assert.Contains(t, stdout.String(), `"page two"`) + assert.Equal(t, "", stderr.String(), "stderr") + + var requestData struct { + Variables map[string]interface{} + } + + bb, err := ioutil.ReadAll(responses[0].Request.Body) + require.NoError(t, err) + err = json.Unmarshal(bb, &requestData) + require.NoError(t, err) + _, hasCursor := requestData.Variables["endCursor"].(string) + assert.Equal(t, false, hasCursor) + + bb, err = ioutil.ReadAll(responses[1].Request.Body) + require.NoError(t, err) + err = json.Unmarshal(bb, &requestData) + require.NoError(t, err) + endCursor, hasCursor := requestData.Variables["endCursor"].(string) + assert.Equal(t, true, hasCursor) + assert.Equal(t, "PAGE1_END", endCursor) +} + func Test_apiRun_inputFile(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/api/pagination.go b/pkg/cmd/api/pagination.go new file mode 100644 index 000000000..3bd00b822 --- /dev/null +++ b/pkg/cmd/api/pagination.go @@ -0,0 +1,87 @@ +package api + +import ( + "encoding/json" + "io" + "net/http" + "regexp" +) + +var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) + +func findNextPage(resp *http.Response) (string, bool) { + for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { + if len(m) >= 2 && m[2] == "next" { + return m[1], true + } + } + return "", false +} + +func findEndCursor(r io.Reader) string { + dec := json.NewDecoder(r) + + var idx int + var stack []json.Delim + var lastKey string + var contextKey string + + var endCursor string + var hasNextPage bool + var foundEndCursor bool + var foundNextPage bool + +loop: + for { + t, err := dec.Token() + if err == io.EOF { + break + } + if err != nil { + return "" + } + + switch tt := t.(type) { + case json.Delim: + switch tt { + case '{', '[': + stack = append(stack, tt) + contextKey = lastKey + idx = 0 + case '}', ']': + stack = stack[:len(stack)-1] + contextKey = "" + idx = 0 + } + default: + isKey := len(stack) > 0 && stack[len(stack)-1] == '{' && idx%2 == 0 + idx++ + + switch tt := t.(type) { + case string: + if isKey { + lastKey = tt + } else if contextKey == "pageInfo" && lastKey == "endCursor" { + endCursor = tt + foundEndCursor = true + if foundNextPage { + break loop + } + } + case bool: + if contextKey == "pageInfo" && lastKey == "hasNextPage" { + hasNextPage = tt + foundNextPage = true + if foundEndCursor { + break loop + } + } + } + } + } + + if hasNextPage { + return endCursor + } + return "" +} diff --git a/pkg/cmd/api/pagination_test.go b/pkg/cmd/api/pagination_test.go new file mode 100644 index 000000000..0ed4566da --- /dev/null +++ b/pkg/cmd/api/pagination_test.go @@ -0,0 +1,118 @@ +package api + +import ( + "bytes" + "io" + "net/http" + "testing" +) + +func Test_findNextPage(t *testing.T) { + tests := []struct { + name string + resp *http.Response + want string + want1 bool + }{ + { + name: "no Link header", + resp: &http.Response{}, + want: "", + want1: false, + }, + { + name: "no next page in Link", + resp: &http.Response{ + Header: http.Header{ + "Link": []string{`; rel="last"`}, + }, + }, + want: "", + want1: false, + }, + { + name: "has next page", + resp: &http.Response{ + Header: http.Header{ + "Link": []string{`; rel="next", ; rel="last"`}, + }, + }, + want: "https://api.github.com/issues?page=2", + want1: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, got1 := findNextPage(tt.resp) + if got != tt.want { + t.Errorf("findNextPage() got = %v, want %v", got, tt.want) + } + if got1 != tt.want1 { + t.Errorf("findNextPage() got1 = %v, want %v", got1, tt.want1) + } + }) + } +} + +func Test_findEndCursor(t *testing.T) { + tests := []struct { + name string + json io.Reader + want string + }{ + { + name: "blank", + json: bytes.NewBufferString(`{}`), + want: "", + }, + { + name: "unrelated fields", + json: bytes.NewBufferString(`{ + "hasNextPage": true, + "endCursor": "THE_END" + }`), + want: "", + }, + { + name: "has next page", + json: bytes.NewBufferString(`{ + "pageInfo": { + "hasNextPage": true, + "endCursor": "THE_END" + } + }`), + want: "THE_END", + }, + { + name: "more pageInfo blocks", + json: bytes.NewBufferString(`{ + "pageInfo": { + "hasNextPage": true, + "endCursor": "THE_END" + }, + "pageInfo": { + "hasNextPage": true, + "endCursor": "NOT_THIS" + } + }`), + want: "THE_END", + }, + { + name: "no next page", + json: bytes.NewBufferString(`{ + "pageInfo": { + "hasNextPage": false, + "endCursor": "THE_END" + } + }`), + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := findEndCursor(tt.json); got != tt.want { + t.Errorf("findEndCursor() = %v, want %v", got, tt.want) + } + }) + } +} From 55be0d2a9ca7282c2c25e13d3bd3c67bc27347b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 22 Jun 2020 19:13:28 +0200 Subject: [PATCH 13/35] Tweak `isColorEnabled` --- utils/color.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/utils/color.go b/utils/color.go index e88549667..447765273 100644 --- a/utils/color.go +++ b/utils/color.go @@ -10,8 +10,8 @@ import ( ) var ( - _isColorEnabled bool = true - _isStdoutTerminal, checkedTerminal, checkedNoColor bool + _isColorEnabled bool = true + _isStdoutTerminal, checkedTerminal bool // Outputs ANSI color if stdout is a tty Magenta = makeColorFunc("magenta") @@ -45,7 +45,7 @@ func NewColorable(f *os.File) io.Writer { func makeColorFunc(color string) func(string) string { cf := ansi.ColorFunc(color) return func(arg string) string { - if isColorEnabled() && isStdoutTerminal() { + if isColorEnabled() { return cf(arg) } return arg @@ -53,13 +53,9 @@ func makeColorFunc(color string) func(string) string { } func isColorEnabled() bool { - if !isStdoutTerminal() { + if os.Getenv("NO_COLOR") != "" { return false } - if !checkedNoColor { - _isColorEnabled = os.Getenv("NO_COLOR") == "" - checkedNoColor = true - } - return _isColorEnabled + return isStdoutTerminal() } From 04b18ea8cb2933d096f5e6b38c559538c4e26fde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 22 Jun 2020 19:32:06 +0200 Subject: [PATCH 14/35] :fire: unused var --- utils/color.go | 1 - 1 file changed, 1 deletion(-) diff --git a/utils/color.go b/utils/color.go index 447765273..dd9a7d11c 100644 --- a/utils/color.go +++ b/utils/color.go @@ -10,7 +10,6 @@ import ( ) var ( - _isColorEnabled bool = true _isStdoutTerminal, checkedTerminal bool // Outputs ANSI color if stdout is a tty From e084a4563fb8aa0d294c0db9c360aacbdf52bef4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 22 Jun 2020 19:36:55 +0200 Subject: [PATCH 15/35] Fix `pr checkout OWNER:BRANCH` when maintainer can modify We did not use to request the necessary GraphQL fields to determine whether the PR in question can be modified by maintainers (i.e. pushed back to). --- api/queries_pr.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index 188e708a8..00d01661a 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -540,8 +540,12 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea headRepositoryOwner { login } + headRepository { + name + } isCrossRepository isDraft + maintainerCanModify reviewRequests(first: 100) { nodes { requestedReviewer { From ac7d5ecc44a7821265b5cc8a00a9eace6428da56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 22 Jun 2020 19:44:16 +0200 Subject: [PATCH 16/35] Ensure markdown still passed through Glamour in no-color mode --- utils/utils.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/utils/utils.go b/utils/utils.go index 9441bc2bc..c84860d9e 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -23,10 +23,6 @@ func OpenInBrowser(url string) error { func RenderMarkdown(text string) (string, error) { style := "notty" - if !isColorEnabled() { - return text, nil - } - if isColorEnabled() { style = "dark" } From 8a4872bab38611904db7a58244c40c92c76cc8e0 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 22 Jun 2020 14:07:49 -0400 Subject: [PATCH 17/35] Remove global repo flag --- command/root.go | 1 - 1 file changed, 1 deletion(-) diff --git a/command/root.go b/command/root.go index 693603e05..3f83ba278 100644 --- a/command/root.go +++ b/command/root.go @@ -52,7 +52,6 @@ func init() { RootCmd.AddCommand(versionCmd) RootCmd.SetVersionTemplate(versionOutput) - RootCmd.PersistentFlags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") RootCmd.PersistentFlags().Bool("help", false, "Show help for command") RootCmd.Flags().Bool("version", false, "Show gh version") // TODO: From dd8cbccbd54ed5b1c94560ab0d917f853ae716c9 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 22 Jun 2020 14:26:41 -0400 Subject: [PATCH 18/35] Add repo flag to issue commands --- command/issue.go | 6 ++++++ command/util.go | 8 ++++++++ 2 files changed, 14 insertions(+) create mode 100644 command/util.go diff --git a/command/issue.go b/command/issue.go index 372ad8bd1..ca9087dbb 100644 --- a/command/issue.go +++ b/command/issue.go @@ -24,8 +24,10 @@ import ( func init() { RootCmd.AddCommand(issueCmd) issueCmd.AddCommand(issueStatusCmd) + includeRepoFlag(issueStatusCmd) issueCmd.AddCommand(issueCreateCmd) + includeRepoFlag(issueCreateCmd) issueCreateCmd.Flags().StringP("title", "t", "", "Supply a title. Will prompt for one otherwise.") issueCreateCmd.Flags().StringP("body", "b", "", @@ -37,6 +39,7 @@ func init() { issueCreateCmd.Flags().StringP("milestone", "m", "", "Add the issue to a milestone by `name`") issueCmd.AddCommand(issueListCmd) + includeRepoFlag(issueListCmd) issueListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") issueListCmd.Flags().StringSliceP("label", "l", nil, "Filter by label") issueListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|all}") @@ -44,10 +47,13 @@ func init() { issueListCmd.Flags().StringP("author", "A", "", "Filter by author") issueCmd.AddCommand(issueViewCmd) + includeRepoFlag(issueViewCmd) issueViewCmd.Flags().BoolP("web", "w", false, "Open an issue in the browser") issueCmd.AddCommand(issueCloseCmd) + includeRepoFlag(issueCloseCmd) issueCmd.AddCommand(issueReopenCmd) + includeRepoFlag(issueReopenCmd) } var issueCmd = &cobra.Command{ diff --git a/command/util.go b/command/util.go new file mode 100644 index 000000000..a7d4a0f31 --- /dev/null +++ b/command/util.go @@ -0,0 +1,8 @@ +package Command + +import "github.com/spf13/cobra" +package Command + +func includeRepoFlag(cmd *cobra.Command) { + cmd.StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") +} From 6a02ad3f1b83cab385d36cdc0e59ef436d38cd1e Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 22 Jun 2020 15:09:21 -0400 Subject: [PATCH 19/35] Add repoflag to pr commands --- command/pr.go | 7 +++++++ command/pr_diff.go | 1 + command/pr_review.go | 1 + 3 files changed, 9 insertions(+) diff --git a/command/pr.go b/command/pr.go index 18150d4eb..0aa7d25b1 100644 --- a/command/pr.go +++ b/command/pr.go @@ -27,16 +27,22 @@ func init() { prCmd.AddCommand(prCheckoutCmd) prCmd.AddCommand(prCreateCmd) prCmd.AddCommand(prStatusCmd) + includeRepoFlag(prStatusCmd) prCmd.AddCommand(prCloseCmd) + includeRepoFlag(prCloseCmd) prCmd.AddCommand(prReopenCmd) + includeRepoFlag(prReopenCmd) prCmd.AddCommand(prMergeCmd) + includeRepoFlag(prMergeCmd) prMergeCmd.Flags().BoolP("delete-branch", "d", true, "Delete the local and remote branch after merge") prMergeCmd.Flags().BoolP("merge", "m", false, "Merge the commits with the base branch") prMergeCmd.Flags().BoolP("rebase", "r", false, "Rebase the commits onto the base branch") prMergeCmd.Flags().BoolP("squash", "s", false, "Squash the commits into one commit and merge it into the base branch") prCmd.AddCommand(prReadyCmd) + includeRepoFlag(prReadyCmd) prCmd.AddCommand(prListCmd) + includeRepoFlag(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") prListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|merged|all}") prListCmd.Flags().StringP("base", "B", "", "Filter by base branch") @@ -44,6 +50,7 @@ func init() { prListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") prCmd.AddCommand(prViewCmd) + includeRepoFlag(prViewCmd) prViewCmd.Flags().BoolP("web", "w", false, "Open a pull request in the browser") } diff --git a/command/pr_diff.go b/command/pr_diff.go index ae50a2618..0bc9c8613 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -19,6 +19,7 @@ func init() { prDiffCmd.Flags().StringP("color", "c", "auto", "Whether or not to output color: {always|never|auto}") prCmd.AddCommand(prDiffCmd) + includeRepoFlag(prDiffCmd) } func prDiff(cmd *cobra.Command, args []string) error { diff --git a/command/pr_review.go b/command/pr_review.go index 08efc638d..77723ab44 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -15,6 +15,7 @@ import ( func init() { prCmd.AddCommand(prReviewCmd) + includeRepoFlag(prCmd) prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request") prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request") From 2761739c29c7f12fb9a1f21e7f5c5bb8779c212f Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 22 Jun 2020 15:19:33 -0400 Subject: [PATCH 20/35] Correct package name --- command/util.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/command/util.go b/command/util.go index a7d4a0f31..130f97844 100644 --- a/command/util.go +++ b/command/util.go @@ -1,8 +1,7 @@ -package Command +package command import "github.com/spf13/cobra" -package Command func includeRepoFlag(cmd *cobra.Command) { - cmd.StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") + cmd.Flags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") } From 625b673b587095df7d985a06e5e5883559215687 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 22 Jun 2020 15:30:22 -0400 Subject: [PATCH 21/35] Ignore repo flag errors in determineBaseRepo --- command/root.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/command/root.go b/command/root.go index 3f83ba278..83dfbd3f0 100644 --- a/command/root.go +++ b/command/root.go @@ -303,8 +303,8 @@ func changelogURL(version string) string { } func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Context) (ghrepo.Interface, error) { - repo, err := cmd.Flags().GetString("repo") - if err == nil && repo != "" { + repo, _ := cmd.Flags().GetString("repo") + if repo != "" { baseRepo, err := ghrepo.FromFullName(repo) if err != nil { return nil, fmt.Errorf("argument error: %w", err) @@ -312,17 +312,12 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return baseRepo, nil } - baseOverride, err := cmd.Flags().GetString("repo") - if err != nil { - return nil, err - } - remotes, err := ctx.Remotes() if err != nil { return nil, err } - repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, baseOverride) + repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, repo) if err != nil { return nil, err } From c945fb4336ff6d6229bed30eb68d946deb110320 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Jun 2020 18:42:57 +0200 Subject: [PATCH 22/35] Automatically add `per_page=100` to paginated REST requests Most endpoints respect this parameter by default. Those that don't will just ignore it. The `per_page=100` parameter is not added if there is already a `per_page` parameter specified in the request. --- pkg/cmd/api/api.go | 7 ++++- pkg/cmd/api/api_test.go | 2 +- pkg/cmd/api/pagination.go | 21 ++++++++++++++ pkg/cmd/api/pagination_test.go | 51 ++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 067ef6bc8..e1edabbc0 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -143,6 +143,7 @@ func apiRun(opts *ApiOptions) error { return err } + isGraphQL := opts.RequestPath == "graphql" requestPath, err := fillPlaceholders(opts.RequestPath, opts) if err != nil { return fmt.Errorf("unable to expand placeholder in path: %w", err) @@ -155,6 +156,10 @@ func apiRun(opts *ApiOptions) error { method = "POST" } + if opts.Paginate && !isGraphQL { + requestPath = addPerPage(requestPath, 100, params) + } + if opts.RequestInputFile != "" { file, size, err := openUserFile(opts.RequestInputFile, opts.IO.In) if err != nil { @@ -189,7 +194,7 @@ func apiRun(opts *ApiOptions) error { break } - if opts.RequestPath == "graphql" { + if isGraphQL { hasNextPage = endCursor != "" if hasNextPage { params["endCursor"] = endCursor diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index dc7a44e41..7b89d715c 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -343,7 +343,7 @@ func Test_apiRun_paginationREST(t *testing.T) { assert.Equal(t, `{"page":1}{"page":2}{"page":3}`, stdout.String(), "stdout") assert.Equal(t, "", stderr.String(), "stderr") - assert.Equal(t, "https://api.github.com/issues", responses[0].Request.URL.String()) + assert.Equal(t, "https://api.github.com/issues?per_page=100", responses[0].Request.URL.String()) assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=2", responses[1].Request.URL.String()) assert.Equal(t, "https://api.github.com/repositories/1227/issues?page=3", responses[2].Request.URL.String()) } diff --git a/pkg/cmd/api/pagination.go b/pkg/cmd/api/pagination.go index 3bd00b822..fce88fb92 100644 --- a/pkg/cmd/api/pagination.go +++ b/pkg/cmd/api/pagination.go @@ -2,9 +2,12 @@ package api import ( "encoding/json" + "fmt" "io" "net/http" + "net/url" "regexp" + "strings" ) var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) @@ -85,3 +88,21 @@ loop: } return "" } + +func addPerPage(p string, perPage int, params map[string]interface{}) string { + if _, hasPerPage := params["per_page"]; hasPerPage { + return p + } + + idx := strings.IndexRune(p, '?') + sep := "?" + + if idx >= 0 { + if qp, err := url.ParseQuery(p[idx+1:]); err == nil && qp.Get("per_page") != "" { + return p + } + sep = "&" + } + + return fmt.Sprintf("%s%sper_page=%d", p, sep, perPage) +} diff --git a/pkg/cmd/api/pagination_test.go b/pkg/cmd/api/pagination_test.go index 0ed4566da..3bb1a8ec5 100644 --- a/pkg/cmd/api/pagination_test.go +++ b/pkg/cmd/api/pagination_test.go @@ -116,3 +116,54 @@ func Test_findEndCursor(t *testing.T) { }) } } + +func Test_addPerPage(t *testing.T) { + type args struct { + p string + perPage int + params map[string]interface{} + } + tests := []struct { + name string + args args + want string + }{ + { + name: "adds per_page", + args: args{ + p: "items", + perPage: 13, + params: nil, + }, + want: "items?per_page=13", + }, + { + name: "avoids adding per_page if already in params", + args: args{ + p: "items", + perPage: 13, + params: map[string]interface{}{ + "state": "open", + "per_page": 99, + }, + }, + want: "items", + }, + { + name: "avoids adding per_page if already in query", + args: args{ + p: "items?per_page=6&state=open", + perPage: 13, + params: nil, + }, + want: "items?per_page=6&state=open", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := addPerPage(tt.args.p, tt.args.perPage, tt.args.params); got != tt.want { + t.Errorf("addPerPage() = %v, want %v", got, tt.want) + } + }) + } +} From aa8f8e8904d109e32a902c871bb75e53f03bf752 Mon Sep 17 00:00:00 2001 From: Pavel Borzenkov Date: Sat, 27 Jun 2020 18:47:06 +0300 Subject: [PATCH 23/35] httpmock: propagate original HTTP request to HTTP response So that it's possible to access it in mocked HTTP tests. Signed-off-by: Pavel Borzenkov --- pkg/httpmock/legacy.go | 8 ++++---- pkg/httpmock/stub.go | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/httpmock/legacy.go b/pkg/httpmock/legacy.go index 9474c3dd8..9402c21c7 100644 --- a/pkg/httpmock/legacy.go +++ b/pkg/httpmock/legacy.go @@ -12,19 +12,19 @@ import ( // TODO: clean up methods in this file when there are no more callers func (r *Registry) StubResponse(status int, body io.Reader) { - r.Register(MatchAny, func(*http.Request) (*http.Response, error) { - return httpResponse(status, body), nil + r.Register(MatchAny, func(req *http.Request) (*http.Response, error) { + return httpResponse(status, req, body), nil }) } func (r *Registry) StubWithFixture(status int, fixtureFileName string) func() { fixturePath := path.Join("../test/fixtures/", fixtureFileName) fixtureFile, err := os.Open(fixturePath) - r.Register(MatchAny, func(*http.Request) (*http.Response, error) { + r.Register(MatchAny, func(req *http.Request) (*http.Response, error) { if err != nil { return nil, err } - return httpResponse(200, fixtureFile), nil + return httpResponse(200, req, fixtureFile), nil }) return func() { if err == nil { diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 48077ed4e..cc42dfd6c 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -59,15 +59,15 @@ func decodeJSONBody(req *http.Request, dest interface{}) error { } func StringResponse(body string) Responder { - return func(*http.Request) (*http.Response, error) { - return httpResponse(200, bytes.NewBufferString(body)), nil + return func(req *http.Request) (*http.Response, error) { + return httpResponse(200, req, bytes.NewBufferString(body)), nil } } func JSONResponse(body interface{}) Responder { - return func(*http.Request) (*http.Response, error) { + return func(req *http.Request) (*http.Response, error) { b, _ := json.Marshal(body) - return httpResponse(200, bytes.NewBuffer(b)), nil + return httpResponse(200, req, bytes.NewBuffer(b)), nil } } @@ -84,7 +84,7 @@ func GraphQLMutation(body string, cb func(map[string]interface{})) Responder { } cb(bodyData.Variables.Input) - return httpResponse(200, bytes.NewBufferString(body)), nil + return httpResponse(200, req, bytes.NewBufferString(body)), nil } } @@ -100,13 +100,14 @@ func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responde } cb(bodyData.Query, bodyData.Variables) - return httpResponse(200, bytes.NewBufferString(body)), nil + return httpResponse(200, req, bytes.NewBufferString(body)), nil } } -func httpResponse(status int, body io.Reader) *http.Response { +func httpResponse(status int, req *http.Request, body io.Reader) *http.Response { return &http.Response{ StatusCode: status, + Request: req, Body: ioutil.NopCloser(body), } } From c66eebc6fb248b77ec8e960ededc19c9595414f9 Mon Sep 17 00:00:00 2001 From: Pavel Borzenkov Date: Sat, 27 Jun 2020 18:47:34 +0300 Subject: [PATCH 24/35] api: return structured error for failed API calls `fmt.Errorf` hides information and makes it hard to test for specific conditions in returned error. Return a structured error instead. Signed-off-by: Pavel Borzenkov --- api/client.go | 17 ++++++++++++++++- api/client_test.go | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/api/client.go b/api/client.go index 1c8c1eaa2..6880609bb 100644 --- a/api/client.go +++ b/api/client.go @@ -157,6 +157,17 @@ func (gr GraphQLErrorResponse) Error() string { return fmt.Sprintf("graphql error: '%s'", strings.Join(errorMessages, ", ")) } +// HTTPError is an error returned by a failed API call +type HTTPError struct { + Code int + URL string + Message string +} + +func (e HTTPError) Error() string { + return fmt.Sprintf("http error, '%s' failed (%d): '%s'", e.URL, e.Code, e.Message) +} + // Returns whether or not scopes are present, appID, and error func (c Client) HasScopes(wantedScopes ...string) (bool, string, error) { url := "https://api.github.com/user" @@ -298,7 +309,11 @@ func handleHTTPError(resp *http.Response) error { message = parsedBody.Message } - return fmt.Errorf("http error, '%s' failed (%d): '%s'", resp.Request.URL, resp.StatusCode, message) + return HTTPError{ + Code: resp.StatusCode, + URL: resp.Request.URL.String(), + Message: message, + } } var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`) diff --git a/api/client_test.go b/api/client_test.go index 4c81bf315..9c908be12 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -2,6 +2,7 @@ package api import ( "bytes" + "errors" "io/ioutil" "reflect" "testing" @@ -66,3 +67,16 @@ func TestRESTGetDelete(t *testing.T) { err := client.REST("DELETE", "applications/CLIENTID/grant", r, nil) eq(t, err, nil) } + +func TestRESTError(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + http.StubResponse(422, bytes.NewBufferString(`{"message": "OH NO"}`)) + + var httpErr HTTPError + err := client.REST("DELETE", "/repos/branch", nil, nil) + if err == nil || !errors.As(err, &httpErr) || httpErr.Code != 422 { + t.Fatalf("got %q", err.Error()) + } +} From 3afec6f90aea8707de7610ce6a539faaf766c6fa Mon Sep 17 00:00:00 2001 From: Pavel Borzenkov Date: Sat, 27 Jun 2020 19:03:25 +0300 Subject: [PATCH 25/35] api: gracefully handle already deleted remote refs If a GitHub repo is configured to automatically delete branches after PR is merged, `gh pr merge` fails with error like: failed to delete remote branch: ... (422): 'Reference does not exist' Gracefully handle such case and don't report the error. Signed-off-by: Pavel Borzenkov --- api/queries_pr.go | 11 ++++++++++- api/queries_pr_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 api/queries_pr_test.go diff --git a/api/queries_pr.go b/api/queries_pr.go index 00d01661a..b42a94eec 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1015,7 +1015,16 @@ func BranchDeleteRemote(client *Client, repo ghrepo.Interface, branch string) er NodeID string `json:"node_id"` } path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), branch) - return client.REST("DELETE", path, nil, &response) + err := client.REST("DELETE", path, nil, &response) + if err != nil { + var httpErr HTTPError + // The ref might have already been deleted by GitHub + if !errors.As(err, &httpErr) || httpErr.Code != 422 { + return err + } + } + + return nil } func min(a, b int) int { diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go new file mode 100644 index 000000000..3b8bf0a65 --- /dev/null +++ b/api/queries_pr_test.go @@ -0,0 +1,42 @@ +package api + +import ( + "bytes" + "testing" + + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/httpmock" +) + +func TestBranchDeleteRemote(t *testing.T) { + var tests = []struct { + name string + code int + body string + expectError bool + }{ + {name: "success", code: 204, body: "", expectError: false}, + {name: "error", code: 500, body: `{"message": "oh no"}`, expectError: true}, + { + name: "already_deleted", + code: 422, + body: `{"message": "Reference does not exist"}`, + expectError: false, + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + http.StubResponse(tc.code, bytes.NewBufferString(tc.body)) + repo, _ := ghrepo.FromFullName("OWNER/REPO") + err := BranchDeleteRemote(client, repo, "branch") + if isError := err != nil; isError != tc.expectError { + t.Fatalf("unexpected result: %v", err) + } + }) + } +} From cb2308c07767f990b3f3ddffd37ce2b7eeade03e Mon Sep 17 00:00:00 2001 From: AliabbasMerchant Date: Mon, 29 Jun 2020 00:15:34 +0530 Subject: [PATCH 26/35] --silent flag in api --- pkg/cmd/api/api.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index e1edabbc0..6b393cc42 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -34,6 +34,7 @@ type ApiOptions struct { RequestHeaders []string ShowResponseHeaders bool Paginate bool + Silent bool HttpClient func() (*http.Client, error) BaseRepo func() (ghrepo.Interface, error) @@ -134,6 +135,7 @@ original query accepts an '$endCursor: String' variable and that it fetches the cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response headers in the output") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The file to use as body for the HTTP request") + cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Silence the output") return cmd } @@ -178,6 +180,10 @@ func apiRun(opts *ApiOptions) error { return err } + if opts.Silent { + opts.IO.Out = ioutil.Discard + } + hasNextPage := true for hasNextPage { resp, err := httpRequest(httpClient, method, requestPath, requestBody, requestHeaders) From 5e56e3138449a1a12ff2f7838ad70ea0ac8d1be9 Mon Sep 17 00:00:00 2001 From: AliabbasMerchant Date: Mon, 29 Jun 2020 00:30:24 +0530 Subject: [PATCH 27/35] Added Test for --silent flag in api --- pkg/cmd/api/api_test.go | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 7b89d715c..a41f82c54 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -39,6 +39,7 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string(nil), ShowResponseHeaders: false, Paginate: false, + Silent: false, }, wantsErr: false, }, @@ -55,6 +56,7 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string(nil), ShowResponseHeaders: false, Paginate: false, + Silent: false, }, wantsErr: false, }, @@ -71,6 +73,7 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string(nil), ShowResponseHeaders: false, Paginate: false, + Silent: false, }, wantsErr: false, }, @@ -87,6 +90,7 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string{"accept: text/plain"}, ShowResponseHeaders: true, Paginate: false, + Silent: false, }, wantsErr: false, }, @@ -103,6 +107,24 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string(nil), ShowResponseHeaders: false, Paginate: true, + Silent: false, + }, + wantsErr: false, + }, + { + name: "with silenced output", + cli: "repos/OWNER/REPO/issues --silent", + wants: ApiOptions{ + RequestMethod: "GET", + RequestMethodPassed: false, + RequestPath: "repos/OWNER/REPO/issues", + RequestInputFile: "", + RawFields: []string(nil), + MagicFields: []string(nil), + RequestHeaders: []string(nil), + ShowResponseHeaders: false, + Paginate: false, + Silent: true, }, wantsErr: false, }, @@ -124,6 +146,7 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string(nil), ShowResponseHeaders: false, Paginate: true, + Silent: false, }, wantsErr: false, }, @@ -145,6 +168,7 @@ func Test_NewCmdApi(t *testing.T) { RequestHeaders: []string(nil), ShowResponseHeaders: false, Paginate: false, + Silent: false, }, wantsErr: false, }, @@ -425,6 +449,37 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { assert.Equal(t, "PAGE1_END", endCursor) } +func Test_apiRun_silent(t *testing.T) { + io, _, stdout, stderr := iostreams.Test() + response := &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`body`)), + Header: http.Header{"Content-Type": []string{"text/plain"}}, + } + + options := ApiOptions{ + IO: io, + HttpClient: func() (*http.Client, error) { + var tr roundTripper = func(req *http.Request) (*http.Response, error) { + resp := response + resp.Request = req + return resp, nil + } + return &http.Client{Transport: tr}, nil + }, + RequestPath: "issues", + Silent: true, + } + + err := apiRun(&options) + assert.NoError(t, err) + + assert.Equal(t, "", stdout.String(), "stdout") + assert.Equal(t, "", stderr.String(), "stderr") + + assert.Equal(t, "https://api.github.com/issues", response.Request.URL.String()) +} + func Test_apiRun_inputFile(t *testing.T) { tests := []struct { name string From aa43c55f60d4818f482ae64ef74f46e7e957c2c3 Mon Sep 17 00:00:00 2001 From: AliabbasMerchant Date: Mon, 29 Jun 2020 07:13:06 +0530 Subject: [PATCH 28/35] Skip printing headers when --silent in api --- pkg/cmd/api/api.go | 2 +- pkg/cmd/api/api_test.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 6b393cc42..15a2b3be4 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -218,7 +218,7 @@ func apiRun(opts *ApiOptions) error { } func processResponse(resp *http.Response, opts *ApiOptions) (endCursor string, err error) { - if opts.ShowResponseHeaders { + if opts.ShowResponseHeaders && !opts.Silent { fmt.Fprintln(opts.IO.Out, resp.Proto, resp.Status) printHeaders(opts.IO.Out, resp.Header, opts.IO.ColorEnabled()) fmt.Fprint(opts.IO.Out, "\r\n") diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index a41f82c54..75c8eaa4a 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -467,8 +467,9 @@ func Test_apiRun_silent(t *testing.T) { } return &http.Client{Transport: tr}, nil }, - RequestPath: "issues", - Silent: true, + RequestPath: "issues", + ShowResponseHeaders: true, + Silent: true, } err := apiRun(&options) From 757bd05c7ad9e32079d91e214d3ef4d588321b6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 30 Jun 2020 14:24:01 +0200 Subject: [PATCH 29/35] :nail_care: tweaks for mention/milestone filter --- command/issue.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/command/issue.go b/command/issue.go index 9dd031ee5..c00d624af 100644 --- a/command/issue.go +++ b/command/issue.go @@ -41,8 +41,8 @@ func init() { issueListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|all}") issueListCmd.Flags().IntP("limit", "L", 30, "Maximum number of issues to fetch") issueListCmd.Flags().StringP("author", "A", "", "Filter by author") - issueListCmd.Flags().StringP("mention", "", "", "Filter by mention") - issueListCmd.Flags().StringP("milestone", "", "", "Filter by milestone") + issueListCmd.Flags().String("mention", "", "Filter by mention") + issueListCmd.Flags().StringP("milestone", "m", "", "Filter by milestone `name`") issueCmd.AddCommand(issueViewCmd) issueViewCmd.Flags().BoolP("web", "w", false, "Open an issue in the browser") @@ -161,7 +161,7 @@ func issueList(cmd *cobra.Command, args []string) error { hasFilters := false cmd.Flags().Visit(func(f *pflag.Flag) { switch f.Name { - case "state", "label", "assignee", "author": + case "state", "label", "assignee", "author", "mention", "milestone": hasFilters = true } }) From 7a04bf1672eeffaa544b6f10e461d277b5180e41 Mon Sep 17 00:00:00 2001 From: AliabbasMerchant Date: Tue, 30 Jun 2020 19:18:28 +0530 Subject: [PATCH 30/35] `api --silent` Changes: Show Response Headers (if requested) even with `--silent` flag Shift silent tests to `Test_apiRun` Changed usage string of `--silent` flag --- pkg/cmd/api/api.go | 15 +++++----- pkg/cmd/api/api_test.go | 62 ++++++++++++++++++++--------------------- 2 files changed, 38 insertions(+), 39 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 15a2b3be4..170c41b55 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -135,7 +135,7 @@ original query accepts an '$endCursor: String' variable and that it fetches the cmd.Flags().BoolVarP(&opts.ShowResponseHeaders, "include", "i", false, "Include HTTP response headers in the output") cmd.Flags().BoolVar(&opts.Paginate, "paginate", false, "Make additional HTTP requests to fetch all pages of results") cmd.Flags().StringVar(&opts.RequestInputFile, "input", "", "The file to use as body for the HTTP request") - cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Silence the output") + cmd.Flags().BoolVar(&opts.Silent, "silent", false, "Do not print the response body") return cmd } @@ -180,6 +180,7 @@ func apiRun(opts *ApiOptions) error { return err } + headersOutputStream := opts.IO.Out if opts.Silent { opts.IO.Out = ioutil.Discard } @@ -191,7 +192,7 @@ func apiRun(opts *ApiOptions) error { return err } - endCursor, err := processResponse(resp, opts) + endCursor, err := processResponse(resp, opts, headersOutputStream) if err != nil { return err } @@ -217,11 +218,11 @@ func apiRun(opts *ApiOptions) error { return nil } -func processResponse(resp *http.Response, opts *ApiOptions) (endCursor string, err error) { - if opts.ShowResponseHeaders && !opts.Silent { - fmt.Fprintln(opts.IO.Out, resp.Proto, resp.Status) - printHeaders(opts.IO.Out, resp.Header, opts.IO.ColorEnabled()) - fmt.Fprint(opts.IO.Out, "\r\n") +func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream io.Writer) (endCursor string, err error) { + if opts.ShowResponseHeaders { + fmt.Fprintln(headersOutputStream, resp.Proto, resp.Status) + printHeaders(headersOutputStream, resp.Header, opts.IO.ColorEnabled()) + fmt.Fprint(headersOutputStream, "\r\n") } if resp.StatusCode == 204 { diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 75c8eaa4a..f590d93b4 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -288,6 +288,36 @@ func Test_apiRun(t *testing.T) { stdout: `gateway timeout`, stderr: "gh: HTTP 502\n", }, + { + name: "silent", + options: ApiOptions{ + Silent: true, + }, + httpResponse: &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`body`)), + }, + err: nil, + stdout: ``, + stderr: ``, + }, + { + name: "show response headers even when silent", + options: ApiOptions{ + ShowResponseHeaders: true, + Silent: true, + }, + httpResponse: &http.Response{ + Proto: "HTTP/1.1", + Status: "200 Okey-dokey", + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`body`)), + Header: http.Header{"Content-Type": []string{"text/plain"}}, + }, + err: nil, + stdout: "HTTP/1.1 200 Okey-dokey\nContent-Type: text/plain\r\n\r\n", + stderr: ``, + }, } for _, tt := range tests { @@ -449,38 +479,6 @@ func Test_apiRun_paginationGraphQL(t *testing.T) { assert.Equal(t, "PAGE1_END", endCursor) } -func Test_apiRun_silent(t *testing.T) { - io, _, stdout, stderr := iostreams.Test() - response := &http.Response{ - StatusCode: 200, - Body: ioutil.NopCloser(bytes.NewBufferString(`body`)), - Header: http.Header{"Content-Type": []string{"text/plain"}}, - } - - options := ApiOptions{ - IO: io, - HttpClient: func() (*http.Client, error) { - var tr roundTripper = func(req *http.Request) (*http.Response, error) { - resp := response - resp.Request = req - return resp, nil - } - return &http.Client{Transport: tr}, nil - }, - RequestPath: "issues", - ShowResponseHeaders: true, - Silent: true, - } - - err := apiRun(&options) - assert.NoError(t, err) - - assert.Equal(t, "", stdout.String(), "stdout") - assert.Equal(t, "", stderr.String(), "stderr") - - assert.Equal(t, "https://api.github.com/issues", response.Request.URL.String()) -} - func Test_apiRun_inputFile(t *testing.T) { tests := []struct { name string From db88ac415523282f6c7cba3fa6434586934ab737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 30 Jun 2020 18:51:53 +0200 Subject: [PATCH 31/35] Declare `-R, --repo` flag on all `issue` and `pr` subcommands --- command/issue.go | 8 ++------ command/pr.go | 9 ++------- command/pr_diff.go | 1 - command/pr_review.go | 1 - command/util.go | 7 ------- 5 files changed, 4 insertions(+), 22 deletions(-) delete mode 100644 command/util.go diff --git a/command/issue.go b/command/issue.go index 9569c9603..287ca7260 100644 --- a/command/issue.go +++ b/command/issue.go @@ -22,12 +22,12 @@ import ( ) func init() { + issueCmd.PersistentFlags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") + RootCmd.AddCommand(issueCmd) issueCmd.AddCommand(issueStatusCmd) - includeRepoFlag(issueStatusCmd) issueCmd.AddCommand(issueCreateCmd) - includeRepoFlag(issueCreateCmd) issueCreateCmd.Flags().StringP("title", "t", "", "Supply a title. Will prompt for one otherwise.") issueCreateCmd.Flags().StringP("body", "b", "", @@ -39,7 +39,6 @@ func init() { issueCreateCmd.Flags().StringP("milestone", "m", "", "Add the issue to a milestone by `name`") issueCmd.AddCommand(issueListCmd) - includeRepoFlag(issueListCmd) issueListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") issueListCmd.Flags().StringSliceP("label", "l", nil, "Filter by labels") issueListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|all}") @@ -47,13 +46,10 @@ func init() { issueListCmd.Flags().StringP("author", "A", "", "Filter by author") issueCmd.AddCommand(issueViewCmd) - includeRepoFlag(issueViewCmd) issueViewCmd.Flags().BoolP("web", "w", false, "Open an issue in the browser") issueCmd.AddCommand(issueCloseCmd) - includeRepoFlag(issueCloseCmd) issueCmd.AddCommand(issueReopenCmd) - includeRepoFlag(issueReopenCmd) } var issueCmd = &cobra.Command{ diff --git a/command/pr.go b/command/pr.go index 961cdbf1a..6d8ae22aa 100644 --- a/command/pr.go +++ b/command/pr.go @@ -23,26 +23,22 @@ import ( ) func init() { + prCmd.PersistentFlags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") + RootCmd.AddCommand(prCmd) prCmd.AddCommand(prCheckoutCmd) prCmd.AddCommand(prCreateCmd) prCmd.AddCommand(prStatusCmd) - includeRepoFlag(prStatusCmd) prCmd.AddCommand(prCloseCmd) - includeRepoFlag(prCloseCmd) prCmd.AddCommand(prReopenCmd) - includeRepoFlag(prReopenCmd) prCmd.AddCommand(prMergeCmd) - includeRepoFlag(prMergeCmd) prMergeCmd.Flags().BoolP("delete-branch", "d", true, "Delete the local and remote branch after merge") prMergeCmd.Flags().BoolP("merge", "m", false, "Merge the commits with the base branch") prMergeCmd.Flags().BoolP("rebase", "r", false, "Rebase the commits onto the base branch") prMergeCmd.Flags().BoolP("squash", "s", false, "Squash the commits into one commit and merge it into the base branch") prCmd.AddCommand(prReadyCmd) - includeRepoFlag(prReadyCmd) prCmd.AddCommand(prListCmd) - includeRepoFlag(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") prListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|merged|all}") prListCmd.Flags().StringP("base", "B", "", "Filter by base branch") @@ -50,7 +46,6 @@ func init() { prListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") prCmd.AddCommand(prViewCmd) - includeRepoFlag(prViewCmd) prViewCmd.Flags().BoolP("web", "w", false, "Open a pull request in the browser") } diff --git a/command/pr_diff.go b/command/pr_diff.go index 0bc9c8613..ae50a2618 100644 --- a/command/pr_diff.go +++ b/command/pr_diff.go @@ -19,7 +19,6 @@ func init() { prDiffCmd.Flags().StringP("color", "c", "auto", "Whether or not to output color: {always|never|auto}") prCmd.AddCommand(prDiffCmd) - includeRepoFlag(prDiffCmd) } func prDiff(cmd *cobra.Command, args []string) error { diff --git a/command/pr_review.go b/command/pr_review.go index 77723ab44..08efc638d 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -15,7 +15,6 @@ import ( func init() { prCmd.AddCommand(prReviewCmd) - includeRepoFlag(prCmd) prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request") prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request") diff --git a/command/util.go b/command/util.go deleted file mode 100644 index 130f97844..000000000 --- a/command/util.go +++ /dev/null @@ -1,7 +0,0 @@ -package command - -import "github.com/spf13/cobra" - -func includeRepoFlag(cmd *cobra.Command) { - cmd.Flags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") -} From 1ca3d171e6de6cfdaec29b0e6bc1b76780476833 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 30 Jun 2020 19:13:54 +0200 Subject: [PATCH 32/35] Tweak HTTP 422 handling when deleting branches --- api/client.go | 45 ++++++++++++++++++++++-------------------- api/client_test.go | 23 ++++++++++++++++----- api/queries_pr.go | 14 +------------ api/queries_pr_test.go | 37 ++++++++++++++++++---------------- command/issue_test.go | 2 +- command/pr.go | 4 +++- pkg/httpmock/stub.go | 6 ++++++ 7 files changed, 73 insertions(+), 58 deletions(-) diff --git a/api/client.go b/api/client.go index 6880609bb..74a5a0417 100644 --- a/api/client.go +++ b/api/client.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net/http" + "net/url" "regexp" "strings" @@ -154,18 +155,21 @@ func (gr GraphQLErrorResponse) Error() string { for _, e := range gr.Errors { errorMessages = append(errorMessages, e.Message) } - return fmt.Sprintf("graphql error: '%s'", strings.Join(errorMessages, ", ")) + return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n")) } // HTTPError is an error returned by a failed API call type HTTPError struct { - Code int - URL string - Message string + StatusCode int + RequestURL *url.URL + Message string } -func (e HTTPError) Error() string { - return fmt.Sprintf("http error, '%s' failed (%d): '%s'", e.URL, e.Code, e.Message) +func (err HTTPError) Error() string { + if err.Message != "" { + return fmt.Sprintf("HTTP %d: %s (%s)", err.StatusCode, err.Message, err.RequestURL) + } + return fmt.Sprintf("HTTP %d (%s)", err.StatusCode, err.RequestURL) } // Returns whether or not scopes are present, appID, and error @@ -294,26 +298,25 @@ func handleResponse(resp *http.Response, data interface{}) error { } func handleHTTPError(resp *http.Response) error { - var message string + httpError := HTTPError{ + StatusCode: resp.StatusCode, + RequestURL: resp.Request.URL, + } + + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + httpError.Message = err.Error() + return httpError + } + var parsedBody struct { Message string `json:"message"` } - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - return err - } - err = json.Unmarshal(body, &parsedBody) - if err != nil { - message = string(body) - } else { - message = parsedBody.Message + if err := json.Unmarshal(body, &parsedBody); err == nil { + httpError.Message = parsedBody.Message } - return HTTPError{ - Code: resp.StatusCode, - URL: resp.Request.URL.String(), - Message: message, - } + return httpError } var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`) diff --git a/api/client_test.go b/api/client_test.go index 9c908be12..b7c226c8f 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -47,9 +47,15 @@ func TestGraphQLError(t *testing.T) { client := NewClient(ReplaceTripper(http)) response := struct{}{} - http.StubResponse(200, bytes.NewBufferString(`{"errors":[{"message":"OH NO"}]}`)) + http.StubResponse(200, bytes.NewBufferString(` + { "errors": [ + {"message":"OH NO"}, + {"message":"this is fine"} + ] + }`)) + err := client.GraphQL("", nil, &response) - if err == nil || err.Error() != "graphql error: 'OH NO'" { + if err == nil || err.Error() != "GraphQL error: OH NO\nthis is fine" { t.Fatalf("got %q", err.Error()) } } @@ -75,8 +81,15 @@ func TestRESTError(t *testing.T) { http.StubResponse(422, bytes.NewBufferString(`{"message": "OH NO"}`)) var httpErr HTTPError - err := client.REST("DELETE", "/repos/branch", nil, nil) - if err == nil || !errors.As(err, &httpErr) || httpErr.Code != 422 { - t.Fatalf("got %q", err.Error()) + err := client.REST("DELETE", "repos/branch", nil, nil) + if err == nil || !errors.As(err, &httpErr) { + t.Fatalf("got %v", err) + } + + if httpErr.StatusCode != 422 { + t.Errorf("expected status code 422, got %d", httpErr.StatusCode) + } + if httpErr.Error() != "HTTP 422: OH NO (https://api.github.com/repos/branch)" { + t.Errorf("got %q", httpErr.Error()) } } diff --git a/api/queries_pr.go b/api/queries_pr.go index b42a94eec..56582d0e3 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1011,20 +1011,8 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er } func BranchDeleteRemote(client *Client, repo ghrepo.Interface, branch string) error { - var response struct { - NodeID string `json:"node_id"` - } path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), branch) - err := client.REST("DELETE", path, nil, &response) - if err != nil { - var httpErr HTTPError - // The ref might have already been deleted by GitHub - if !errors.As(err, &httpErr) || httpErr.Code != 422 { - return err - } - } - - return nil + return client.REST("DELETE", path, nil, nil) } func min(a, b int) int { diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 3b8bf0a65..07370023b 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -1,7 +1,6 @@ package api import ( - "bytes" "testing" "github.com/cli/cli/internal/ghrepo" @@ -10,31 +9,35 @@ import ( func TestBranchDeleteRemote(t *testing.T) { var tests = []struct { - name string - code int - body string - expectError bool + name string + responseStatus int + responseBody string + expectError bool }{ - {name: "success", code: 204, body: "", expectError: false}, - {name: "error", code: 500, body: `{"message": "oh no"}`, expectError: true}, { - name: "already_deleted", - code: 422, - body: `{"message": "Reference does not exist"}`, - expectError: false, + name: "success", + responseStatus: 204, + responseBody: "", + expectError: false, + }, + { + name: "error", + responseStatus: 500, + responseBody: `{"message": "oh no"}`, + expectError: true, }, } - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { http := &httpmock.Registry{} - client := NewClient(ReplaceTripper(http)) + http.Register(httpmock.MatchAny, httpmock.StatusStringResponse(tt.responseStatus, tt.responseBody)) - http.StubResponse(tc.code, bytes.NewBufferString(tc.body)) + client := NewClient(ReplaceTripper(http)) repo, _ := ghrepo.FromFullName("OWNER/REPO") + err := BranchDeleteRemote(client, repo, "branch") - if isError := err != nil; isError != tc.expectError { + if (err != nil) != tt.expectError { t.Fatalf("unexpected result: %v", err) } }) diff --git a/command/issue_test.go b/command/issue_test.go index 418de25d3..b7db7a7e2 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -402,7 +402,7 @@ func TestIssueView_web_notFound(t *testing.T) { defer restoreCmd() _, err := RunCommand("issue view -w 9999") - if err == nil || err.Error() != "graphql error: 'Could not resolve to an Issue with the number of 9999.'" { + if err == nil || err.Error() != "GraphQL error: Could not resolve to an Issue with the number of 9999." { t.Errorf("error running command `issue view`: %v", err) } diff --git a/command/pr.go b/command/pr.go index 2068a10e8..d1720ca42 100644 --- a/command/pr.go +++ b/command/pr.go @@ -523,7 +523,9 @@ func prMerge(cmd *cobra.Command, args []string) error { if !crossRepoPR { err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName) - if err != nil { + var httpErr api.HTTPError + // The ref might have already been deleted by GitHub + if err != nil && (!errors.As(err, &httpErr) || httpErr.StatusCode != 422) { err = fmt.Errorf("failed to delete remote branch %s: %w", utils.Cyan(pr.HeadRefName), err) return err } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index cc42dfd6c..c57b7a1ad 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -64,6 +64,12 @@ func StringResponse(body string) Responder { } } +func StatusStringResponse(status int, body string) Responder { + return func(req *http.Request) (*http.Response, error) { + return httpResponse(status, req, bytes.NewBufferString(body)), nil + } +} + func JSONResponse(body interface{}) Responder { return func(req *http.Request) (*http.Response, error) { b, _ := json.Marshal(body) From 10b4c8ab26b7a65ed58a403f20a8eb76be4a1512 Mon Sep 17 00:00:00 2001 From: wilso199 Date: Tue, 30 Jun 2020 18:17:05 -0400 Subject: [PATCH 33/35] Adding config set example for vscode to docs --- command/config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command/config.go b/command/config.go index 7f34b8687..0a77e99e5 100644 --- a/command/config.go +++ b/command/config.go @@ -48,6 +48,7 @@ var configSetCmd = &cobra.Command{ Short: "Update configuration with a value for the given key", Example: heredoc.Doc(` $ gh config set editor vim + $ gh config set editor "code --wait" `), Args: cobra.ExactArgs(2), RunE: configSet, From 9bbdf4af990c108ad061579085a8519de8714414 Mon Sep 17 00:00:00 2001 From: Eli Schwartz Date: Wed, 1 Jul 2020 11:04:51 -0400 Subject: [PATCH 34/35] Make: properly add environment C compiler flags to CGO Do not pass LDFLAGS as arguments to -ldflags, since these are passed to 'go tool link' and C compiler ldflags need to be passed as -ldflags "-extldflags \"$LDFLAGS\"" with unreliable handling. Instead copy over (unmodified) the standard environment variable to the special golang-specific variable which the go compiler chooses to respect. While we are at it, handle CPPFLAGS and CFLAGS too. --- Makefile | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index b925c11fd..fd219d42d 100644 --- a/Makefile +++ b/Makefile @@ -8,15 +8,26 @@ ifdef SOURCE_DATE_EPOCH else BUILD_DATE ?= $(shell date "$(DATE_FMT)") endif -LDFLAGS := -X github.com/cli/cli/command.Version=$(GH_VERSION) $(LDFLAGS) -LDFLAGS := -X github.com/cli/cli/command.BuildDate=$(BUILD_DATE) $(LDFLAGS) + +ifndef CGO_CPPFLAGS + export CGO_CPPFLAGS := $(CPPFLAGS) +endif +ifndef CGO_CFLAGS + export CGO_CFLAGS := $(CFLAGS) +endif +ifndef CGO_LDFLAGS + export CGO_LDFLAGS := $(LDFLAGS) +endif + +GO_LDFLAGS := -X github.com/cli/cli/command.Version=$(GH_VERSION) +GO_LDFLAGS := -X github.com/cli/cli/command.BuildDate=$(BUILD_DATE) ifdef GH_OAUTH_CLIENT_SECRET - LDFLAGS := -X github.com/cli/cli/internal/config.oauthClientID=$(GH_OAUTH_CLIENT_ID) $(LDFLAGS) - LDFLAGS := -X github.com/cli/cli/internal/config.oauthClientSecret=$(GH_OAUTH_CLIENT_SECRET) $(LDFLAGS) + GO_LDFLAGS := -X github.com/cli/cli/internal/config.oauthClientID=$(GH_OAUTH_CLIENT_ID) + GO_LDFLAGS := -X github.com/cli/cli/internal/config.oauthClientSecret=$(GH_OAUTH_CLIENT_SECRET) endif bin/gh: $(BUILD_FILES) - @go build -trimpath -ldflags "$(LDFLAGS)" -o "$@" ./cmd/gh + @go build -trimpath -ldflags "$(GO_LDFLAGS)" -o "$@" ./cmd/gh test: go test ./... From cd5a0d69fbddd10214895bcd58bd6a1e66dd596d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 2 Jul 2020 12:36:31 +0200 Subject: [PATCH 35/35] :nail_polish: be clearer about the value passed to ResolveRemotesToRepos `repo` will always be blank here, so replace the argument with a blank literal instead. --- command/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/root.go b/command/root.go index 83dfbd3f0..cf0e3cbb1 100644 --- a/command/root.go +++ b/command/root.go @@ -317,7 +317,7 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return nil, err } - repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, repo) + repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, "") if err != nil { return nil, err }