homu responds to `@homu r+` does not merge, though

[MichaelHirn]

I just wanted to have this PR merged, but homu doesn't seem to work anymore. It states though that the PR was accepted.

On the website is an entrance in the queue, but neither synchronize nor create a rollup seems to make a difference.

http://homu.io/q/autumnai/phloem

[barosl]

Huh, this is interesting! I recently introduced the "test exemption" feature (1ad7067) to skip the unnecessary Travis pass on certain PRs, which means, if the Travis test result on a PR is "fresh" (no other PRs are merged into master since the PR is made), Homu will directly attempt to merge the PR into master.

This feature turns out to be problematic if the repository turned on the "branch protection" feature, which prevents direct pushing to the branch. I'm getting this:

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Required status check "homu" is expected
To git@github.com:autumnai/phloem.git
 ! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to 'git@github.com:autumnai/phloem.git'
* Failed to execute command: ['git', '-C', 'cache/autumnai/phloem', 'push', '-f', 'origin', 'master']

Frankly speaking, I didn't even know the existence of the feature! I'm embarrassed now.

So I'm not sure how to fix this problem, there are at least three choices:

1. First try to push to master, and only if it fails, move on to auto and try again master immediately.
2. Enable test exemption per repository basis.
3. Homu errors, and tells the user to turn off branch protection.

1 is great but it silently gives a speed penalty to all the affected repositories. By using 2, many users may ignore the option, not benefiting from the test exemption feature. 3 forces the user to give up branch protection, which doesn't sound ideal.

There might be a compromise like doing both 1 and 2, which makes Homu push to auto but also warn the user to turn off branch protection. But in this case the user will also be discouraged to use branch protection, though not exactly forced.

What do you think of this issue?

[barosl]

There are other users that seem to be using branch protection, cc @hauleth.

[hauleth]

I needed to give @homu admin access and give permission for admins to push to master. Then it works nice. Needed to reexecute @homu r+ several times to find what was wrong.

[barosl]

@hauleth I think that was because you didn't turn on "include administrators" in the branch protection settings? So branch protection was not applied to Homu. While this may be a solution, it would still be problematic with those using the "include administrators" option. (Or don't want to give Homu an admin access.)

[MichaelHirn]

@barosl I think, the 'test exemption' feature sounds really valuable, although you can't benefit from it with branch protection. Now, as an interim solution, I removed the branch protection from master did @homu r+ and invoked the branch protection again afterwards. As expected that worked pretty well.

Now for a final solution I would discourage the alternative option with admin rights. First, admins make mistakes as well and second giving a 3rd party admin access raises a lot of security questions.

But I think the solution you proposed by combining 1. and 3. is very reasonable. I like it and would love to see that one pushed forward.