Trac and PR?
59 views
Skip to first unread message
Vadim Zeitlin
Feb 23, 2015, 10:04:49 AM2/23/15
to wx-dev
Hello,
Now that we've migrated to Git (huge thanks to Bryan!), I wonder what
should we do about PRs on Github. My feeling is that we should accept them
as it is easier than making patches and so we'll get more contributions
like this, however at the same time I also feel that Github issues are too
primitive for our needs and would like to continue our Trac workflow.
So should we just accept both Github PRs and Trac patches? This seems like
the most straightforward thing to do but also risks to be most confusing.
OTOH I don't see what else could we do. I thought about writing (as
apparently such thing doesn't exist yet) a tool opening a Trac ticket for
every new PR, but I'm not sure if it's worth it...
What do you think?
VZ
Now that we've migrated to Git (huge thanks to Bryan!), I wonder what
should we do about PRs on Github. My feeling is that we should accept them
as it is easier than making patches and so we'll get more contributions
like this, however at the same time I also feel that Github issues are too
primitive for our needs and would like to continue our Trac workflow.
So should we just accept both Github PRs and Trac patches? This seems like
the most straightforward thing to do but also risks to be most confusing.
OTOH I don't see what else could we do. I thought about writing (as
apparently such thing doesn't exist yet) a tool opening a Trac ticket for
every new PR, but I'm not sure if it's worth it...
What do you think?
VZ
Bryan Petty
Feb 23, 2015, 1:13:50 PM2/23/15
to wxWidgets Development
Here's some added context for anyone that hasn't been following what
we do with pull requests right now:
GitHub PRs can not be turned off on any GitHub repository. Just Issues
can, and they have been turned off for all wxWidgets repos except for
the website. So regardless of whether we advise users to avoid them or
not, they will likely still come in.
Right now, we have an automated bot leaving a standard reply to all
pull requests opened. For example, this isn't actually me replying to
this one: https://github.com/wxWidgets/wxWidgets/pull/18
This notice is also shown to all users just prior to submitting a new
pull request to the wxWidgets repo, advising them that we use Trac
instead:
https://github.com/wxWidgets/wxWidgets/blob/master/CONTRIBUTING.md
Even though we currently discourage it, any new pull requests and
changes to it are automatically compiled and unit tested using Travis
CI. And sadly, I'm just not aware of any easy way to configure this to
work with Trac patches.
My thoughts:
There are certainly some drawbacks and advantages to both systems, but
I think that as long as there's more core developers aware that
patches might come in from either location, and there's at least a few
actively watching and handling new pull requests, it's really not a
problem to give contributors the choice. It's certainly easier for a
lot of developers, and that's very likely to encourage more
contributions we might not have ever seen anyone bother with.
--
Regards,
Bryan Petty
we do with pull requests right now:
GitHub PRs can not be turned off on any GitHub repository. Just Issues
can, and they have been turned off for all wxWidgets repos except for
the website. So regardless of whether we advise users to avoid them or
not, they will likely still come in.
Right now, we have an automated bot leaving a standard reply to all
pull requests opened. For example, this isn't actually me replying to
this one: https://github.com/wxWidgets/wxWidgets/pull/18
This notice is also shown to all users just prior to submitting a new
pull request to the wxWidgets repo, advising them that we use Trac
instead:
https://github.com/wxWidgets/wxWidgets/blob/master/CONTRIBUTING.md
Even though we currently discourage it, any new pull requests and
changes to it are automatically compiled and unit tested using Travis
CI. And sadly, I'm just not aware of any easy way to configure this to
work with Trac patches.
My thoughts:
There are certainly some drawbacks and advantages to both systems, but
I think that as long as there's more core developers aware that
patches might come in from either location, and there's at least a few
actively watching and handling new pull requests, it's really not a
problem to give contributors the choice. It's certainly easier for a
lot of developers, and that's very likely to encourage more
contributions we might not have ever seen anyone bother with.
--
Regards,
Bryan Petty
Bryan Petty
Feb 23, 2015, 1:20:44 PM2/23/15
to wxWidgets Development
On Mon, Feb 23, 2015 at 11:13 AM, Bryan Petty <br...@ibaku.net> wrote:
> There are certainly some drawbacks and advantages to both systems, but
> I think that as long as there's more core developers aware that
> patches might come in from either location, and there's at least a few
> actively watching and handling new pull requests, it's really not a
> problem to give contributors the choice. It's certainly easier for a
> lot of developers, and that's very likely to encourage more
> contributions we might not have ever seen anyone bother with.
Just to clarify one point: I still think Issues should remain off, at
> There are certainly some drawbacks and advantages to both systems, but
> I think that as long as there's more core developers aware that
> patches might come in from either location, and there's at least a few
> actively watching and handling new pull requests, it's really not a
> problem to give contributors the choice. It's certainly easier for a
> lot of developers, and that's very likely to encourage more
> contributions we might not have ever seen anyone bother with.
least until we find an easy way to bridge/sync them with Trac issues.
We can continue to only leave the option of submitting patches and not
bug reports to GitHub.
--
Regards,
Bryan Petty
Vadim Zeitlin
Feb 23, 2015, 8:17:59 PM2/23/15
to wxWidgets Development
On Mon, 23 Feb 2015 11:13:47 -0700 Bryan Petty wrote:
BP> GitHub PRs can not be turned off on any GitHub repository. Just Issues
BP> can, and they have been turned off for all wxWidgets repos except for
BP> the website.
FWIW I definitely agree that issues should remain turned off.
BP> Right now, we have an automated bot leaving a standard reply to all
BP> pull requests opened. For example, this isn't actually me replying to
BP> this one: https://github.com/wxWidgets/wxWidgets/pull/18
Presumably this bot could be modified to open a Trac ticket instead?
BP> There are certainly some drawbacks and advantages to both systems, but
BP> I think that as long as there's more core developers aware that
BP> patches might come in from either location, and there's at least a few
BP> actively watching and handling new pull requests, it's really not a
BP> problem to give contributors the choice. It's certainly easier for a
BP> lot of developers, and that's very likely to encourage more
BP> contributions we might not have ever seen anyone bother with.
The simplest solution is indeed to just allow both, but if we could have
at least a one directional synchronization, i.e. create (and update?) Trac
tickets when a PR is created (or changed), it would be even better. And the
ideal solution would be to have a two way synchronization, i.e. also update
(e.g. close) PRs when Trac ticket is updated, but this would almost
certainly be too difficult...
Regards,
VZ
BP> GitHub PRs can not be turned off on any GitHub repository. Just Issues
BP> can, and they have been turned off for all wxWidgets repos except for
BP> the website.
FWIW I definitely agree that issues should remain turned off.
BP> Right now, we have an automated bot leaving a standard reply to all
BP> pull requests opened. For example, this isn't actually me replying to
BP> this one: https://github.com/wxWidgets/wxWidgets/pull/18
Presumably this bot could be modified to open a Trac ticket instead?
BP> There are certainly some drawbacks and advantages to both systems, but
BP> I think that as long as there's more core developers aware that
BP> patches might come in from either location, and there's at least a few
BP> actively watching and handling new pull requests, it's really not a
BP> problem to give contributors the choice. It's certainly easier for a
BP> lot of developers, and that's very likely to encourage more
BP> contributions we might not have ever seen anyone bother with.
The simplest solution is indeed to just allow both, but if we could have
at least a one directional synchronization, i.e. create (and update?) Trac
tickets when a PR is created (or changed), it would be even better. And the
ideal solution would be to have a two way synchronization, i.e. also update
(e.g. close) PRs when Trac ticket is updated, but this would almost
certainly be too difficult...
Regards,
VZ
Bryan Petty
Feb 23, 2015, 9:54:54 PM2/23/15
to wxWidgets Development
On Mon, Feb 23, 2015 at 6:17 PM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
> On Mon, 23 Feb 2015 11:13:47 -0700 Bryan Petty wrote:
>
> BP> Right now, we have an automated bot leaving a standard reply to all
> BP> pull requests opened. For example, this isn't actually me replying to
> BP> this one: https://github.com/wxWidgets/wxWidgets/pull/18
>
> Presumably this bot could be modified to open a Trac ticket instead?
Supposedly getting that far wouldn't be terribly hard as long as the
> On Mon, 23 Feb 2015 11:13:47 -0700 Bryan Petty wrote:
>
> BP> Right now, we have an automated bot leaving a standard reply to all
> BP> pull requests opened. For example, this isn't actually me replying to
> BP> this one: https://github.com/wxWidgets/wxWidgets/pull/18
>
> Presumably this bot could be modified to open a Trac ticket instead?
XmlRpcPlugin is installed and configured correctly in Trac:
http://trac-hacks.org/wiki/XmlRpcPlugin
I'd still have some work to do from there, but I see your point. If it
can at least do that, it's definitely an argument in support of not
actively discouraging PRs.
--
Regards,
Bryan Petty
Bryan Petty
Mar 15, 2015, 3:12:02 PM3/15/15
to wxWidgets Development
On Mon, Feb 23, 2015 at 7:54 PM, Bryan Petty <br...@ibaku.net> wrote:
> Supposedly getting that far wouldn't be terribly hard as long as the
> XmlRpcPlugin is installed and configured correctly in Trac:
> http://trac-hacks.org/wiki/XmlRpcPlugin
Are you able to install this plugin Vadim, or does Robin have to do it?
> Supposedly getting that far wouldn't be terribly hard as long as the
> XmlRpcPlugin is installed and configured correctly in Trac:
> http://trac-hacks.org/wiki/XmlRpcPlugin
--
Regards,
Bryan Petty
Vadim Zeitlin
Mar 15, 2015, 7:57:13 PM3/15/15
to wxWidgets Development
On Sun, 15 Mar 2015 13:12:00 -0600 Bryan Petty wrote:
BP> On Mon, Feb 23, 2015 at 7:54 PM, Bryan Petty <br...@ibaku.net> wrote:
BP> > Supposedly getting that far wouldn't be terribly hard as long as the
BP> > XmlRpcPlugin is installed and configured correctly in Trac:
BP> > http://trac-hacks.org/wiki/XmlRpcPlugin
BP>
BP> Are you able to install this plugin Vadim, or does Robin have to do it?
I think shell access to the Trac server is required for this, isn't it?
And I don't have it (and never did), I just have the admin access to the
Trac site (which I believe you have as well). So I don't think I can do
this, unless I'm missing something.
Regards,
VZ
BP> On Mon, Feb 23, 2015 at 7:54 PM, Bryan Petty <br...@ibaku.net> wrote:
BP> > Supposedly getting that far wouldn't be terribly hard as long as the
BP> > XmlRpcPlugin is installed and configured correctly in Trac:
BP> > http://trac-hacks.org/wiki/XmlRpcPlugin
BP>
BP> Are you able to install this plugin Vadim, or does Robin have to do it?
I think shell access to the Trac server is required for this, isn't it?
And I don't have it (and never did), I just have the admin access to the
Trac site (which I believe you have as well). So I don't think I can do
this, unless I'm missing something.
Regards,
VZ
Bryan Petty
Mar 16, 2015, 1:03:06 AM3/16/15
to wxWidgets Development
On Sun, Mar 15, 2015 at 5:57 PM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
> I think shell access to the Trac server is required for this, isn't it?
> And I don't have it (and never did), I just have the admin access to the
> Trac site (which I believe you have as well).
Probably right about shell access. I still don't have admin access
> I think shell access to the Trac server is required for this, isn't it?
> And I don't have it (and never did), I just have the admin access to the
> Trac site (which I believe you have as well).
though, so I wasn't sure.
--
Regards,
Bryan Petty
Robin Dunn
Mar 26, 2015, 12:01:06 PM3/26/15
to wx-...@googlegroups.com
--
Robin Dunn
Software Craftsman
http://wxPython.org
Bryan Petty
Mar 26, 2015, 2:18:58 PM3/26/15
to wxWidgets Development
On Thu, Mar 26, 2015 at 10:01 AM, Robin Dunn <ro...@alldunn.com> wrote:
> Bryan Petty wrote:
>>
>> On Mon, Feb 23, 2015 at 7:54 PM, Bryan Petty<br...@ibaku.net> wrote:
>>>
>>> Supposedly getting that far wouldn't be terribly hard as long as the
>>> XmlRpcPlugin is installed and configured correctly in Trac:
>>> http://trac-hacks.org/wiki/XmlRpcPlugin
>>
>>
>> Are you able to install this plugin Vadim, or does Robin have to do it?
>>
>
> I've just installed the plugin, with all features enabled by default.
Thanks for doing this (and for admin).
> Bryan Petty wrote:
>>
>> On Mon, Feb 23, 2015 at 7:54 PM, Bryan Petty<br...@ibaku.net> wrote:
>>>
>>> Supposedly getting that far wouldn't be terribly hard as long as the
>>> XmlRpcPlugin is installed and configured correctly in Trac:
>>> http://trac-hacks.org/wiki/XmlRpcPlugin
>>
>>
>> Are you able to install this plugin Vadim, or does Robin have to do it?
>>
>
> I've just installed the plugin, with all features enabled by default.
I'll look into writing up some kind of GitHub PR to Trac bridge soon.
--
Regards,
Bryan Petty
Artur Wieczorek
Apr 1, 2015, 4:26:28 PM4/1/15
to wx-...@googlegroups.com
>
> I've just installed the plugin, with all features enabled by default.
>
>
I am not sure if this is an expected behaviour but submitted PR's (not
> I've just installed the plugin, with all features enabled by default.
>
>
yet merged) seems to be visible as changesets in the Trac's Timeline, e.g.:
https://github.com/wxWidgets/wxWidgets/pull/20
and
http://trac.wxwidgets.org/changeset/aa2055e5293c478c3bbfbd6a257f1444eba90ce7/git-wxWidgets
or
https://github.com/wxWidgets/wxWidgets/pull/18
and
http://trac.wxwidgets.org/changeset/68f1670d16916dac2d4239247d45fc961b388c7b/git-wxWidgets
Maybe it would be possible to filter them out somehow?
Kind regards,
AW
Bryan Petty
Apr 1, 2015, 5:28:45 PM4/1/15
to wxWidgets Development
On Wed, Apr 1, 2015 at 2:26 PM, Artur Wieczorek <art...@wp.pl> wrote:
> Maybe it would be possible to filter them out somehow?
I noticed that recently myself, and find it a little annoying as well.
> Maybe it would be possible to filter them out somehow?
I have adjusted the filters that control which git branch commits are
added to the timeline. It won't filter out existing entries, but it
should prevent new ones from showing up.
--
Regards,
Bryan Petty
Bryan Petty
Apr 2, 2015, 6:27:01 PM4/2/15
to wxWidgets Development
On Thu, Mar 26, 2015 at 12:18 PM, Bryan Petty <br...@ibaku.net> wrote:
> I'll look into writing up some kind of GitHub PR to Trac bridge soon.
I've done a lot of research into this now, and actually decided to
> I'll look into writing up some kind of GitHub PR to Trac bridge soon.
just extend our existing trac-github plugin with the features required
to do this instead of writing a new XML-RPC client. Figured it would
be best to simply leave this code running on the Trac installation
itself so there's one less point of failure, and so it can potentially
tie into ticket creation/update events in Trac itself more easily (no
polling).
So if you want to remove the XML-RPC plugin, that's fine, though it
certainly doesn't hurt to leave it either.
I have a partially working implementation going already:
https://github.com/aaugustin/trac-github/compare/master...tierra:issue-sync
On my test Trac installation, it's already creating new tickets for
all newly opened pull requests, and it also automatically attaches a
patch, along with new patches for all new commits added to a PR. But
that's basically just one-way sync for right now, it doesn't push
anything back to GitHub yet.
My Python is really rusty these days, if anyone is interested in
helping, I'd appreciate any code reviews.
--
Regards,
Bryan Petty
Vadim Zeitlin
Apr 3, 2015, 9:23:50 AM4/3/15
to wxWidgets Development
On Thu, 2 Apr 2015 16:26:59 -0600 Bryan Petty wrote:
BP> I have a partially working implementation going already:
BP> https://github.com/aaugustin/trac-github/compare/master...tierra:issue-sync
BP>
BP> On my test Trac installation, it's already creating new tickets for
BP> all newly opened pull requests, and it also automatically attaches a
BP> patch, along with new patches for all new commits added to a PR.
That's already great!
BP> But that's basically just one-way sync for right now, it doesn't push
BP> anything back to GitHub yet.
IMHO just closing the PR when the corresponding ticket is closed should be
enough in practice.
BP> My Python is really rusty these days, if anyone is interested in
BP> helping, I'd appreciate any code reviews.
I'm working on updating Buildbot config for git and I even have trouble
with the baby Python used there, so unfortunately I can't help here. I do
have a question though: would you have code for authenticating Github web
hooks somewhere by chance? It looks like Buildbot supports being notified
from Github about the changes but there is no support for checking that the
connection really comes from Github which makes me a little nervous...
TIA,
VZ
BP> I have a partially working implementation going already:
BP> https://github.com/aaugustin/trac-github/compare/master...tierra:issue-sync
BP>
BP> On my test Trac installation, it's already creating new tickets for
BP> all newly opened pull requests, and it also automatically attaches a
BP> patch, along with new patches for all new commits added to a PR.
That's already great!
BP> But that's basically just one-way sync for right now, it doesn't push
BP> anything back to GitHub yet.
IMHO just closing the PR when the corresponding ticket is closed should be
enough in practice.
BP> My Python is really rusty these days, if anyone is interested in
BP> helping, I'd appreciate any code reviews.
I'm working on updating Buildbot config for git and I even have trouble
with the baby Python used there, so unfortunately I can't help here. I do
have a question though: would you have code for authenticating Github web
hooks somewhere by chance? It looks like Buildbot supports being notified
from Github about the changes but there is no support for checking that the
connection really comes from Github which makes me a little nervous...
TIA,
VZ
Bryan Petty
Apr 3, 2015, 2:45:48 PM4/3/15
to wxWidgets Development
On Fri, Apr 3, 2015 at 7:23 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
> I do
> have a question though: would you have code for authenticating Github web
> hooks somewhere by chance? It looks like Buildbot supports being notified
> from Github about the changes but there is no support for checking that the
> connection really comes from Github which makes me a little nervous...
For simple push events, it's usually not a serious issue since it's
> I do
> have a question though: would you have code for authenticating Github web
> hooks somewhere by chance? It looks like Buildbot supports being notified
> from Github about the changes but there is no support for checking that the
> connection really comes from Github which makes me a little nervous...
really just notification that the repo might have changes. Trac is
subscribed to those already as well, and the plugin that subscribes to
them just fires a basic "do a remote update" event, and doesn't
actually verify that the hook came from GitHub either. If there's
changes, it still fetches them over the verified SSL remote URL, not
from the GitHub hook, so it really wouldn't even matter if the hook
came from someone else.
However, for the issue and pull request events I'm now subscribing to,
I do require hook verification, since otherwise, anyone could use the
endpoint to generate spam tickets or comments. GitHub offers an
optional hook "secret" when you configure a hook. It will sign the
HTTP request body with that secret string, leaving the signature in
the "X-Hub-Signature" HTTP header.
I verify that signature here (fetching the original secret from
trac.ini settings: hook_secret):
https://github.com/tierra/trac-github/blob/5ae0dac7/tracext/github.py#L44-L53
GitHub has more info about this, but mostly just helpful to Ruby:
https://developer.github.com/webhooks/securing/
--
Regards,
Bryan Petty
Vadim Zeitlin
Apr 6, 2015, 4:33:17 PM4/6/15
to wxWidgets Development
On Fri, 3 Apr 2015 12:45:46 -0600 Bryan Petty wrote:
BP> I verify that signature here (fetching the original secret from
BP> trac.ini settings: hook_secret):
BP> https://github.com/tierra/trac-github/blob/5ae0dac7/tracext/github.py#L44-L53
Thanks, it actually turns out that the latest version of buildbot 0.8
already does have support for it (I did check for this, but I looked in the
_really_ latest version, which is 0.9, and, somehow, it doesn't have this
yet) and so I could just take the code from there and Buildbot now verifies
that the hook submission really does have the correct hash.
And you're right that it probably doesn't matter in practice anyhow
because the worst somebody could do would have been the ability to trigger
rebuilds on all slaves which would quickly finish because there would be
nothing to rebuild, but I still feel better with the extra check.
Just sorry for bothering you needlessly,
VZ
BP> I verify that signature here (fetching the original secret from
BP> trac.ini settings: hook_secret):
BP> https://github.com/tierra/trac-github/blob/5ae0dac7/tracext/github.py#L44-L53
Thanks, it actually turns out that the latest version of buildbot 0.8
already does have support for it (I did check for this, but I looked in the
_really_ latest version, which is 0.9, and, somehow, it doesn't have this
yet) and so I could just take the code from there and Buildbot now verifies
that the hook submission really does have the correct hash.
And you're right that it probably doesn't matter in practice anyhow
because the worst somebody could do would have been the ability to trigger
rebuilds on all slaves which would quickly finish because there would be
nothing to rebuild, but I still feel better with the extra check.
Just sorry for bothering you needlessly,
VZ
Reply all
Reply to author
Forward
0 new messages