Automatic CI kickoff

[jasnell]

It would be fantastic if the bot could automatically kick off a new CI run based on a request in comments. For instance, if I posted a comment like: @nodejs-github-bot run CI please.

[addaleax]

How about letting the bot do that when there has not been a previous CI run and the first “approved” review is made? That seems like a reasonably automated way of implementing this to me, but I can’t really tell how tricky that would be.

[phillipj]

That's not too difficult, as far as I know Jenkins has a decent API for triggering builds. My only concern is the reasoning behind the CERTIFY_SAFE option we have to manually check when starting a PR build:

Maybe as long as the approval comes from a collaborator, that is as good as checking the CERTIFY_SAFE option?

[addaleax]

Maybe as long as the approval comes from a collaborator, that is as good as checking the CERTIFY_SAFE option?

Yeah, that’s what I had in mind. Sorry, forgot for a minute that non-collaborators can approve, too ;)

[jbergstroem]

The logical next step would be just starting a job if a committer files a pr, no?

[gdams]

could we not just go down the route of using https://wiki.jenkins-ci.org/display/JENKINS/GitHub+pull+request+builder+plugin though? You an admin simply types ok to test and then the CI runs

[gibfahn]

This is something we're looking at for citgm as well. I think what's been suggested so far makes sense, triggering a build when:

• The person who raised the PR has commit access to the project
• Someone who has commit access approves the PR.

I think it would probably be worth having a manual trigger again (e.g. for rerunning CI). Maybe something like github-bot run CI as a comment from someone with commit access?

[jbergstroem]

@gibfahn ..or we could rebuild on any pushes to the branch [again, making the assumption that a committer filed the PR]?

[gibfahn]

@jbergstroem we could definitely do that (I guess as well as the above options rather than instead of?), the only issue would be whether we'd end up doing too many CI runs for branches that are updated frequently.

I guess the other part of this would be for @evanlucas's core-validate-commit module to check whether CI passed (or at least was run). That would help people make sure that CI is run before they merge PRs (assuming people use it).

[gibfahn]

@jbergstroem @phillipj @Fishrock123 So @gdams and I would like to start looking at implementing this for citgm. Is there any Jenkins access we'd need to get this set up?

The plan was to trigger CI runs of this job on new citgm PRs.

[phillipj]

Fantastic!

Yes, sounds like we will have to grant the bot access to Jenkins. The only communication between Jenkins and the bot ATM, is when Jenkins sends requests the bot reporting build statuses for PRs. In other words, there is no communication initiated from the bot to Jenkins.

[gdams]

okay so what's the main steps that we need to follow to set up jenkins to report back to github?

[phillipj]

@jbergstroem knowns what is needed for a Jenkins build to report back to the bot

[gdams]

brilliant thanks

[jbergstroem]

We basically inject a lot of curl as part of the build steps. It's non optimal and at times I actually don't understand why it doesn't work. My preferred approach would be pinging at start/end but also having a process actually monitoring this to verify its results.

[phillipj]

My preferred approach would be pinging at start/end but also having a process actually monitoring this to verify its results.

Pinging as in the bot continuously polling the Jenkins REST API for build updates?

[jbergstroem]

Yes; once a job starts we have a pretty good idea on how long it will take based on previous jobs.

[phillipj]

@jbergstroem would you mind setting up citgm with curl to get that going, and do a separate issue refactoring adding Jenkins pulling as you suggest?

[jbergstroem]

do i post to the same endpoint? a new?

[gdams]

@jbergstroem what else needs to be set up to get this working for citgm?

[jbergstroem]

@phillipj: do you perhaps want to set this up now that you have access?

[phillipj]

Sure, I'll open a issue over at citgm for further discussions.

[gdams]

Sgtm

[phillipj]

Status update on this: first iteration on this has been deployed and is currently being used by the CITGM crew.

After a test periode on CITGM, and possibly doing some fixes, we can consider enabling this on other repos in collaboration with their governing WG.

[BridgeAR]

Is there something that can be done to push this forward? I guess the testing period has been long enough? Have there been any issues so far that still have to be addressed?

[jasnell]

Small refinement on the request here... running CI automatically for all PRs definitely is not ideal. Perhaps a label like ci-ready can be used to flag PRs that should be submitted to CI? If a light CI run is required, then something like ci-lite-ready, and an equivalent citgm-ready for citgm runs, would be a good approach?

[targos]

I would prefer to have to write a comment @-mentioning the bot. It gives more flexibility and allows to easily trigger new runs.

[BridgeAR]

I think the suggestion from @addaleax is nice but I would like to be a bit more conservative about it. We all can make mistakes and if someone would really want to harm us, it would probably be possible to obfuscate some code good enough to maybe get at least a single LG. We should also not trigger the CI on a update without a new LG.

So maybe we can use a combination of labels and LGs?

As in triggering a build when:

• The person who raised the PR has commit access to the project
• Two persons who have commit access approved the PR
• The code was updated (a rebase, new commit...) and the code got two new approvals from collaborators
• A author-ready label is applied
• A ci-ready label is applied (and the label would be removed as soon as the CI run is done)

I guess in this case there would not be to many CI runs even though we will have some obsolete runs. To reduce the overhead on our machines it would be great if the bot stops all still running CIs in case a new one is started.

I think the bot is capable of detecting a CI-Lite and a regular CI well enough by just checking if only doc changes were applied or anything else. There might be a few false-positives but I think auto detecting this is still best.

I am also definitely +1 on having a label to start the CITGM.

[bnoordhuis]

^ feels overly conservative. If you have push access, you're implicitly trusted anyway.

I'd be good with kicking off CI right away rather than after first review. If the CI is angry red, you know you can hold off on reviewing because the author has some fixing up to do.

[Trott]

Seems like this is (partially?) implemented now in #173. Closing, but if you think that's totally wrong, by all means, re-open.