Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usage of pull request timeline API #54

Merged
merged 3 commits into from
Mar 13, 2019

Conversation

bluekeyes
Copy link
Member

@bluekeyes bluekeyes commented Mar 12, 2019

We originally used this API to have consistent ordering between comments, commits, and reviews. However, this didn't respect commit push times, so we switched to using timestamps for ordering with the switch to GraphQL. Later, we discovered that the timeline API has a random delay before commits, particularly from forks, appear in the result. This causes the bot to evaluate the status for the wrong commit, breaking the invalidate_on_push option.

Instead, get commits, comments, and reviews from the pull request API, which should be up-to-date when we respond to a pull_request hook. Because all three of these sources are used in most evaluations, continue to load them with a single query instead of making separate queries for each type.

See #47 for more details. This will hopefully fix the problem, but I'll submit a separate PR that adds a check for the head commit.

We originally used this API to have consistent ordering between
comments, commits, and reviews. However, this didn't respect commit push
times, so we switched to using timestamps for ordering with the switch
to GraphQL. Later, we discovered that the timeline API has a random
delay before commits, particularly from forks, appear in the result.
This causes the bot to evaluate the status for the wrong commit,
breaking the invalidate_on_push option.

Instead, get commits, comments, and reviews from the pull request API,
which should be up-to-date when we respond to a pull_request hook.
Because all three of these sources are used in most evaluations,
continue to load them with a single query instead of making separate
queries for each type.
@bluekeyes
Copy link
Member Author

This also updates stretchr/testify to fix a bug that caused failed assertions to panic instead of printing a diff in certain cases.

@bluekeyes bluekeyes merged commit 3b4a822 into develop Mar 13, 2019
@bluekeyes bluekeyes deleted the bkeyes/remove-timeline-api branch March 13, 2019 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants