Edit CI comment when subsequent PR jobs are started #226
Conversation
|
I haven't reviewed the code, but I'm very 👍 on this idea, and thank you for the quick turnaround time on it.
Does that include the "automatic CI" comment? I think that would be ideal so that there is one comment at the top of the issue that can be treated as an audit log of sorts. This would also mean appending the comment and not just overwriting it. |
lib/push-jenkins-update.js
Outdated
| function findExistingCiComment (options, logger, cb) { | ||
| async.parallel([ | ||
| botUsername.resolve, | ||
| githubClient.issues.getComments.bind(githubClient.issues, { |
There was a problem hiding this comment.
This will only return one page of entries (i.e. 30). We have issues (nodejs/node#26322) with much more, so this won't find the comment and revert to the current behavior...
I'm playing with the graphQL API, that can return 100. I was thinking of fetching a batch from the top, and from the bottom.
BTW: the graphQL schema has viewerDidAuthor property, so no need for the bot's id anyway.
There was a problem hiding this comment.
Good catch, obviously didn't remember that at first. Fetching all pages of entries is something we'd have to do elsewhere, e.g. in ./lib/node-repo.js.
I'm playing with the graphQL API, that can return 100. I was thinking of fetching a batch from the top, and from the bottom.
Cool, the GraphQL API is something I've been wanting to try out for a while too!
There was a problem hiding this comment.
I'll try to push a suggestion, as soon as I refresh my sinon & supertest skills...
I think it should. It's a convenient place, and it's easy to find (see paging) for PRs authored by Collaborators. |
|
Thanks for the lightning quick reviews!
My first intention was to put out the immediate "fire" of flooding CI comments since yesterday (#224). On second thought it probably won't be many changes needed to what is currently in this PR to make that happen, and thereby fix both while we're at it.
Mind opening a PR with what you've got if it's ready-ish? I'm certain we'd end up with a better solution at the end if we share notes so to speak.. |
|
|
Perhaps I'm in the minority, but I love having a new CI commit for every CI run. Currently, I manually paste the CI commit in for every single run I do, so the existing behaviour just automates what I do manually. |
|
@sam-github in general I feel the same way as you do, but I'm not an active core contributor these days and have never been involved in many PRs simultaneously so 🤷♂️ |
The motivation behind these changes are to prevent flooding PRs with CI comments from the bot whenever new node-test-pull-request jobs are started. Instead if the bot has already posted a CI comment earlier, it will update the last CI comment and change the URL in that comment to the new job that got started.
phillipj
force-pushed
the
edit-ci-comment
branch
from
f0b36c9
to
0338bd3
Compare
|
Rebased and force pushed after the bot username got hardcoded in 4f4ac43, thereby removing the previous complexity of getting the username async from the API. |
|
Closing this in favour of #228 which is more in line with keeping one bot comment that is edited as soon as new builds are kicked off, and gives GitHub's GraphQL API a shot which is valuable getting some experience with for later. |
The motivation behind these changes are to prevent flooding PRs with CI comments from the bot whenever new node-test-pull-request jobs are started. Instead if the bot has already posted a CI comment earlier, it will update the last CI comment and change the URL in that comment to the new job that got started.
Snook in a bugfix for the already existing helper function that fetches the bot's username. There has obviously been a regression in that function, so replaced the existing zero-value-unit-test with an integration test that should increase its reliability going forward.
/cc @nodejs/github-bot @cjihrig