Can Jenkins add itself as a code reviewer and reject the diff if build fails

[rbwu]

Great plugin!

Jenkins would add comment to a Phabricator revision with build status. But in addition to this, is there a way for Jenkins to also add itself as a reviewer and accept or reject a revision based on the build status?

I just think this would make it more obvious that a build fails.

[ascandella]

At Uber, and at many other builds systems I've seen, it works like:

• A human submits a change to a remote server
• That server kicks off a build and informs the designated reviewers (if any) of the code to revniew
• CI system returns a result
• Both Human and CI must agree on result (except in exceptional cases)

So yes, we could add a feature to have jenkins auto-accept diffs, but then are you really doing code review?

[rbwu]

The reason I want to do this is because:

1. On the Phabricator web interface, it is more obvious if 'Jenkins' accepts or rejects your diff.
2. When doing 'arc land', although arc would prompt you if your Jenkins build fails, you can still override that and go ahead and land anyway.

What I want to achieve is to prevent people from merging their diffs into the master branch if the build for that diff fails. Happy to hear your thoughts.

[ascandella]

Yes, there's definitely some usability issues around harbormaster status versus accepted/rejected that could be improved on the phab side.

With regards to arc land -- you can bypass both an un-accepted diff (or one with requested changes) as well as one with a failed harbormaster build. So it's not really any "safer" to request changes IMHO.

I'm open to adding a feature to have jenkins approve/reject, but the workflows we use internally and what I've seen with other shops using the plugin, harbormaster status is generally the correct fit.

[rbwu]

I am curious to know how Uber deals with this. Some people would inadvertently merge a branch with broken builds into the master branch. Given that this is essentially an honor system, does this happen at lot at Uber and when it does happen, how do you guys deal with it (reverting, apply another fix to mater branch, etc)?

[ascandella]

It's mostly an honor system. For some of our more critical repos (e.g. our mobile builds, where app store pushes need a regular cadence) we have a server-side gitolite pre-receive hook that will check the Phabricator APIs and verify that it was code reviewed and has a successful build. This will block the push (land).

[martsa1]

Re preventing people accidentally pushing code etc., I have a herald rule configured on my system that blocks any attempt to push changes to master that don't have an accepted diff related to them. That way accident or not its not possible for a user to push their changes to master on phabricator until they've had their diff approved. At that point a reviewer would be able to approve/request changes to a diff and take the build status into account.

I've included a grab of what the herald rule looks like, I don't know if it may be helpful here, and it doesn't prevent someone from landing modified changes from an accepted diff, but its good enough for my purposes..