I’ve been poking around this some more.
The root issue seems to be that the plugin is picking up some contributions (merged PRs) as commits. This means that the numbers are off, and users who do not have commit rights on the repo are incorrectly granted committer
badges.
Here’re some examples, both PRs from @Bruno - who does not have commit rights on the repo :
- 20 Dec 2019
1d21cf166a8ed7def514a1860318e74b64dd47da
merged by mjsax
. Plugin categories this as a contribution (correctly).
- 29 Oct 2020
a85b9440118ae5aa442d2e1e70b379841f941ddc
merged by ableegoldman
. Plugin incorrectly categories this as a commit (incorrectly).
The role
field is how it is viewing the commit vs contribution (code):
SELECT * from github_commits
where sha IN ('a85b9440118ae5aa442d2e1e70b379841f941ddc','1d21cf166a8ed7def514a1860318e74b64dd47da')
This looks like the code that decides whether it’s a PR (contribution) or not:
Looking at the data the GitHub API returns we can see the values for the two commits above:
$ curl --show-error --silent -H "Accept: application/vnd.github.v3+json" \
https://api.github.com/repos/apache/kafka/commits/1d21cf166a8ed7def514a1860318e74b64dd47da | jq '.commit.author,.commit.committer'
{
"name": "Bruno Cadonna",
"email": "bruno@confluent.io",
"date": "2019-12-20T22:21:12Z"
}
{
"name": "Matthias J. Sax",
"email": "matthias@confluent.io",
"date": "2019-12-20T22:21:12Z"
}
$ curl --show-error --silent -H "Accept: application/vnd.github.v3+json" \
https://api.github.com/repos/apache/kafka/commits/a85b9440118ae5aa442d2e1e70b379841f941ddc | jq '.commit.author,.commit.committer'
{
"name": "Bruno Cadonna",
"email": "bruno@confluent.io",
"date": "2020-10-29T23:10:30Z"
}
{
"name": "GitHub",
"email": "noreply@github.com",
"date": "2020-10-29T23:10:30Z"
}
So the second commit (a85b9440118ae5aa442d2e1e70b379841f941ddc
) despite still being a PR that was merged, has fallen victim to the logic in the plugin code shown above, which says that if the committer name is GitHub
then it must be a direct commit by the author - and I don’t think that’s a valid assumption for it to make.