Adjustment to catch and raise merge conflicts #68
Conversation
jakirkham
force-pushed
the
adj_catch_merge_conflicts
branch
from
3557f8f
to
0a60908
Compare
| pull_request = repo.get_pull(pr_id) | ||
| mergeable = None | ||
| while mergeable is None: | ||
| time.sleep(0.1) |
There was a problem hiding this comment.
Go ahead and sleep to start with. This simplifies the logic of this loop and gives GitHub a chance to compute the mergeability of the PR before we request it.
There was a problem hiding this comment.
Also, not sure what a reasonable timeout is between checks, but figured 0.1s would be a good starting place.
There was a problem hiding this comment.
Should add that mergeable appears to be undefined if the pull request is closed as well. So there is a pretty rare case that could occur where we get here and the pull request is closed. This would result in an infinite loop. We can solve that a couple of ways.
- Check whether the PR is still open.
- Retry up to some limit.
My preference is to go for 1. Though maybe there is value in doing 2 as well. We would probably want to not have a message in that case. Currently we would get the merge conflict message. Feedback appreciated.
There was a problem hiding this comment.
We now handle 1. This functionality is also broken out in PR ( #70 ), but had to be merged back into this PR to handle merge conflicts.
| @@ -38,7 +43,7 @@ def compute_lint_message(repo_owner, repo_name, pr_id): | |||
| sha = str(repo.head.object.hexsha) | |||
|
|
|||
| # Raise an error if the PR is not mergeable. | |||
| if not pull_request.mergeable: | |||
| if not mergeable: | |||
There was a problem hiding this comment.
Switched to using the stored result from our checks for mergeability above.
jakirkham
force-pushed
the
adj_catch_merge_conflicts
branch
2 times, most recently
from
21842af
to
6186ca4
Compare
jakirkham
force-pushed
the
adj_catch_merge_conflicts
branch
from
6186ca4
to
8ab34d3
Compare
jakirkham
force-pushed
the
adj_catch_merge_conflicts
branch
from
8ab34d3
to
597e77b
Compare
jakirkham
force-pushed
the
adj_catch_merge_conflicts
branch
from
597e77b
to
3b0036a
Compare
| time.sleep(0.1) | ||
| pull_request = remote_repo.get_pull(pr_id) | ||
| if pull_request.state != "open": | ||
| return {} |
There was a problem hiding this comment.
Skip linting if the PR turns out to be closed at any point.
|
Merging/deploying so we can get a fix out for this issue ASAP. |
|
Please also look over this @pelson to make sure I haven't missed anything or if there are any tweaks you would recommend. |
|
Tried to retrigger this issue by closing and reopening PRs (caused the issue somewhat reliably before) and did not encounter it in any of the 8 times this was done with these changes. Think we have done a reasonable job at correcting this issue. Hopefully we don't see it resurface. |
|
Also am seeing this triggered correctly when there is a merge conflict. |
Fixes #67
This is a tweak to PR ( #55 ), which add the ability to raise merge conflicts. In some cases, it appears that mergeability is indeterminate. If mergeability is undetermined, we wait a little bit and recheck until we get some kind of result. This should avoid some false positives.