Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGHE compatibility for `pr list/status` #1102
Conversation
|
Thanks for exploring this. It's a creative solution to a hard problem and the resulting code could certainly be a lot more complex. I do think this will be difficult to maintain but no more so than any similar approach; I really appreciate how this frees us from having to actually track what is available in each Enterprise release. I also assume that the minority of our queries will ultimately require the feature detection dance. I'm
Do you have an example of what that might look like?
Caching makes sense; is there a simpler compromise here of not bothering with feature detection if we notice we're on
The rest of this approach could be merged though, right? |
I'm a little worried about this. It's probably OK, but it is noticeable and annoying. Caching the result would work, but I'm not sure when we would invalidate the cache?
This isn't a gnarly as I thought it would be! I was trying to figure out if there was a way to pull out the feature detection code into a separate function that would be easy to run for other commands. But it looks like each call will be pretty specific? But maybe pulling out the feature detection code for each function into a new function would make it a little easier to follow? There are a lot of graphl query structs in each function! |
| jsonFile, _ := os.Open("../test/fixtures/prStatus.json") | ||
| defer jsonFile.Close() | ||
| http.StubResponse(200, jsonFile) | ||
| http.Register(httpmock.GraphQL(`\b__type\b`), httpmock.FileResponse("../test/fixtures/prIntrospection.json")) |
This comment has been minimized.
This comment has been minimized.
probablycorey
Jun 4, 2020
Contributor
What does that \b do? I'm assuming it's a regex word boundary?
@vilmibm No, but that's a good question. I suspect we might need to either: 1) fork the library or 2) generate structs dynamically using Go's
That's not a bad idea, but we have to consider the fact that, before firing off a request, we don't know whether an authentication token belongs to the "GitHub CLI" OAuth app or is a Personal Access Token. PATs don't have access to feature flags, so we still get slightly different versions of the API even while on That's the main reason why I would vote for doing feature detection always, but caching it per hashed combo of host+token.
@probablycorey I think caching the results of feature detection queries for 24h would be the sweet-spot! It's just long enough to not feel it persistently during a day, but just short enough to be able to get new features if
That's a good call! I will clean this up once this PR is unblocked on the API front. |
|
@mislav do you still intend to see this one through or can it be closed? |
|
@vilmibm I still intend to see this one through! |
mislav commentedJun 4, 2020
•
edited
This uses GraphQL introspection to query the API functionality of the server and adjust the query accordingly (i.e. never include fields or filters that are unsupported). This approach allows us to conditionally adopt new or feature-flagged APIs in CLI without breaking compatibility with GitHub instances that don't offer the same APIs.
This PR addresses:
pr listassigneeandauthorfiltering viapullRequests.filterByfeature-flagged API #641assigneeisDraftif it's not availablepr statusisDraftif it's not availablereviewDecisioninfo if not availablestatusCheckRollupinfo if not availableMy findings and feelings:
githubv4GraphQL libraryIf we were to go this way, than the remaining API surface to cover wouldn't be large:
draftinput topr createunless it's supportedTo resolve before this can be merged:
pr list/status? should we cache the result locally?pullRequests.filterByAPI isn't yet ready for adoption due to inconsistencies with different filtering combinationsRef. #823 (comment) #273