GHE 2.20 compatibility for pr commands - #2035
Conversation
… GHE We first ask the GHE server for whether it supports these fields.
It looks like GHE v2.20 does not support `@me` in search yet.
This speeds up `pr`-related commands for GHE by caching schema introspection queries for 24h.
|
Built this locally, confirmed that |
I have no idea what's going on there, so I'll just give up the streaming approach and read the entire contents of the cache file to memory. https://github.com/cli/cli/pull/2035/checks?check_run_id=1194798056
| bodyCopy := &bytes.Buffer{} | ||
| defer req.Body.Close() | ||
| _, err := io.Copy(h, io.TeeReader(req.Body, bodyCopy)) | ||
| req.Body = ioutil.NopCloser(bodyCopy) |
There was a problem hiding this comment.
Out of curiosity, what is the purpose of this line, and the similar line in writeCache?
There was a problem hiding this comment.
The caching layer is essentially a middleware. This method needs to read the entire request body to compute the signature of the request, but then req.Body would have already been exhausted from this point onward and nothing else could read from it. This code essentially "rewinds" an io.Reader by replacing it with another Reader that can be read again from the start.
Maybe I should wrap this in a named helper function so the intent of the logic is clearer 👍
| } | ||
| ` | ||
|
|
||
| if currentUsername == "@me" && ghinstance.IsEnterprise(repo.RepoHost()) { |
There was a problem hiding this comment.
Is there a reason why we only want to evaluate this on GHES?
There was a problem hiding this comment.
Yes: we know that github.com already supports @me and thus we can avoid an overhead of the extra request. I'm not sure which GHE version adds support for @me, but I have confirmed that GHE v2.20 doesn't.
- make thread-safe - only cache GET, HEAD, and GraphQL requests - only cache non-5xx, non-403 responses - include `Accept` and `Authorization` headers in cache key
vilmibm
left a comment
vilmibm
left a comment
There was a problem hiding this comment.
I found the caching stuff easy to follow and it seemed good to me; the resulting complexity for checking feature support isn't so bad. Thanks!
4 participants
Fixes
gh pr statusgh pr viewgh pr createfor GHE versions where
statusCheckRolluporreviewDecisionare unavailable.To avoid the overhead of GraphQL schema introspection queries every time, their result is cached on disk per-host for 24 hours.
Fixes #1709, closes #1102