Kicking off Jenkins build triggered by comment

[phillipj]

Putting this out for public scrutiny to show there is some kind of progress.

The main goal is to get the bot to kick off a Jenkins build triggered by a collaborator commenting in a PR, mentioning the bot.

Refs #82

[phillipj]

With the current changes, the bot replies to any comments in gets mentioned in as long as the comment author has checked out to be a repo collaborator.

TestOrgPleaseIgnore/node#28 (comment)

[phillipj]

IMO this is getting ready to merge.

How does this work?

Any repository collaborator creates a comment in any PR:

@nodejs-github-bot run CI

That is picked up by the bot that verifies the comment author is actually a collaborator in that particular repository. If so, the bot requests http://ci.nodejs.org to trigger a build for a preconfigured job for that repository.

As soon as it gets a response from Jenkins, the bot replies to the comment author saying the build started and the corresponding job URL, or informing an error occurred when communicating with Jenkins.

If the Jenkins job is configured showing inline PR build statuses via this bot, build statuses will start to be visible in the PR as soon as they start / end. That's not really part of this PR, that functionality has already been implemented.

How is this configured?

Three new $ENV variables are introduced here. A "global" $JENKINS_API_CREDENTIALS used to authorise as the nodejs-github-bot user when communicating with Jenkins. And two other $ENVs specific to each repository and related Jenkins job.

This have to be configured and enabled explicitly per repo.

Read more about them in the changes done in README.md.

Further improvements

The comment made by the bot to inform about build started, contains the Jenkins job URL, which ideally would be the build URL. Improving that means continuously polling Jenkins for build started after the triggering a build. The reason behind that is Jenkins doesn't wait to respond until the build is actually started, but responds as soon as the build as been put into the build queue.

What's next?

• Review from anyone else in the @nodejs/github-bot group
• Thumbs up functionality wise from @gdams @gibfahn

[williamkapke]

You could assigning it to the bot-- or create a 'needs rebuild' label. Either option requires a person to be a collaborator- so that check would be already done.

[phillipj]

Good point @williamkapke. My only concern with those two are the awkwardness of triggering more builds after changes are made. Having to re-assign or re-label rather than posting a new comment.

[williamkapke]

@phillipj When a label is applied or it is assigned, it puts a message like "williamkapke assigned this issue to node-github-bot at some-date"

... when the bot gets the notification, it could reply like "Ok, williamkapke- I've started the build."

Oh- and you'd want to unassign/unlabel after it's done.

[mscdex]

I'm confused, don't collaborators already have access to start a new job from the Jenkins UI?

If so, why this feature? It seems like it would just add more noise to PRs.

[phillipj]

@mscdex absolutely. I see this as simplifying the process of starting a build against the changes in a PR, rather than clicking through the Jenkins UI and filling in the parameters by hand (ref #82).

Possibly a precursor to automatically kicking off builds whenever a collaborator opens a PR, tho that's a different discussion.

[mscdex]

One potential issue is that this kind of mechanism does not require a collaborator to explicitly acknowledge that they have reviewed the PR and are sure that it is "safe" (this is currently done in the Jenkins UI via the "CERTIFY_SAFE" checkbox). If we ignore that with the bot, why keep the checkbox in the Jenkins UI?

[phillipj]

Yepp, we have to have that discussion in nodejs/node before enabling this functionality there.

This have to be enabled very explicitly per repo. ATM we want this in the citgm repo where CERTIFY_SAFE is not an issue. The code has been written to work on any repo though, so if we see this working well for the citgm crew, we might consider enabling it on other repos as well.

[gdams]

@mscdex I think that we can just update the contributing guidelines per repo to state that by typing @nodejs-github-bot run CI you confirm that you have approved the changes and that they are safe to run

[mscdex]

@gdams But then what's the point of keeping the checkbox on the Jenkins UI? You could just say that by submitting that form you are implicitly certifying the PR is "safe" instead of having to check a checkbox.

[gibfahn]

@mscdex True, but then what's the issue with the checkbox in the Jenkins UI? It doesn't really take any extra time to click.

I guess we could have the phrase as @nodejs-github-bot confirm safe run ci if you want, but that just seems like a pain.

I guess my point is that checking another box is ~2% of time when you're running through CI, but adding confirm safe doubles the time it takes to trigger CI.

[phillipj]

Don't want to undermine the confirm safe discussion, that is obviously important and we will not avoid that discussion before even thinking about enabling this on nodejs/node.

But for the time being, could we ignore that a bit and focus reviewing the code changes and getting this tested on nodejs/citgm?

[phillipj]

@williamkapke said:
You could assigning it to the bot-- or create a 'needs rebuild' label. Either option requires a person to be a collaborator- so that check would be already done.

@gdams @gibfahn you got any thoughts about those two alternatives, compared to commenting to trigger builds?

[gdams]

@phillipj, no I'm much more of a fan of the way you have don't it, it's the only way that will make rebuilds easy

[gibfahn]

I agree with @gdams, commenting is the easiest way. To be honest it'd be ideal if the bot had a shorter name, but it's not the end of the world (due to tab complete).

Labels and assigning require people to remove and re-add to trigger another CI run, which is odd. EDIT: The bot can remove the label, as pointed out by @williamkapke. I think it's also easier for new collaborators to understand what @nodejs-github-bot run ci means, a tag or assignment would be less clear.

It will also be much easier if we want to have multiple commands for the bot in the future (e.g. run v8 ci or retry macos, or even squash land).

I know @mentioning in a comment is how Rust use their bot.

[williamkapke]

@gibfahn No one would need to un-assign+assign or remove+re-addd. The bot just removes does the removal as part of it's work.

I'm not MASSIVELY against using the comment method. I just prefer to use things that reduces the number of email notifications that are sent to everyone. But- email filters can be added, right? 😆

@phillipj Thanks for considering it- that's all I ask. 👍

[gibfahn]

@williamkapke that's a good point, I hadn't thought of the notifications question (probably because I use the web notifications). I guess the problem is that you'd get two emails, one for the @nodejs-github-bot comment, and one for the reply?

[gdams]

There wouldn't need to be a reply

[gibfahn]

I think pinging people when you rerun CI is a good thing (it's what we have now, and IMO it works well).

@gdams the problem with just having the Github CI status section is that (AFAIK) you can only see the results for the latest build that was run. I find it useful to go back to previous CI runs.

Also in the past the reporting hasn't always been correct, but maybe that won't be a problem going forward.

[phillipj]

Ping @nodejs/github-bot as I'm missing a clear thumbs up on this before deploying it and doing some initial tests with CITGM.

[addaleax]

@phillipj really looking forward to seeing this!

[phillipj]

With the latest commented issues fixed, I'm leaning towards merging this before the end of the week if no objections are made.

[phillipj]

@gdams @gibfahn this is running in production and configured for the CITGM repo 👯

As I don't have collab access to that repo, I can't trigger runs.. So the honours is all yours when you have the time -- abuse nodejs/citgm#375 as much as you want.

[addaleax]

@phillipj Is this live for nodejs/node, too?

[phillipj]

@addaleax nope. We'll test it on CITGM first, then have a certify safe discussion (ref #128 (comment)) before considering enabling it on nodejs/node as well.