Checks status hub pr list #2361

joshuabezaleel · 2019-11-15T06:17:01Z

Add check status for pull requests list on the command of hub pr list
Add query.go for constructing the particular GraphQL query using graphb.
Screenshot attached below.

I am so sorry beforehand if I got the design wrong in any way. Looking forward to the review! 🙂

mislav

Nice going! This is a great start.

One thing you can do here is to construct the GraphQL query by hand instead of using the library. I think it would be simpler, and we avoid one extra dependency. I've left some notes about that.

Something that is missing from this implementation is that it only queries Statuses but not Checks. This might be a little confusing, but GitHub offers two different mechanisms to attach a success/failure status to commits and PRs: Statuses (legacy) and Checks (for GitHub Apps). Checks are more full-featured, but require access by separate APIs. Unfortunately, querying Checks is still a bit complicated over GraphQL, and there might be some API updates on our end to make it simpler, so I don't want you to worry about this now, as you already did a great job covering Statuses. When I find time, I can add Checks support on top of your Statuses implementation.

Great job! Thank you for working on this.

mislav · 2019-11-16T11:06:21Z

commands/pr.go

@@ -234,14 +238,25 @@ func listPulls(cmd *Command, args *Args) {
 	flagPullRequestLimit := args.Flag.Int("--limit")
 	flagPullRequestFormat := args.Flag.Value("--format")
 	if !args.Flag.HasReceived("--format") {
-		flagPullRequestFormat = "%pC%>(8)%i%Creset  %t%  l%n"
+		flagPullRequestFormat = "%pC%>(8)%i%Creset  %t %cC%cs %l%n"


I'm not sure if we should change the default format string to include this just yet. Maybe it should remain opt-in? Anyway, you don't have to change this right away, let me think about this for a bit.

mislav · 2019-11-16T11:06:21Z

commands/pr.go

 	}

 	pulls, err := gh.FetchPullRequests(project, filters, flagPullRequestLimit, func(pr *github.PullRequest) bool {
 		return !(onlyMerged && pr.MergedAt.IsZero())
 	})
 	utils.Check(err)

+	checkStatus, err := gh.FetchPullRequestsCheckStatus(project, filters, flagPullRequestLimit)


It would be ideal if we avoided making this extra API request unless the format flag listed that it wants these fields. I don't have a good idea how to do that yet (we don't have a mechanism to ask for which fields are listed in --format); just thinking out loud

I assume that this review and the previous review are talking on the same subject of whether we should make the status to be opted-in so that either the API call is executed and/or the format is changed when the user stated that they want to see the status in the --format input, right? Any further information on this? 😁

Any further info on this, @mislav ? 😁
Should we implement Status in the form of --format flag option to then be able to:

Modify the flagPullRequestFormat and,

Also make the API request call to fetchStatus become optional

Looking forward to hearing from you! 🙂

mislav · 2019-11-16T11:06:21Z

github/client.go

+		return
+	}
+
+	response, err := api.PostJSONPreview(fmt.Sprintf("/graphql"), query, checksType)


fmt.Sprintf("/graphql") can just be "graphql" (note that you should drop the leading / so this can work with GitHub Enterprise as well)

Will fix this. Thank you!

mislav · 2019-11-16T11:06:21Z

github/query.go

+import (
+	"encoding/json"
+
+	"github.com/azaky/graphb"


I'm wondering if we can avoid pulling in an extra dependency to construct the GraphQL query?

After all, the only thing we need to do is POST some JSON to the graphql endpoint. The JSON payload can be constructed like this:

variables := map[string]interface{}{ "state": "open", } payload := map[string]interface{}{ "variables": variables, "query": ` query(...) { repository(...) { pullRequests(...) { ... } } }`, } // later: api.PostJSONPreview("/graphql", payload, checksType)

I think it would be even simpler and more readable to write the query string by hand like that instead of using the library. In the future, when maintaining GraphQL queries becomes numerous and more cumbersome, we can consider using a library.

Sure, we can 😁 . I just thought that there will be more GraphQL queries than only this one and it will be bloated for other developers if we are all using plain string but let's tackle that later. I am going to drop this dependency.

joshuabezaleel · 2019-12-04T06:56:30Z

Hi @mislav , truly sorry for a really late reply 😞 .
First of all, I just want to say that I am blown away and thank you very much for the really kind reply 🙂 .

Yeah, I noticed it before that there are 2 ways of getting status which the first is Status and the second is Check, which is represented in the GraphQL query that you mentioned in the issue before, right? Thus if it's okay with you, I have several questions on this.

Wouldn't all GitHub repositories with CI installed will always have Status value but not always have Checks status value from GitHub Apps installed? and,
I thought that if there are GitHub apps installed and it would return Checks value, the value of the Checks will always be the same and reflected by the Status value? So that it's sufficient to retrieve the Status value only.
And if we are going to retrieve both of the two values from Status and also Checks, which value are we going to use? In case it would return different value.

I am really sorry beforehand if it's because I have a wrong understanding on this Status and Checks. I will reply on the remaining reviews below. Looking forward to your reply and hope you have a great week ahead! 🙂

mislav · 2019-12-04T10:03:45Z

I am really sorry beforehand if it's because I have a wrong understanding on this Status and Checks.

No need to apologize, this stuff isn't really easy, and our documentation doesn't explain the differences between Statuses and Checks very well. Our GraphQL documentation for Checks is also lacking since that API is in preview phase.

This is what it all comes down to:

Statuses were the original CI for GitHub
Checks are the new generation of Statuses and come from GitHub Apps
A repository can be configured with any combination of Checks and Statuses
Checks right now are complex to fetch over GraphQL.

For the simplicity of this PR, I would like you to focus on Statuses only, and I can then add Checks support as follow-up. 🙇

mislav · 2019-12-04T10:05:22Z

@joshuabezaleel For an example of how to perform GraphQL queries in hub, here is a PR that adds some simple queries in a way that doesn't require an external library #2363

mislav · 2019-12-16T00:55:21Z

I have pushed some changes, mainly to:

compose GraphQL requests directly without the graphb library - this utilizes the Client.GraphQL function that is new in master
restore default --format to original - I feel like this feature is still premature to be enabled by default
avoid fetching statuses via GraphQL unless %cs/%cC were explicitly specified via --format
support --limit values > 100

@joshuabezaleel Please review! 🙇

I'm still not sure whether this feature is ready to merge, though:

It only fetches Statuses so far and not Checks (to reiterate: I will take on implementing Checks when I find time, so you don't have to worry about that);
It fetches pull requests double: once over REST (like before) and another time over GraphQL (to fetch Statuses). Ideally it would only fetch them once, so maybe we could switch to GraphQL entirely for the case when Statuses are needed.
--sort long-running is supported over REST API but not over GraphQL.

XVilka · 2020-04-14T08:16:58Z

Are there any updates regarding this PR? It needs to be rebased.

mislav · 2020-04-14T09:33:58Z

@XVilka @joshuabezaleel The GitHub API in the meantime had some updates to make querying Checks + Statuses easier! I plan to update this PR incorporating those changes when I find time.

…r-list

dawsbot · 2020-08-12T17:36:40Z

I would really love this feature. Is anyone working on this still?

mislav · 2020-08-13T11:50:33Z

@dawsbot I haven't worked on it since my last comment, but since then there is a new GraphQL statusCheckRollup API that combines information about both Checks and Statuses and their resolution, so there's less work on the client to assemble this data. I plan to adopt it in this PR when I find time. Or, in the meantime, anyone is welcome to base their work off of my branch! 🙇

joshuabezaleel force-pushed the checks-status-hub-pr-list branch 2 times, most recently from 762fe3c to 1091565 Compare Nov 15, 2019

mislav requested changes Nov 16, 2019

View changes

bhrutledge mentioned this pull request Dec 9, 2019

Add a pr list format string for review status #2386

Open

joshuabezaleel and others added 3 commits Dec 15, 2019

Add check status for command hub pr list

f7838fb

[pr] Fetch build statuses without external GraphQL library

b0504e3

[pr] Fetch statuses only if explicitly requested via format string

430a50c

mislav force-pushed the checks-status-hub-pr-list branch from 4d24590 to 430a50c Compare Dec 15, 2019

mislav added 2 commits Dec 16, 2019

[pr] Fix GraphQL pagination

a4baf0f

[pr] Enable GraphQL querying by base or head

77d7ee9

This comment has been minimized.

Sign in to view

mislav added 2 commits Apr 14, 2020

Merge remote-tracking branch 'origin/master' into checks-status-hub-p 1�7

c530b16

…r-list

go mod tidy

2725f45

mislav force-pushed the checks-status-hub-pr-list branch from cfe1975 to 2725f45 Compare Apr 14, 2020

Checks status hub pr list #2361

Checks status hub pr list #2361

joshuabezaleel commented Nov 15, 2019

mislav left a comment

mislav Nov 16, 2019

mislav Nov 16, 2019

joshuabezaleel Dec 4, 2019

joshuabezaleel Dec 8, 2019

mislav Nov 16, 2019

joshuabezaleel Dec 4, 2019

mislav Nov 16, 2019

joshuabezaleel Dec 4, 2019

joshuabezaleel commented Dec 4, 2019

mislav commented Dec 4, 2019

mislav commented Dec 4, 2019

mislav commented Dec 16, 2019

This comment has been minimized.

XVilka commented Apr 14, 2020

mislav commented Apr 14, 2020

dawsbot commented Aug 12, 2020

mislav commented Aug 13, 2020

Checks status hub pr list #2361

Are you sure you want to change the base?

Checks status hub pr list #2361

Conversation

joshuabezaleel commented Nov 15, 2019

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuabezaleel commented Dec 4, 2019

mislav commented Dec 4, 2019

mislav commented Dec 4, 2019

mislav commented Dec 16, 2019

This comment has been minimized.

XVilka commented Apr 14, 2020

mislav commented Apr 14, 2020

dawsbot commented Aug 12, 2020

mislav commented Aug 13, 2020