Automate testing of merge requests to streamline review.

[nejucomo]

It would be nice if prior to a review an automated test pass was performed. This would save both the contributor and the reviewer some time.

Note: This is a potential security risk, because currently all automated test code is vetted by those with push access to the official repository or the buildbot configuration.

We need some way to address the security risk, or to explicitly accept the risk.

[daira]

I don't think it is actually a problem if this is post-review rather than pre-review (i.e. it is something you do if you would have committed the code anyway in the old flow).

An issue here is that commits can be added to a pull request at any time, so the committer should specify a precise commit hash to be tested. I seem to remember that was the conclusion of the discussion we had a while ago, as well (or at least that it is what I was arguing for then).

[markberger]

I think the easiest solution to this problem would be to use travis. It already has great Github integration and setting it up would require a configuration file. We also wouldn't have to worry about the potential security risk. Buildbot could still do all of the platform and package testing when something is merged to master, but travis would save us time when reviewing pull requests.

[markberger]

For travis, the developer doesn't specify the commit to test. Instead, travis just selects the latest commit in the series. Here is an example of what travis looks like with multiple commits: ​https://github.com/rspec/rspec-core/pull/1027

Another potential solution to this ticket would be to write a Github bot. For example, someone opens a pull request and a developer reviews it. If the developer thinks everything looks good they would comment on the pull request @tahoe-bot: test. This would trigger buildbot to fetch the the pull request and run the tests. The bot then comments on the pull request accordingly. To solve the security issue, only allow members of the Tahoe-LAFS organization to trigger forced builds.

In the event that we try to integrate buildbot with Github, this link will be useful: ​http://developer.github.com/v3/repos/statuses/#create-a-status

[zooko]

I want buildbot tests pre-review, in order to take load off of reviewers. (I also want them post-review. I also want them on the branch where they were developed and I also want them on trunk.)

[markberger]

Are we going to explicitly accept the risk then?

[zooko]

Well… how about this:

• the buildbot should run code only from a whitelist of repositories. This is already in place: ​https://github.com/markberger/buildbot-config-tahoe/blob/master/tahoe/git/master.cfg#L43
• a trusted Tahoe-LAFS volunteer is expected to manually commit any patches to some repo in the whitelist in order for buildbot to test that patch; This implies that we extend the whitelist to include at least one repo that every trusted Tahoe-LAFS volunteer can push into
• the volunteer is expected to perform some sort of minimal gating of these patches, such as that they glanced at the patch for 1 second, or that the patch came from a source that they know, before pushing it into such a whitelisted repo
• the volunteer is not expected to inspect the patch critically in search of vulnerabilities or trojan code before so pushing it
• we have some kind of append-only log of which patches got built and the provenance of where those patches came from (maybe git+github will give us this for free?)

markberger: what do you think so far?

[markberger]

Zooko: I think that your plan is a good way for buildbot to test pull requests. However, I'm not sure it accomplishes the end goal of making the reviewer's job easier because the reviewer is still manually initiating the tests. Also there is an additional step of merging from the new whitelist branch to master.

But if you think that process will make your life easier I should be able to implement it easily. I just need access to the tahoe-lafs repo settings.

[daira]

I'm skeptical that pre-review buildbot testing will actually take significant load off reviewers relative to post-review testing (because problems found by buildbot testing tend to be orthogonal to problems found by manual review). Therefore I am dubious about accepting any increased security risk in order to enable pre-review buildbot testing.

[zooko]

I disagree — I think that it will be very valuable to have the buildbot give feedback to patch-writers automatically, without requiring (much) manual effort on the part of a reviewer.

I also don't think the security risks are dangerous. As long as the buildslave is running only code that has been indelibly committed to a publicly visible repository, then I don't think there is a lot of risk of an attacker going to all the effort of making a trojan patch and submitting it, in order to take over a buildslave. If that happened, it would be very interesting! It would be worth losing a buildslave (or a VM or whatever) just to see that happen.

[zooko]

Replying to markberger:

But if you think that process will make your life easier I should be able to implement it easily. I just need access to the tahoe-lafs repo settings.

You mean to register a github post-commit hook? Maybe you could tell Brian what hook to register and he could do it.

[markberger]

Replying to zooko:

Replying to markberger:

But if you think that process will make your life easier I should be able to implement it easily. I just need access to the tahoe-lafs repo settings.

You mean to register a github post-commit hook? Maybe you could tell Brian what hook to register and he could do it.

That was my plan but I didn't understand post-commit hooks at the time. I don't think anything needs to be changed on that end.

[daira]

Replying to zooko:

I also don't think the security risks are dangerous. As long as the buildslave is running only code that has been indelibly committed to a publicly visible repository, then I don't think there is a lot of risk of an attacker going to all the effort of making a trojan patch and submitting it, in order to take over a buildslave. If that happened, it would be very interesting! It would be worth losing a buildslave (or a VM or whatever) just to see that happen.

Ugh. I'm not at all happy about increasing the risks to buildslave operators, relative to what they originally signed up for.

[zooko]

Replying to daira:

Ugh. I'm not at all happy about increasing the risks to buildslave operators, relative to what they originally signed up for.

Well that's a good point. How about if there is just one sacrificial buildslave which runs tests on the "pre-reviewed" branch? In the future that one buildslave could be confined with authority-reduction technologies (like "FreeBSD jail" ​https://en.wikipedia.org/wiki/FreeBSD_jail).

[markberger]

I've added a pull request for this: ​https://github.com/tahoe-lafs/buildbot-config-tahoe/pull/2. Once there is a bot dedicated for testing pull requests someone will have to add a branch scheduler, but this should add the github status integration.

[amontero]

Here's my feedback from what I've understood from ​http://about.travis-ci.org/blog/announcing-pull-request-support .

I'm a PHP programmer and sysadmin. So, no code contributions to Tahoe-LAFS by now, just docs. However, I think that automating tests as described above would help to lower the code contributor entry barrier and ease reviewer's work.

Imagine I finally find the time to learn Python and want to get my feet wet with Tahoe-LAFS. I fork on GH and start hacking.

1. As I'm learning, it may take some time. New tests may be written meanwhile and since CI happens against current upstream, those new tests may trigger new errors to me.
2. Reviewers won't even know about my attempts, since CI would happen in my branch even before submitting my PR. Less noise for them.
3. Reviewers found new tests worth automating. Someone (even the wanna be contributor) writes them, improving overall QA. All this in a separate/own branch. Less noise again (now and in the future).

I'm not too familiar with Travis. I come from Drupal and each core submitted patch is automatically testbotted at submission and it helps a lot when you are in the learning phase of contribution (think on all those GSoCcers and newbies out there). Setting a local Travis may be an added difficulty for newcomers. I find Tahoe's code intimidating enough for a newbie to add the CI to the picture.

I'm assuming that the local tests described in ​https://tahoe-lafs.org/trac/tahoe-lafs/wiki/AdvancedInstall#point8 can be run by Travis-CI automatically. Is this possible?

[zooko]

I had trouble understanding comment:15 until I went ahead and read ​http://about.travis-ci.org/blog/announcing-pull-request-support/ . Now it is all clear! Here is the desired functionality (desired at least by amontero and by me):

1. A new/casual contributor clicks the "fork this" button on github.
2. The contributor write a patch and pushes it to their repo on github.
3. The contributor pushes the "pull request" button on github.
4. Very soon, and with no manual intervention from any Tahoe-LAFS developer, some automated bot somewhere runs the unit tests on the version in the pull request, and posts a comment on the pull request indicating whether the tests pass or not, and including a link to the test results.

This is exactly what I want, and apparently other people also want this and have already implemented it, using travis-ci. It looks like we might be able to get the travis-ci people to do this for us and avoid all the problems of implementing it ourselves in our buildbot scripts, and the problems of security of our buildslaves.

I intend to investigate signing up for this. It looks like, from the end of ​http://about.travis-ci.org/blog/announcing-pull-request-support/, that you have to pay ("donate") to the travis-ci project in order to get in on this.

[zooko]

Oh nevermind that last part about having to donate to the project -- that blog post was from May 2012. Here's some newer information:

​http://about.travis-ci.org/blog/2012-09-04-pull-requests-just-got-even-more-awesome/

[zooko]

This sounds great! ​https://github.com/blog/1227-commit-status-api

What's the next step to try it out? Somebody do the necessary configuration to get travis-ci to run Tahoe-LAFS's unit tests and show us the result!

[amontero]

Looks like even individual commits without a PR would get CI run: ​http://about.travis-ci.org/blog/2012-09-04-pull-requests-just-got-even-more-awesome/#comment-639886287

Contributors now can ensure their patch pass before submitting a PR.

[markberger]

Replying to zooko:

This sounds great! ​https://github.com/blog/1227-commit-status-api

What's the next step to try it out? Somebody do the necessary configuration to get travis-ci to run Tahoe-LAFS's unit tests and show us the result!

I will try to do this sometime next week before dev chat on Wednesday.

Edit: Never mind, looks like daira is setting it up! :)

[daira]

Replying to markberger:

Replying to zooko:

This sounds great! ​https://github.com/blog/1227-commit-status-api

What's the next step to try it out? Somebody do the necessary configuration to get travis-ci to run Tahoe-LAFS's unit tests and show us the result!

I will try to do this sometime next week before dev chat on Wednesday.

Edit: Never mind, looks like daira is setting it up! :)

​It works! However, there is a security issue we need to discuss: #2072

[amontero]

As the icing on the cake, shouldn't we add ​status images somewhere in the home page?

[amontero]

Caveat: Although people contributing from their own forked repos can enable CI on them, this can produce lots of noise on tahoe's IRC as it is configured now. Edited NewbieDeveloperSetup to add Travis CI related info.

[daira]

Replying to amontero:

As the icing on the cake, shouldn't we add ​​status images somewhere in the home page?

+1, once #2072 is resolved. Filed as #2074.

Replying to amontero:

Caveat: Although people contributing from their own forked repos can enable CI on them, this can produce lots of noise on tahoe's IRC as it is configured now.

I think we can cross that bridge when/if we come to it; in the meantime I don't mind being nosy about what forks are doing :-) ​This Travis issue is relevant.

Edited NewbieDeveloperSetup to add Travis CI related info.

Thanks, I added a security caveat about needing write access.

[warner]

I'm ok with relying on travis and github pull-requests for this. These days, the workflow is excellent, for both contributors and committers. The pull-request has little status badges that get updated when travis reports the status of the test build.

Buildbot can stick to testing things that actually land on master. We get slightly better test coverage from buildbot (more than just linux), but I don't think it's worth the effort to try and provide that for pre-commit branches.