-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix : Commit statuses from forks are not included #62
base: main
Are you sure you want to change the base?
Conversation
utils/buildQuery.js
Outdated
oid | ||
author { | ||
user { | ||
url | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oid | |
author { | |
user { | |
url | |
} | |
} | |
oid | |
author { | |
user { | |
url | |
} | |
} |
utils/buildQuery.js
Outdated
commit{ | ||
author{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit{ | |
author{ | |
commit { | |
author { |
utils/runQuery.js
Outdated
Array.prototype.forEachAsync = async function (fn) { | ||
for (let t of this) { await fn(t) } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we definitely shouldn't be modifying builtins :-)
Array.prototype.forEachAsync = async function (fn) { | |
for (let t of this) { await fn(t) } | |
} |
this should be fns.reduce((prev, task) => prev.then(() => task), Promise.resolve())
, inline, instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jordan I've replaced it with reduce in the recent commit, would you please take a look
utils/runQuery.js
Outdated
if(response.repository.commit){ | ||
const checks = response.repository.commit?.statusCheckRollup?.contexts?.nodes | ||
return checks | ||
} | ||
return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(response.repository.commit){ | |
const checks = response.repository.commit?.statusCheckRollup?.contexts?.nodes | |
return checks | |
} | |
return null | |
return response.repository.commit?.statusCheckRollup?.contexts?.nodes; |
utils/runQuery.js
Outdated
|
||
|
||
|
||
const getCommitChecks = async (name,owner,sha,token)=>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const getCommitChecks = async (name,owner,sha,token)=>{ | |
const getCommitChecks = async (name ,owner, sha, token) => { |
utils/runQuery.js
Outdated
`; | ||
|
||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`; | |
try { | |
`; | |
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to also add some tests to cover this :-)
utils/runQuery.js
Outdated
if (response.repository.commit) { | ||
const checks = | ||
response.repository.commit?.statusCheckRollup?.contexts?.nodes; | ||
return checks; | ||
} | ||
return response.repository.commit?.statusCheckRollup?.contexts?.nodes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two paths seem like they're the same - did you mean to check something else on line 50?
commit.statusCheckRollup.contexts.nodes = allChecks; | ||
commit.statusCheckRollup.contexts.totalCount = allChecks.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way we could avoid mutation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How'd we proceed without mutating ? are you suggesting creating a new object ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly
I've rebased this after the latest merges. |
// get the commit checks for the commit that ran on the fork | ||
await getCommitChecks( | ||
name, | ||
url.split('/').slice(-1).shift(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this also be url.slice(url.lastIndexOf('/'))
?
The fork is deleted, so this and #72 are unrecoverable. I'll keep these open in case someone can resurrect anything out of it. |
yay, turns out the fork was just private! i'll rebase this once "allow edits" is re-checked update: it seems likely that the act of making this repo public severed the connection to all PRs from forks, which means they're indeed unrecoverable :-( |
can-merge was not fetching the commit statuses from forks, this PR addresses the issue.
There's no way in the github API that allows us to get those checks, only way to get the commit checks of a commit is by providing the original owner of the commit and then running the repository/search query.
As discussed with @ljharb I started by modifying the
buildQUery
so that it return us the original author of the commit along with the commits.In the
runQuery.js
i've added a new query that basically is similar to the repo query that we already use, and it calls the github api with the actual author of the commit, which returns us the commit checks. I then merge the two checks.I've added this modification to just the search query for now, and can update it to work for the repo query as well after the reviews.🙂