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

Contributors ignored during a single evaluation #63

Closed
iamdanfox opened this issue Apr 15, 2019 · 4 comments · Fixed by #68
Closed

Contributors ignored during a single evaluation #63

iamdanfox opened this issue Apr 15, 2019 · 4 comments · Fixed by #68
Labels
bug Something isn't working

Comments

@iamdanfox
Copy link

iamdanfox commented Apr 15, 2019

Edit: see comment for a summary of the underlying bug.


I left a comment on a single line of this PR (palantir/gradle-conjure#136), but it seems it caused the entire thing to merge.

Screenshot 2019-04-15 at 13 15 10

Interestingly, the UI seems to still say the PR has not yet been approved, but the status check went green so bulldozer mnerged it.

Screenshot 2019-04-15 at 11 57 15

@asvoboda
Copy link
Member

I think this is a dupe of #52.

@iamdanfox
Copy link
Author

No I actually think this is a separate issue. I interpret #52 as user confusion that a PR-level comment containing the string :+1: results in approving the PR.

My issue here is that a comment on a single line of a diff caused the entire PR to be approved.

@bluekeyes bluekeyes removed the duplicate This issue or pull request already exists label Apr 15, 2019
@bluekeyes
Copy link
Member

Your comment only triggered an evaluation of the pull request. Review comments (i.e. comments that are part of a review or are attached to a specific line of the diff) are not loaded or considered by policy-bot and the logs confirm that you were not considered as an approver.

What actually happened is that policy-bot did not consider @dansanduleac a contributor for that one evaluation and counted his original approval as fulfilling the repo owner approval rule. This is clearly a bug, as future evaluations from the details page show the state as pending.

I will try to figure out what caused this, but it's likely related to the commit loading changes in 1.7.0. That version has already been rolled back to 1.6.1 due to another problem.

@bluekeyes bluekeyes changed the title Comments on a line of code should not be counted as approval of entire PR Contributors ignored during a single evaluation Apr 16, 2019
@bluekeyes bluekeyes added the bug Something isn't working label Apr 16, 2019
@bluekeyes
Copy link
Member

I believe the root cause of this problem is that the new commit loading in 1.7.0 uses the commit count reported as part of the pull request object to know how many commits to list, but pull_request_review events (and only these events as far as I can tell) include a partial pull request object that is inexplicably missing this field (despite it appearing in the documentation). That leads us to list zero commits and lose all contributor information.

If the policy used the invalidate_on_push option, this would have caused a crash instead, which is the other problem we've been seeing with version 1.7.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants