[Twisted-Python] Required tests for a PR to be merged
Hi, 1. Are Travis OSX builds functional yet? Can we switch to Travis for OSX builder? Over https://www.traviscistatus.com/ I see no backlog for macOS. ------------ 2. Right now, when we receive a PR from a non-team member, I think that the only way to trigger the required tests is to create a new branch in twisted/twisted. Then you can go to https://buildbot.twistedmatrix.com/boxes-supported?branch=9176-inlinetraceba... and force the tests. This looks complicated, so what I was doing, is create a new PR. -------- Am I doing something wrong? Is there a simple way? Am I the only one who thinks this is not productive? -------- What I am thinking is to have a single PR (the once created by the author). Once a Twisted team member has reviewed/checked the PR, a message like "Go, Buildbot, go!" can be left as a comment and the Buildbot GH hook will trigger the stable tests for that branch. In this way, buildbot runs are only triggered for external branches after a core team member has checked the code and assure that no malware is contained. With GitHub we can do changs to any forked repos... so if small changes are required, Twisted team members can push directly to the external forks... not sure if this is a feature or a security bug :) --------- What do you think? Do you see any problem in this? Will such a change make life easier for you? I should be able to update the current GH Buildbot hook to handle the proposed change. -- Adi Roiban
On Mon, Oct 2, 2017 at 7:20 AM, Adi Roiban <adi@roiban.ro> wrote:
No backlog right now but what about most of the rest of the time? Monday at 7AM ET is probably about the least busy possible time for travis.
Use admin/pr_as_branch
This doesn't seem inherently bad, though you probably need per *revision* signoff, not per branch signoff.
On 2 October 2017 at 12:24, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
On Mon, Oct 2, 2017 at 7:20 AM, Adi Roiban <adi@roiban.ro> wrote:
[snip]
Will it trigger the Buidlbot stable builders?
From what I can see in the code, it only creates the branch.
[snip]
We can have something like: "Go, Buildbot, go for 1d32a23!" . In the initial email I was thinking to use "Go, Buildbot, go!" and to trigger only the commit which was pushed before the comment. So you need to let a comment each time you want to trigger the tests. A comment will not automatically trigger any future commit. But now that I am reviewing it, do we need to be that secure? I was thinking to leverage the GitHub Contributor information and if the author is a "Contributor" (a commit was previously accepted), it is automatically trusted. Thanks! -- Adi Roiban
On Mon, 2 Oct 2017 at 13:36 Adi Roiban <adi@roiban.ro> wrote:
Will it trigger the Buidlbot stable builders?
From what I can see in the code, it only creates the branch.
I believe buildbot will trigger a build on the branch being pushed, no need for a separate PR. Then, because GitHub commit statuses are on the _commit_, the PR should be green too if the build succeeds.
On 2 October 2017 at 14:41, Tristan Seligmann <mithrandi@mithrandi.net> wrote:
True. Thanks! Found the code here https://github.com/twisted-infra/braid/blob/master/services/buildbot/master/... -- Adi Roiban
On Mon, Oct 2, 2017 at 7:35 AM, Adi Roiban <adi@roiban.ro> wrote:
If it doesn't, there's probably no reason it *couldn't* (apart from the obvious labor requirements). I did think it already did everything you wanted which is why I suggested it but I think you're right that's it's not quite complete. [snip]
That seems fine to me - I just wanted it to be clear that the comment only triggers the current HEAD of the branch (there's still a potential problem since you can't ever know what the HEAD will be when you actually post the comment but perhaps it's safe to ignore this possible attack).
Creating a way to delegate CI authority seems like a good idea, yes. Perhaps the heuristic should not be exactly "one previously accepted commit" but something along those lines seems sane. You still probably want a way to trigger a complete CI run for folks who haven't yet been admitted into this set, though. Jean-Paul
On Mon, Oct 2, 2017 at 7:20 AM, Adi Roiban <adi@roiban.ro> wrote:
No backlog right now but what about most of the rest of the time? Monday at 7AM ET is probably about the least busy possible time for travis.
Use admin/pr_as_branch
This doesn't seem inherently bad, though you probably need per *revision* signoff, not per branch signoff.
On 2 October 2017 at 12:24, Jean-Paul Calderone <exarkun@twistedmatrix.com> wrote:
On Mon, Oct 2, 2017 at 7:20 AM, Adi Roiban <adi@roiban.ro> wrote:
[snip]
Will it trigger the Buidlbot stable builders?
From what I can see in the code, it only creates the branch.
[snip]
We can have something like: "Go, Buildbot, go for 1d32a23!" . In the initial email I was thinking to use "Go, Buildbot, go!" and to trigger only the commit which was pushed before the comment. So you need to let a comment each time you want to trigger the tests. A comment will not automatically trigger any future commit. But now that I am reviewing it, do we need to be that secure? I was thinking to leverage the GitHub Contributor information and if the author is a "Contributor" (a commit was previously accepted), it is automatically trusted. Thanks! -- Adi Roiban
On Mon, 2 Oct 2017 at 13:36 Adi Roiban <adi@roiban.ro> wrote:
Will it trigger the Buidlbot stable builders?
From what I can see in the code, it only creates the branch.
I believe buildbot will trigger a build on the branch being pushed, no need for a separate PR. Then, because GitHub commit statuses are on the _commit_, the PR should be green too if the build succeeds.
On 2 October 2017 at 14:41, Tristan Seligmann <mithrandi@mithrandi.net> wrote:
True. Thanks! Found the code here https://github.com/twisted-infra/braid/blob/master/services/buildbot/master/... -- Adi Roiban
On Mon, Oct 2, 2017 at 7:35 AM, Adi Roiban <adi@roiban.ro> wrote:
If it doesn't, there's probably no reason it *couldn't* (apart from the obvious labor requirements). I did think it already did everything you wanted which is why I suggested it but I think you're right that's it's not quite complete. [snip]
That seems fine to me - I just wanted it to be clear that the comment only triggers the current HEAD of the branch (there's still a potential problem since you can't ever know what the HEAD will be when you actually post the comment but perhaps it's safe to ignore this possible attack).
Creating a way to delegate CI authority seems like a good idea, yes. Perhaps the heuristic should not be exactly "one previously accepted commit" but something along those lines seems sane. You still probably want a way to trigger a complete CI run for folks who haven't yet been admitted into this set, though. Jean-Paul
participants (3)
-
Adi Roiban -
Jean-Paul Calderone -
Tristan Seligmann