search

servo / highfive

Github hooks to provide an encouraging atmosphere for new contributors
Mozilla Public License 2.0
254 stars 58 forks source link

Hit GitHub until it returns a PR's mergeability on synchronize #75

Closed magni- closed 8 years ago

magni- commented 8 years ago

This fixes an issue with the removing the S-needs-rebase label when getting a synchronize event from GitHub.

GitHub doesn't wait to calculate mergeability when firing off a webhook, instead they send mergeable as null. In that happens, with this PR we now hit GitHub again until they return the mergeable field as either true or false.

Two questions: 1) What's an appropriate wait time in between GETs? 2) How do I test this? Is there a way to specify multiple API response payloads?

highfive commented 8 years ago

Heads up! This PR modifies the following files:

jdm commented 8 years ago

Thank you for doing this work! I haven't been able to come up with a better solution, but let's move to a 1s delay. I'm also tempted to limit the scope of get_pull by renaming it is_pull_request_mergeable. This also makes it easier to write a test; we can just make is_pull_request_mergeable return true in the test API, and have the test JSON contain mergeable: null, then verify that the label is removed.

magni- commented 8 years ago

I went ahead and changed the delay, but kept the method to the more general get_pull.

I also figured out how to write the test (should have looked more closely at test.py the first time around). Good thing I did, since it caught the missing import! (seems like a lint check could come in handy for that though... :smile: )

jdm commented 8 years ago

Thanks!