Fix invalidate_on_push, use GraphQL for timeline #37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, you could bypass the invalidate_on_push option by faking commit times. The v4 API exposes push times as a property, so switch to using that for timeline requests. This also simplifies loading commits, since it can be done in a single call now.
Make ResponsePlayer more flexible by introducing a matcher interface. Add an exact path matcher for the old behavior and a GraphQL matcher that parses queries and responds if the query contains a given node path prefix.
bluekeyes
changed the title
asvoboda
reviewed
Dec 6, 2018
| HasNextPage bool | ||
| } | ||
| Nodes []*timelineEvent | ||
| } `graphql:"timeline(first: 100, after: $cursor)"` |
There was a problem hiding this comment.
It's an arbitrary page size, but 100 is the maximum allowed by GitHub.
asvoboda
reviewed
Dec 6, 2018
| // Backfill this to all other commits for the purpose of sorting | ||
| var lastPushed *time.Time | ||
| for i := len(allEvents) - 1; i >= 0; i-- { | ||
| if event := allEvents[i]; event.Type == "Commit" { |
There was a problem hiding this comment.
Should we use timelineevent#CreatedAt() here?
There was a problem hiding this comment.
I don't think so - this loop fills in the underlying property that makes CreatedAt() function correctly for commit events, so I think the clarity of using the low-level property is desired.
|
I just realized that with the addition of valid timestamps on everything, we can probably remove the |
Now that everything has timestamps, compare using those.
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary point of this PR is to fix a bug where commits are ordered by commit time (which is easily forged by clients) instead of push time. As a result, the
invalidate_on_pushoption is currently a suggestion rather than a guarantee.To fix this, we need the
pushedDate, which as far as I can tell is only exposed in the v4 GraphQL API. The second part of the PR convertspull.GitHubContextto use GraphQL. This coincidentally cleans up some commit fetching logic, since we can get all the information at once now. Unfortunately, we still need the v3 client because PR file information is still in preview and even that isn't in GHE 2.15.x.The final part of this change is updating the tests to work with GraphQL. This required rewriting how our mock
http.Transportworked.Some points for discussion:
I pass two clients around everywhere. Would it be better to define an aggregate struct?
All the testing tools are defined in the test package via
*_test.gofiles. Should this be extracted to a real package? If so, where? I'm not sure it's mature enough to move togo-githubapp.I pull in a GraphQL parsing library to implement the GraphQL matcher. This is potentially overkill. Is there another equally safe way to match on GraphQL query bodies?
Fixes #9.