From fc4cd12cbfe0a63f3fd24cf9f4fa3e80d3ec8ec9 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sun, 8 May 2022 12:32:41 -0700 Subject: [PATCH 1/3] Ignore FORBIDDEN errors for `gh status` Fixes #5587 --- pkg/cmd/status/fixtures/search_forbidden.json | 104 ++++++++++++++ pkg/cmd/status/status.go | 64 ++++++++- pkg/cmd/status/status_test.go | 132 ++++++++++++++++++ 3 files changed, 293 insertions(+), 7 deletions(-) create mode 100644 pkg/cmd/status/fixtures/search_forbidden.json diff --git a/pkg/cmd/status/fixtures/search_forbidden.json b/pkg/cmd/status/fixtures/search_forbidden.json new file mode 100644 index 000000000..7ec0727f9 --- /dev/null +++ b/pkg/cmd/status/fixtures/search_forbidden.json @@ -0,0 +1,104 @@ +{ + "data": { + "assignments": { + "edges": [ + { + "node": { + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" + } + } + }, + null, + { + "node": { + "__typename": "Issue", + "updatedAt": "2022-03-10T21:33:57Z", + "title": "yolo", + "number": 157, + "repository": { + "nameWithOwner": "vilmibm/testing" + } + } + }, + null + ] + }, + "reviewRequested": { + "edges": [ + { + "node": { + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" + } + } + }, + null + ] + } + }, + "errors": [ + { + "type": "FORBIDDEN", + "path": [ + "assignments", + "edges", + 0 + ], + "extensions": { + "saml_failure": true + }, + "locations": [ + { + "line": 4, + "column": 5 + } + ], + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + }, + { + "type": "FORBIDDEN", + "path": [ + "assignments", + "edges", + 0 + ], + "extensions": { + "saml_failure": true + }, + "locations": [ + { + "line": 4, + "column": 5 + } + ], + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + }, + { + "type": "FORBIDDEN", + "path": [ + "reviewRequested", + "edges", + 0 + ], + "extensions": { + "saml_failure": true + }, + "locations": [ + { + "line": 4, + "column": 5 + } + ], + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + } + ] +} diff --git a/pkg/cmd/status/status.go b/pkg/cmd/status/status.go index c9e5ff3e6..d9b534566 100644 --- a/pkg/cmd/status/status.go +++ b/pkg/cmd/status/status.go @@ -8,6 +8,7 @@ import ( "net/url" "sort" "strings" + "sync" "time" "github.com/MakeNowJust/heredoc" @@ -44,7 +45,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "status", Short: "Print information about relevant issues, pull requests, and notifications across repositories", - Long: heredoc.Doc(` + Long: heredoc.Docf(` The status command prints information about your work on GitHub across all the repositories you're subscribed to, including: - Assigned Issues @@ -52,7 +53,9 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co - Review Requests - Mentions - Repository Activity (new issues/pull requests, comments) - `), + + Some status may be unavailable due to organization SAML enforcement. Run %[1]sgh auth refresh%[1]s to authorization access to organizations. + `, "`"), Example: heredoc.Doc(` $ gh status -e cli/cli -e cli/go-gh # Exclude multiple repositories $ gh status -o cli # Limit results to a single organization @@ -166,6 +169,8 @@ type StatusGetter struct { Mentions []StatusItem ReviewRequests []StatusItem RepoActivity []StatusItem + hasAuthErrors bool + authErrorsMu sync.Mutex } func NewStatusGetter(client *http.Client, hostname string, opts *StatusOptions) *StatusGetter { @@ -295,7 +300,14 @@ func (s *StatusGetter) LoadNotifications() error { preview: actual, }) } else if err != nil { - return fmt.Errorf("could not fetch comment: %w", err) + var httpErr api.HTTPError + if !errors.As(err, &httpErr) || httpErr.StatusCode != 403 { + return fmt.Errorf("could not fetch comment: %w", err) + } else if httpErr.StatusCode == 403 { + s.authErrorsMu.Lock() + s.hasAuthErrors = true + s.authErrorsMu.Unlock() + } } } @@ -372,7 +384,18 @@ func (s *StatusGetter) LoadSearchResults() error { } err := c.GraphQL(s.hostname(), searchQuery, variables, &resp) if err != nil { - return fmt.Errorf("could not search for assignments: %w", err) + // Exclude any FORBIDDEN errors and show status for what we can. + if gqlErrResponse, ok := err.(*api.GraphQLErrorResponse); ok { + err = filterGraphQLErrors(gqlErrResponse) + if err == nil { + s.authErrorsMu.Lock() + s.hasAuthErrors = true + s.authErrorsMu.Unlock() + } + } + if err != nil { + return fmt.Errorf("could not search for assignments: %w", err) + } } prs := []SearchResult{} @@ -384,13 +407,16 @@ func (s *StatusGetter) LoadSearchResults() error { issues = append(issues, e.Node) } else if e.Node.Type == "PullRequest" { prs = append(prs, e.Node) - } else { + } else if e.Node.Type != "" { + // FORBIDDEN nodes are null in response, so Type == "" here. panic("you shouldn't be here") } } for _, e := range resp.ReviewRequested.Edges { - reviewRequested = append(reviewRequested, e.Node) + if e.Node.Type != "" { + reviewRequested = append(reviewRequested, e.Node) + } } sort.Sort(Results(issues)) @@ -428,6 +454,19 @@ func (s *StatusGetter) LoadSearchResults() error { return nil } +func filterGraphQLErrors(gqlErrResponse *api.GraphQLErrorResponse) error { + gqlErrors := make([]api.GraphQLError, 0, len(gqlErrResponse.Errors)) + for _, gqlErr := range gqlErrResponse.Errors { + if gqlErr.Type != "FORBIDDEN" { + gqlErrors = append(gqlErrors, gqlErr) + } + } + if len(gqlErrors) > 0 { + return &api.GraphQLErrorResponse{Errors: gqlErrors} + } + return nil +} + // Populate .RepoActivity func (s *StatusGetter) LoadEvents() error { perPage := 100 @@ -506,6 +545,13 @@ func (s *StatusGetter) LoadEvents() error { return nil } +func (s *StatusGetter) HasAuthErrors() bool { + s.authErrorsMu.Lock() + defer s.authErrorsMu.Unlock() + + return s.hasAuthErrors +} + func statusRun(opts *StatusOptions) error { client, err := opts.HttpClient() if err != nil { @@ -548,10 +594,10 @@ func statusRun(opts *StatusOptions) error { }) err = g.Wait() + opts.IO.StopProgressIndicator() if err != nil { return err } - opts.IO.StopProgressIndicator() cs := opts.IO.ColorScheme() out := opts.IO.Out @@ -628,5 +674,9 @@ func statusRun(opts *StatusOptions) error { fmt.Fprintln(out, lipgloss.JoinHorizontal(lipgloss.Top, rrSection, mSection)) fmt.Fprintln(out, raSection) + if sg.HasAuthErrors() { + fmt.Fprintln(out, cs.Gray("Some status unavailable due to organization SAML enforcement. Run `gh auth refresh` to authorization access to organizations.")) + } + return nil } diff --git a/pkg/cmd/status/status_test.go b/pkg/cmd/status/status_test.go index 9475ade1c..a845cab90 100644 --- a/pkg/cmd/status/status_test.go +++ b/pkg/cmd/status/status_test.go @@ -271,6 +271,138 @@ func TestStatusRun(t *testing.T) { }, wantOut: "Assigned Issues │ Assigned Pull Requests \nvilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions \ncli/cli#3223 Repo garden...│ rpd/todo#73 Board up RPD windows \nrpd/todo#514 Reducing zo...│ cli/cli#4768 Issue Frecency \nvilmibm/testing#74 welp │ \nadreyer/arkestrator#22 complete mo...│ \n │ \nReview Requests │ Mentions \ncli/cli#5272 Pin extensions │ rpd/todo#110 hello @jillvalentine ...\nvilmibm/testing#1234 Foobar │ \nrpd/todo#50 Welcome party...│ \ncli/cli#4671 This pull req...│ \nrpd/todo#49 Haircut for Leon│ \n │ \nRepository Activity\nrpd/todo#5326 new PR Only write UTF-8 BOM on Windows where it is needed\n\n", }, + { + name: "forbidden errors", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/110"), + httpmock.StatusStringResponse(403, `{ + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + }`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/4113"), + httpmock.StringResponse(`{"body":"this is a comment"}`)) + reg.Register( + httpmock.REST("GET", "repos/cli/cli/issues/1096"), + httpmock.StringResponse(`{"body":"@jillvalentine hi"}`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/comments/1065"), + httpmock.StringResponse(`{"body":"not a real mention"}`)) + reg.Register( + httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), + httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.FileResponse("./fixtures/search_forbidden.json")) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.FileResponse("./fixtures/notifications.json")) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.FileResponse("./fixtures/events.json")) + }, + opts: &StatusOptions{}, + wantOut: "Assigned Issues │ Assigned Pull Requests \nvilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions \n │ \nReview Requests │ Mentions \ncli/cli#5272 Pin extensions │ cli/cli#1096 @jillval...\n │ vilmibm/gh-screensaver#15 a messag...\n \nRepository Activity\nrpd/todo#5326 new PR Only write UTF-8 BOM on W...\nvilmibm/testing#5325 comment on Ability to sea... We are working on dedicat...\ncli/cli#5319 comment on [Codespaces] D... Wondering if we shouldn't...\ncli/cli#5300 new issue Terminal bell when a runn...\n\nSome status unavailable due to organization SAML enforcement. Run `gh auth refresh` to authorization access to organizations.\n", + }, + { + name: "notification errors", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/110"), + httpmock.StatusStringResponse(429, `{ + "message": "Too many requests." + }`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.StringResponse(`{"data": { "assignments": {"edges": [] }, "reviewRequested": {"edges": []}}}`)) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.StringResponse(`[ + { + "reason": "mention", + "subject": { + "title": "Good", + "url": "https://api.github.com/repos/rpd/todo/issues/110", + "latest_comment_url": "https://api.github.com/repos/rpd/todo/issues/110", + "type": "Issue" + }, + "repository": { + "full_name": "rpd/todo", + "owner": { + "login": "rpd" + } + } + } + ]`)) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.StringResponse(`[]`)) + }, + opts: &StatusOptions{}, + wantErrMsg: "could not load notifications: could not fetch comment: HTTP 429 (https://api.github.com/repos/rpd/todo/issues/110)", + }, + { + name: "search errors", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.StringResponse(`{ + "data": { + "assignments": { + "edges": [ + null + ] + } + }, + "errors": [ + { + "type": "NOT FOUND", + "path": [ + "assignments", + "edges", + 0 + ], + "message": "Not found." + } + ] + }`)) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.StringResponse(`[]`)) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.StringResponse(`[]`)) + }, + opts: &StatusOptions{}, + wantErrMsg: "failed to search: could not search for assignments: GraphQL: Not found. (assignments.edges.0)", + }, } for _, tt := range tests { From d103ba251af9b58874c51f7ee68a6084c1706b9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 10 May 2022 11:55:55 +0200 Subject: [PATCH 2/3] :nail_care: optimize fetches --- pkg/cmd/status/fixtures/search.json | 228 ++++++------- pkg/cmd/status/fixtures/search_forbidden.json | 58 ++-- pkg/cmd/status/status.go | 306 ++++++++++-------- pkg/cmd/status/status_test.go | 104 +++--- 4 files changed, 340 insertions(+), 356 deletions(-) diff --git a/pkg/cmd/status/fixtures/search.json b/pkg/cmd/status/fixtures/search.json index e0367bdca..27599c833 100644 --- a/pkg/cmd/status/fixtures/search.json +++ b/pkg/cmd/status/fixtures/search.json @@ -1,185 +1,153 @@ { "data": { "assignments": { - "edges": [ + "nodes": [ { - "node": { - "updatedAt": "2022-03-15T17:10:25Z", - "__typename": "PullRequest", - "title": "Pin extensions", - "number": 5272, - "repository": { - "nameWithOwner": "cli/cli" - } + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" } }, { - "node": { - "updatedAt": "2022-03-01T17:10:25Z", - "__typename": "PullRequest", - "title": "Board up RPD windows", - "number": 73, - "repository": { - "nameWithOwner": "rpd/todo" - } + "updatedAt": "2022-03-01T17:10:25Z", + "__typename": "PullRequest", + "title": "Board up RPD windows", + "number": 73, + "repository": { + "nameWithOwner": "rpd/todo" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2022-03-10T21:33:57Z", - "title": "yolo", - "number": 157, - "repository": { - "nameWithOwner": "vilmibm/testing" - } + "__typename": "Issue", + "updatedAt": "2022-03-10T21:33:57Z", + "title": "yolo", + "number": 157, + "repository": { + "nameWithOwner": "vilmibm/testing" } }, { - "node": { - "updatedAt": "2022-01-06T05:05:12Z", - "__typename": "PullRequest", - "title": "Issue Frecency", - "number": 4768, - "repository": { - "nameWithOwner": "cli/cli" - } + "updatedAt": "2022-01-06T05:05:12Z", + "__typename": "PullRequest", + "title": "Issue Frecency", + "number": 4768, + "repository": { + "nameWithOwner": "cli/cli" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2021-08-05T20:03:07Z", - "title": "Reducing zombie threat in Raccoon City", - "number": 514, - "repository": { - "nameWithOwner": "rpd/todo" - } + "__typename": "Issue", + "updatedAt": "2021-08-05T20:03:07Z", + "title": "Reducing zombie threat in Raccoon City", + "number": 514, + "repository": { + "nameWithOwner": "rpd/todo" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2021-10-16T23:20:35Z", - "title": "Repo garden for Windows", - "number": 3223, - "repository": { - "nameWithOwner": "cli/cli" - } + "__typename": "Issue", + "updatedAt": "2021-10-16T23:20:35Z", + "title": "Repo garden for Windows", + "number": 3223, + "repository": { + "nameWithOwner": "cli/cli" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2020-11-10T23:07:37Z", - "title": "welp", - "number": 74, - "repository": { - "nameWithOwner": "vilmibm/testing" - } + "__typename": "Issue", + "updatedAt": "2020-11-10T23:07:37Z", + "title": "welp", + "number": 74, + "repository": { + "nameWithOwner": "vilmibm/testing" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2019-07-20T01:30:01Z", - "title": "Ability to send emails from helpdesk UI", - "number": 42, - "repository": { - "nameWithOwner": "tildetown/tildetown-admin" - } + "__typename": "Issue", + "updatedAt": "2019-07-20T01:30:01Z", + "title": "Ability to send emails from helpdesk UI", + "number": 42, + "repository": { + "nameWithOwner": "tildetown/tildetown-admin" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2019-07-22T19:37:45Z", - "title": "complete move to class based views", - "number": 22, - "repository": { - "nameWithOwner": "adreyer/arkestrator" - } + "__typename": "Issue", + "updatedAt": "2019-07-22T19:37:45Z", + "title": "complete move to class based views", + "number": 22, + "repository": { + "nameWithOwner": "adreyer/arkestrator" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2019-02-20T17:54:24Z", - "title": "`(every)` macro", - "number": 145, - "repository": { - "nameWithOwner": "vilmibm/tildemush" - } + "__typename": "Issue", + "updatedAt": "2019-02-20T17:54:24Z", + "title": "`(every)` macro", + "number": 145, + "repository": { + "nameWithOwner": "vilmibm/tildemush" } }, { - "node": { - "__typename": "Issue", - "updatedAt": "2019-02-20T18:10:53Z", - "title": "audit move rules", - "number": 79, - "repository": { - "nameWithOwner": "vilmibm/tildemush" - } + "__typename": "Issue", + "updatedAt": "2019-02-20T18:10:53Z", + "title": "audit move rules", + "number": 79, + "repository": { + "nameWithOwner": "vilmibm/tildemush" } } ] }, "reviewRequested": { - "edges": [ + "nodes": [ { - "node": { - "updatedAt": "2022-03-15T17:10:25Z", - "__typename": "PullRequest", - "title": "Pin extensions", - "number": 5272, - "repository": { - "nameWithOwner": "cli/cli" - } + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" } }, { - "node": { - "updatedAt": "2022-02-18T19:38:20Z", - "__typename": "PullRequest", - "title": "Foobar", - "number": 1234, - "repository": { - "nameWithOwner": "vilmibm/testing" - } + "updatedAt": "2022-02-18T19:38:20Z", + "__typename": "PullRequest", + "title": "Foobar", + "number": 1234, + "repository": { + "nameWithOwner": "vilmibm/testing" } }, { - "node": { - "updatedAt": "2022-02-04T06:56:48Z", - "__typename": "PullRequest", - "title": "Welcome party for Leon", - "number": 50, - "repository": { - "nameWithOwner": "rpd/todo" - } + "updatedAt": "2022-02-04T06:56:48Z", + "__typename": "PullRequest", + "title": "Welcome party for Leon", + "number": 50, + "repository": { + "nameWithOwner": "rpd/todo" } }, { - "node": { - "updatedAt": "2021-12-20T10:30:39Z", - "__typename": "PullRequest", - "title": "Haircut for Leon", - "number": 49, - "repository": { - "nameWithOwner": "rpd/todo" - } + "updatedAt": "2021-12-20T10:30:39Z", + "__typename": "PullRequest", + "title": "Haircut for Leon", + "number": 49, + "repository": { + "nameWithOwner": "rpd/todo" } }, { - "node": { - "updatedAt": "2021-12-20T16:43:15Z", - "__typename": "PullRequest", - "title": "This pull request adds support for json output the to `gh pr checks` …", - "number": 4671, - "repository": { - "nameWithOwner": "cli/cli" - } + "updatedAt": "2021-12-20T16:43:15Z", + "__typename": "PullRequest", + "title": "This pull request adds support for json output the to `gh pr checks` …", + "number": 4671, + "repository": { + "nameWithOwner": "cli/cli" } } ] diff --git a/pkg/cmd/status/fixtures/search_forbidden.json b/pkg/cmd/status/fixtures/search_forbidden.json index 7ec0727f9..5a60801e9 100644 --- a/pkg/cmd/status/fixtures/search_forbidden.json +++ b/pkg/cmd/status/fixtures/search_forbidden.json @@ -1,44 +1,38 @@ { "data": { "assignments": { - "edges": [ + "nodes": [ { - "node": { - "updatedAt": "2022-03-15T17:10:25Z", - "__typename": "PullRequest", - "title": "Pin extensions", - "number": 5272, - "repository": { - "nameWithOwner": "cli/cli" - } + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" } }, null, { - "node": { - "__typename": "Issue", - "updatedAt": "2022-03-10T21:33:57Z", - "title": "yolo", - "number": 157, - "repository": { - "nameWithOwner": "vilmibm/testing" - } + "__typename": "Issue", + "updatedAt": "2022-03-10T21:33:57Z", + "title": "yolo", + "number": 157, + "repository": { + "nameWithOwner": "vilmibm/testing" } }, null ] }, "reviewRequested": { - "edges": [ + "nodes": [ { - "node": { - "updatedAt": "2022-03-15T17:10:25Z", - "__typename": "PullRequest", - "title": "Pin extensions", - "number": 5272, - "repository": { - "nameWithOwner": "cli/cli" - } + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" } }, null @@ -50,8 +44,8 @@ "type": "FORBIDDEN", "path": [ "assignments", - "edges", - 0 + "nodes", + 1 ], "extensions": { "saml_failure": true @@ -68,8 +62,8 @@ "type": "FORBIDDEN", "path": [ "assignments", - "edges", - 0 + "nodes", + 3 ], "extensions": { "saml_failure": true @@ -86,8 +80,8 @@ "type": "FORBIDDEN", "path": [ "reviewRequested", - "edges", - 0 + "nodes", + 1 ], "extensions": { "saml_failure": true diff --git a/pkg/cmd/status/status.go b/pkg/cmd/status/status.go index d9b534566..45c3bde77 100644 --- a/pkg/cmd/status/status.go +++ b/pkg/cmd/status/status.go @@ -2,6 +2,7 @@ package status import ( "bytes" + "context" "errors" "fmt" "net/http" @@ -14,8 +15,10 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/charmbracelet/lipgloss" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/set" "github.com/cli/cli/v2/utils" "github.com/spf13/cobra" "golang.org/x/sync/errgroup" @@ -45,7 +48,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "status", Short: "Print information about relevant issues, pull requests, and notifications across repositories", - Long: heredoc.Docf(` + Long: heredoc.Doc(` The status command prints information about your work on GitHub across all the repositories you're subscribed to, including: - Assigned Issues @@ -53,9 +56,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co - Review Requests - Mentions - Repository Activity (new issues/pull requests, comments) - - Some status may be unavailable due to organization SAML enforcement. Run %[1]sgh auth refresh%[1]s to authorization access to organizations. - `, "`"), + `), Example: heredoc.Doc(` $ gh status -e cli/cli -e cli/go-gh # Exclude multiple repositories $ gh status -o cli # Limit results to a single organization @@ -96,6 +97,7 @@ type Notification struct { } FullName string `json:"full_name"` } + index int } type StatusItem struct { @@ -103,6 +105,7 @@ type StatusItem struct { Identifier string // eg cli/cli#1234 or just 1234 preview string // eg This is the truncated body of something... Reason string // only used in repo activity + index int } func (s StatusItem) Preview() string { @@ -158,6 +161,12 @@ func (rs Results) Swap(i, j int) { rs[i], rs[j] = rs[j], rs[i] } +type stringSet interface { + Len() int + Add(string) + ToSlice() []string +} + type StatusGetter struct { Client *http.Client cachedClient func(*http.Client, time.Duration) *http.Client @@ -169,8 +178,12 @@ type StatusGetter struct { Mentions []StatusItem ReviewRequests []StatusItem RepoActivity []StatusItem - hasAuthErrors bool - authErrorsMu sync.Mutex + + authErrors stringSet + authErrorsMu sync.Mutex + + currentUsername string + usernameMu sync.Mutex } func NewStatusGetter(client *http.Client, hostname string, opts *StatusOptions) *StatusGetter { @@ -201,6 +214,13 @@ func (s *StatusGetter) ShouldExclude(repo string) bool { } func (s *StatusGetter) CurrentUsername() (string, error) { + s.usernameMu.Lock() + defer s.usernameMu.Unlock() + + if s.currentUsername != "" { + return s.currentUsername, nil + } + cachedClient := s.CachedClient(time.Hour * 48) cachingAPIClient := api.NewClientFromHTTP(cachedClient) currentUsername, err := api.CurrentLoginName(cachingAPIClient, s.hostname()) @@ -208,10 +228,11 @@ func (s *StatusGetter) CurrentUsername() (string, error) { return "", fmt.Errorf("failed to get current username: %w", err) } + s.currentUsername = currentUsername return currentUsername, nil } -func (s *StatusGetter) ActualMention(n Notification) (string, error) { +func (s *StatusGetter) ActualMention(commentURL string) (string, error) { currentUsername, err := s.CurrentUsername() if err != nil { return "", err @@ -224,17 +245,15 @@ func (s *StatusGetter) ActualMention(n Notification) (string, error) { resp := struct { Body string }{} - if err := c.REST(s.hostname(), "GET", n.Subject.LatestCommentURL, nil, &resp); err != nil { + if err := c.REST(s.hostname(), "GET", commentURL, nil, &resp); err != nil { return "", err } - var ret string - if strings.Contains(resp.Body, "@"+currentUsername) { - ret = resp.Body + return resp.Body, nil } - return ret, nil + return "", nil } // These are split up by endpoint since it is along that boundary we parallelize @@ -249,16 +268,63 @@ func (s *StatusGetter) LoadNotifications() error { query.Add("participating", "true") query.Add("all", "true") + fetchWorkers := 10 + ctx, abortFetching := context.WithCancel(context.Background()) + defer abortFetching() + toFetch := make(chan Notification) + fetched := make(chan StatusItem) + + wg := new(errgroup.Group) + for i := 0; i < fetchWorkers; i++ { + wg.Go(func() error { + for { + select { + case <-ctx.Done(): + return nil + case n, ok := <-toFetch: + if !ok { + return nil + } + if actual, err := s.ActualMention(n.Subject.LatestCommentURL); actual != "" && err == nil { + // I'm so sorry + split := strings.Split(n.Subject.URL, "/") + fetched <- StatusItem{ + Repository: n.Repository.FullName, + Identifier: fmt.Sprintf("%s#%s", n.Repository.FullName, split[len(split)-1]), + preview: actual, + index: n.index, + } + } else if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == 403 { + s.addAuthError(httpErr.Message, factory.SSOURL()) + } else { + abortFetching() + return fmt.Errorf("could not fetch comment: %w", err) + } + } + } + } + }) + } + + doneCh := make(chan struct{}) + go func() { + for item := range fetched { + s.Mentions = append(s.Mentions, item) + } + close(doneCh) + }() + // this sucks, having to fetch so much :/ but it was the only way in my // testing to really get enough mentions. I would love to be able to just // filter for mentions but it does not seem like the notifications API can // do that. I'd switch to the GraphQL version, but to my knowledge that does // not work with PATs right now. - var ns []Notification - var resp []Notification - pages := 0 + nIndex := 0 p := fmt.Sprintf("notifications?%s", query.Encode()) - for pages < 3 { + for pages := 0; pages < 3; pages++ { + var resp []Notification next, err := c.RESTWithNext(s.hostname(), "GET", p, nil, &resp) if err != nil { var httpErr api.HTTPError @@ -266,52 +332,36 @@ func (s *StatusGetter) LoadNotifications() error { return fmt.Errorf("could not get notifications: %w", err) } } - ns = append(ns, resp...) + + for _, n := range resp { + if n.Reason != "mention" { + continue + } + if s.Org != "" && n.Repository.Owner.Login != s.Org { + continue + } + if s.ShouldExclude(n.Repository.FullName) { + continue + } + n.index = nIndex + nIndex++ + toFetch <- n + } if next == "" || len(resp) < perPage { break } - - pages++ p = next } - s.Mentions = []StatusItem{} - - for _, n := range ns { - if n.Reason != "mention" { - continue - } - - if s.Org != "" && n.Repository.Owner.Login != s.Org { - continue - } - - if s.ShouldExclude(n.Repository.FullName) { - continue - } - - if actual, err := s.ActualMention(n); actual != "" && err == nil { - // I'm so sorry - split := strings.Split(n.Subject.URL, "/") - s.Mentions = append(s.Mentions, StatusItem{ - Repository: n.Repository.FullName, - Identifier: fmt.Sprintf("%s#%s", n.Repository.FullName, split[len(split)-1]), - preview: actual, - }) - } else if err != nil { - var httpErr api.HTTPError - if !errors.As(err, &httpErr) || httpErr.StatusCode != 403 { - return fmt.Errorf("could not fetch comment: %w", err) - } else if httpErr.StatusCode == 403 { - s.authErrorsMu.Lock() - s.hasAuthErrors = true - s.authErrorsMu.Unlock() - } - } - } - - return nil + close(toFetch) + err := wg.Wait() + close(fetched) + <-doneCh + sort.Slice(s.Mentions, func(i, j int) bool { + return s.Mentions[i].index < s.Mentions[j].index + }) + return err } const searchQuery = ` @@ -335,18 +385,14 @@ fragment pr on PullRequest { } query AssignedSearch($searchAssigns: String!, $searchReviews: String!, $limit: Int = 25) { assignments: search(first: $limit, type: ISSUE, query: $searchAssigns) { - edges { - node { - ...issue - ...pr - } + nodes { + ...issue + ...pr } } reviewRequested: search(first: $limit, type: ISSUE, query: $searchReviews) { - edges { - node { - ...pr - } + nodes { + ...pr } } }` @@ -372,25 +418,29 @@ func (s *StatusGetter) LoadSearchResults() error { var resp struct { Assignments struct { - Edges []struct { - Node SearchResult - } + Nodes []*SearchResult } ReviewRequested struct { - Edges []struct { - Node SearchResult - } + Nodes []*SearchResult } } err := c.GraphQL(s.hostname(), searchQuery, variables, &resp) if err != nil { - // Exclude any FORBIDDEN errors and show status for what we can. - if gqlErrResponse, ok := err.(*api.GraphQLErrorResponse); ok { - err = filterGraphQLErrors(gqlErrResponse) - if err == nil { - s.authErrorsMu.Lock() - s.hasAuthErrors = true - s.authErrorsMu.Unlock() + var gqlErrResponse *api.GraphQLErrorResponse + if errors.As(err, &gqlErrResponse) { + gqlErrors := make([]api.GraphQLError, 0, len(gqlErrResponse.Errors)) + // Exclude any FORBIDDEN errors and show status for what we can. + for _, gqlErr := range gqlErrResponse.Errors { + if gqlErr.Type == "FORBIDDEN" { + s.addAuthError(gqlErr.Message, factory.SSOURL()) + } else { + gqlErrors = append(gqlErrors, gqlErr) + } + } + if len(gqlErrors) == 0 { + err = nil + } else { + err = &api.GraphQLErrorResponse{Errors: gqlErrors} } } if err != nil { @@ -402,21 +452,25 @@ func (s *StatusGetter) LoadSearchResults() error { issues := []SearchResult{} reviewRequested := []SearchResult{} - for _, e := range resp.Assignments.Edges { - if e.Node.Type == "Issue" { - issues = append(issues, e.Node) - } else if e.Node.Type == "PullRequest" { - prs = append(prs, e.Node) - } else if e.Node.Type != "" { - // FORBIDDEN nodes are null in response, so Type == "" here. - panic("you shouldn't be here") + for _, node := range resp.Assignments.Nodes { + if node == nil { + continue // likely due to FORBIDDEN results + } + switch node.Type { + case "Issue": + issues = append(issues, *node) + case "PullRequest": + prs = append(prs, *node) + default: + return fmt.Errorf("unkown Assignments type: %q", node.Type) } } - for _, e := range resp.ReviewRequested.Edges { - if e.Node.Type != "" { - reviewRequested = append(reviewRequested, e.Node) + for _, node := range resp.ReviewRequested.Nodes { + if node == nil { + continue // likely due to FORBIDDEN results } + reviewRequested = append(reviewRequested, *node) } sort.Sort(Results(issues)) @@ -454,19 +508,6 @@ func (s *StatusGetter) LoadSearchResults() error { return nil } -func filterGraphQLErrors(gqlErrResponse *api.GraphQLErrorResponse) error { - gqlErrors := make([]api.GraphQLError, 0, len(gqlErrResponse.Errors)) - for _, gqlErr := range gqlErrResponse.Errors { - if gqlErr.Type != "FORBIDDEN" { - gqlErrors = append(gqlErrors, gqlErr) - } - } - if len(gqlErrors) > 0 { - return &api.GraphQLErrorResponse{Errors: gqlErrors} - } - return nil -} - // Populate .RepoActivity func (s *StatusGetter) LoadEvents() error { perPage := 100 @@ -545,11 +586,25 @@ func (s *StatusGetter) LoadEvents() error { return nil } +func (s *StatusGetter) addAuthError(msg, ssoURL string) { + s.authErrorsMu.Lock() + defer s.authErrorsMu.Unlock() + + if s.authErrors == nil { + s.authErrors = set.NewStringSet() + } + + s.authErrors.Add(msg) + if ssoURL != "" { + s.authErrors.Add(fmt.Sprintf("Authorize in your web browser: %s", ssoURL)) + } +} + func (s *StatusGetter) HasAuthErrors() bool { s.authErrorsMu.Lock() defer s.authErrorsMu.Unlock() - return s.hasAuthErrors + return s.authErrors != nil && s.authErrors.Len() > 0 } func statusRun(opts *StatusOptions) error { @@ -568,31 +623,26 @@ func statusRun(opts *StatusOptions) error { // TODO break out sections into individual subcommands g := new(errgroup.Group) + g.Go(func() error { + if err := sg.LoadNotifications(); err != nil { + return fmt.Errorf("could not load notifications: %w", err) + } + return nil + }) + g.Go(func() error { + if err := sg.LoadEvents(); err != nil { + return fmt.Errorf("could not load events: %w", err) + } + return nil + }) + g.Go(func() error { + if err := sg.LoadSearchResults(); err != nil { + return fmt.Errorf("failed to search: %w", err) + } + return nil + }) + opts.IO.StartProgressIndicator() - g.Go(func() error { - err := sg.LoadNotifications() - if err != nil { - err = fmt.Errorf("could not load notifications: %w", err) - } - return err - }) - - g.Go(func() error { - err := sg.LoadEvents() - if err != nil { - err = fmt.Errorf("could not load events: %w", err) - } - return err - }) - - g.Go(func() error { - err := sg.LoadSearchResults() - if err != nil { - err = fmt.Errorf("failed to search: %w", err) - } - return err - }) - err = g.Wait() opts.IO.StopProgressIndicator() if err != nil { @@ -675,7 +725,9 @@ func statusRun(opts *StatusOptions) error { fmt.Fprintln(out, raSection) if sg.HasAuthErrors() { - fmt.Fprintln(out, cs.Gray("Some status unavailable due to organization SAML enforcement. Run `gh auth refresh` to authorization access to organizations.")) + for _, msg := range sg.authErrors.ToSlice() { + fmt.Fprintln(out, cs.Gray(fmt.Sprintf("warning: %s", msg))) + } } return nil diff --git a/pkg/cmd/status/status_test.go b/pkg/cmd/status/status_test.go index a845cab90..816034808 100644 --- a/pkg/cmd/status/status_test.go +++ b/pkg/cmd/status/status_test.go @@ -2,10 +2,13 @@ package status import ( "bytes" + "io/ioutil" "net/http" + "strings" "testing" "time" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" @@ -96,7 +99,7 @@ func TestStatusRun(t *testing.T) { httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("AssignedSearch"), - httpmock.StringResponse(`{"data": { "assignments": {"edges": [] }, "reviewRequested": {"edges": []}}}`)) + httpmock.StringResponse(`{"data": { "assignments": {"nodes": [] }, "reviewRequested": {"nodes": []}}}`)) reg.Register( httpmock.REST("GET", "notifications"), httpmock.StringResponse(`[]`)) @@ -110,18 +113,6 @@ func TestStatusRun(t *testing.T) { { name: "something", httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("UserCurrent"), httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) @@ -140,9 +131,6 @@ func TestStatusRun(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("AssignedSearch"), httpmock.FileResponse("./fixtures/search.json")) @@ -159,15 +147,6 @@ func TestStatusRun(t *testing.T) { { name: "exclude a repository", httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("UserCurrent"), httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) @@ -183,9 +162,6 @@ func TestStatusRun(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("AssignedSearch"), httpmock.FileResponse("./fixtures/search.json")) @@ -212,9 +188,6 @@ func TestStatusRun(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("AssignedSearch"), httpmock.FileResponse("./fixtures/search.json")) @@ -235,12 +208,6 @@ func TestStatusRun(t *testing.T) { { name: "filter to an org", httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("UserCurrent"), httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) @@ -253,9 +220,6 @@ func TestStatusRun(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/rpd/todo/issues/comments/1065"), httpmock.StringResponse(`{"body":"not a real mention"}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("AssignedSearch"), httpmock.FileResponse("./fixtures/search.json")) @@ -274,26 +238,22 @@ func TestStatusRun(t *testing.T) { { name: "forbidden errors", httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("UserCurrent"), httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.REST("GET", "repos/rpd/todo/issues/110"), - httpmock.StatusStringResponse(403, `{ - "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." - }`)) + func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: 403, + Body: ioutil.NopCloser(strings.NewReader(`{"message": "You don't have permission to access this resource."}`)), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "X-Github-Sso": []string{"required; url=http://click.me/auth"}, + }, + }, nil + }) reg.Register( httpmock.REST("GET", "repos/rpd/todo/issues/4113"), httpmock.StringResponse(`{"body":"this is a comment"}`)) @@ -306,9 +266,6 @@ func TestStatusRun(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("AssignedSearch"), httpmock.FileResponse("./fixtures/search_forbidden.json")) @@ -319,15 +276,28 @@ func TestStatusRun(t *testing.T) { httpmock.REST("GET", "users/jillvalentine/received_events"), httpmock.FileResponse("./fixtures/events.json")) }, - opts: &StatusOptions{}, - wantOut: "Assigned Issues │ Assigned Pull Requests \nvilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions \n │ \nReview Requests │ Mentions \ncli/cli#5272 Pin extensions │ cli/cli#1096 @jillval...\n │ vilmibm/gh-screensaver#15 a messag...\n \nRepository Activity\nrpd/todo#5326 new PR Only write UTF-8 BOM on W...\nvilmibm/testing#5325 comment on Ability to sea... We are working on dedicat...\ncli/cli#5319 comment on [Codespaces] D... Wondering if we shouldn't...\ncli/cli#5300 new issue Terminal bell when a runn...\n\nSome status unavailable due to organization SAML enforcement. Run `gh auth refresh` to authorization access to organizations.\n", + opts: &StatusOptions{}, + wantOut: heredoc.Doc(` + Assigned Issues │ Assigned Pull Requests + vilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions + │ + Review Requests │ Mentions + cli/cli#5272 Pin extensions │ cli/cli#1096 @jillval... + │ vilmibm/gh-screensaver#15 a messag... + + Repository Activity + rpd/todo#5326 new PR Only write UTF-8 BOM on W... + vilmibm/testing#5325 comment on Ability to sea... We are working on dedicat... + cli/cli#5319 comment on [Codespaces] D... Wondering if we shouldn't... + cli/cli#5300 new issue Terminal bell when a runn... + + warning: Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization. + warning: You don't have permission to access this resource. + `), }, { name: "notification errors", httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL("UserCurrent"), - httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) reg.Register( httpmock.GraphQL("UserCurrent"), httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) @@ -338,7 +308,7 @@ func TestStatusRun(t *testing.T) { }`)) reg.Register( httpmock.GraphQL("AssignedSearch"), - httpmock.StringResponse(`{"data": { "assignments": {"edges": [] }, "reviewRequested": {"edges": []}}}`)) + httpmock.StringResponse(`{"data": { "assignments": {"nodes": [] }, "reviewRequested": {"nodes": []}}}`)) reg.Register( httpmock.REST("GET", "notifications"), httpmock.StringResponse(`[ @@ -376,7 +346,7 @@ func TestStatusRun(t *testing.T) { httpmock.StringResponse(`{ "data": { "assignments": { - "edges": [ + "nodes": [ null ] } @@ -386,7 +356,7 @@ func TestStatusRun(t *testing.T) { "type": "NOT FOUND", "path": [ "assignments", - "edges", + "nodes", 0 ], "message": "Not found." @@ -401,7 +371,7 @@ func TestStatusRun(t *testing.T) { httpmock.StringResponse(`[]`)) }, opts: &StatusOptions{}, - wantErrMsg: "failed to search: could not search for assignments: GraphQL: Not found. (assignments.edges.0)", + wantErrMsg: "failed to search: could not search for assignments: GraphQL: Not found. (assignments.nodes.0)", }, } From c8baec74c0ef28bb14532a4576096165f3cd7c07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 10 May 2022 16:05:27 +0200 Subject: [PATCH 3/3] Ensure stable presentation of FORBIDDEN errors --- pkg/cmd/status/status.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/status/status.go b/pkg/cmd/status/status.go index 45c3bde77..9aaefdf9c 100644 --- a/pkg/cmd/status/status.go +++ b/pkg/cmd/status/status.go @@ -594,9 +594,10 @@ func (s *StatusGetter) addAuthError(msg, ssoURL string) { s.authErrors = set.NewStringSet() } - s.authErrors.Add(msg) if ssoURL != "" { - s.authErrors.Add(fmt.Sprintf("Authorize in your web browser: %s", ssoURL)) + s.authErrors.Add(fmt.Sprintf("%s\nAuthorize in your web browser: %s", msg, ssoURL)) + } else { + s.authErrors.Add(msg) } } @@ -725,7 +726,9 @@ func statusRun(opts *StatusOptions) error { fmt.Fprintln(out, raSection) if sg.HasAuthErrors() { - for _, msg := range sg.authErrors.ToSlice() { + errs := sg.authErrors.ToSlice() + sort.Strings(errs) + for _, msg := range errs { fmt.Fprintln(out, cs.Gray(fmt.Sprintf("warning: %s", msg))) } }