Automate testing of merge requests to streamline review. #2052

New issue

Closed

opened 2013-08-07 19:55:37 +00:00 by nejucomo · 25 comments

nejucomo commented

2013-08-07 19:55:37 +00:00

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.

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.

nejucomo added the

labels 2013-08-07 19:55:37 +00:00

nejucomo added this to the undecided milestone 2013-08-07 19:55:37 +00:00

daira was assigned by nejucomo

2013-08-07 19:55:37 +00:00

daira commented

2013-08-08 00:17:48 +00:00

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).

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 commented

2013-08-08 13:20:02 +00:00

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 commented

2013-08-08 13:33:09 +00:00

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

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 commented

2013-08-20 14:26:13 +00:00

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.)

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 commented

2013-08-26 14:56:17 +00:00

Are we going to explicitly accept the risk then?

zooko commented

2013-08-26 20:16:54 +00:00

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?

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 commented

2013-08-26 21:18:06 +00:00

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.

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 commented

2013-08-27 02:28:57 +00:00

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 commented

2013-08-27 04:34:46 +00:00

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.

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 commented

2013-08-28 02:33:26 +00:00

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.

Replying to [markberger](/tahoe-lafs/trac/issues/2052#issuecomment-394766): > > 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 commented

2013-08-28 02:41:02 +00:00

Replying to [zooko]comment:10:

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.

Replying to [zooko]comment:10: > Replying to [markberger](/tahoe-lafs/trac/issues/2052#issuecomment-394766): > > > > 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 commented

2013-08-28 03:45:36 +00:00

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.

Replying to [zooko](/tahoe-lafs/trac/issues/2052#issuecomment-394768): > 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 commented

2013-08-28 11:51:09 +00:00

Replying to [daira]comment:12:

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).

Replying to [daira]comment:12: > 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 commented

2013-08-29 15:33:20 +00:00

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.

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 commented

2013-08-31 14:34:38 +00:00

Owner

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.

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.
Reviewers won't even know about my attempts, since CI would happen in my branch even before submitting my PR. Less noise for them.
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?

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 commented

2013-08-31 15:05:55 +00:00

I had trouble understanding comment:394774 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):

A new/casual contributor clicks the "fork this" button on github.
The contributor write a patch and pushes it to their repo on github.
The contributor pushes the "pull request" button on github.
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.

I had trouble understanding [comment:394774](/tahoe-lafs/trac/issues/2052#issuecomment-394774) 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 commented

2013-08-31 15:08:05 +00:00

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/

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 commented

2013-08-31 15:19:20 +00:00

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!

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 commented

2013-08-31 15:20:11 +00:00

Owner

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.

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 commented

2013-08-31 18:03:32 +00:00

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! :)

Replying to [zooko](/tahoe-lafs/trac/issues/2052#issuecomment-394777): > 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 commented

2013-09-01 01:09:08 +00:00

Replying to [markberger]comment:20:

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

Replying to [markberger]comment:20: > Replying to [zooko](/tahoe-lafs/trac/issues/2052#issuecomment-394777): > > 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!](https://travis-ci.org/travis-tahoe/tahoe-lafs) However, there is a security issue we need to discuss: #2072

amontero commented

2013-09-02 16:28:59 +00:00

Owner

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

As the icing on the cake, shouldn't we add [status images](http://about.travis-ci.org/docs/user/status-images/) somewhere in the home page?

amontero commented

2013-09-02 16:40:33 +00:00

Owner

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.

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](wiki/NewbieDeveloperSetup) to add Travis CI related info.

daira commented

2013-09-02 19:55:16 +00:00

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.

Replying to [amontero](/tahoe-lafs/trac/issues/2052#issuecomment-394781): > As the icing on the cake, shouldn't we add [status images](http://about.travis-ci.org/docs/user/status-images/) somewhere in the home page? +1, once #2072 is resolved. Filed as #2074. Replying to [amontero](/tahoe-lafs/trac/issues/2052#issuecomment-394782): > 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](https://github.com/travis-ci/travis-ci/issues/1094) is relevant. > Edited [NewbieDeveloperSetup](wiki/NewbieDeveloperSetup) to add Travis CI related info. Thanks, I added a security caveat about needing write access.

warner added

c/dev-infrastructure

and removed

c/unknown

labels 2014-09-11 22:32:22 +00:00

warner commented

2016-03-27 18:40:42 +00:00

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.

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.