The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process, yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code. What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people, or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
OK, go! :)
Hi Simon,
On Fri, 2011-08-05 at 00:24 +1000, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process,
Yes, there is. A tradition of opening up every commit to review and discussion on the mailing list.
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code.
Note that even those who have commit rights must get an ack before pushing.
What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people,
I could be wrong, but I think that actually is our model. If it's not, then I agree with you.
or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
I think that having patches go across the mailing list before committing is still a fine idea.
But I do agree that we should have a low bar for giving people push rights. The patch review process and the ability to revert give us enough of a safety net. We should trust and empower new contributors.
I also agree folks should be a lot more proactive about acking. What might be part of the problem here is that we seem to require patches be applied and tested before being acked on this project. I don't see why people shouldn't ack patches after just reading them.
Another thing - we should trust the author of patches to decide whether an ack is absolutely necessary. If you've sent a simple and obvious patch, waited a day and no-one has anything to say ... then IMHO, push it and ask for forgiveness later.
Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.
Cheers, Mark.
Hey Mark,
Thanks for the quick reply.
On 05/08/2011, at 12:53 AM, Mark McLoughlin wrote:
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I suppose I mean ala GitHub but it's also predicated on limited commit rights which you address further on. That said, to open up development to a wider community, GitHub style pull requests is a marvellous mechanism for doing so.
Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.
+a bazillion :)
On 08/05/11 - 12:57:55AM, Simon Harris wrote:
Hey Mark,
Thanks for the quick reply.
On 05/08/2011, at 12:53 AM, Mark McLoughlin wrote:
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I suppose I mean ala GitHub but it's also predicated on limited commit rights which you address further on. That said, to open up development to a wider community, GitHub style pull requests is a marvellous mechanism for doing so.
To be fair, I think pretty much everyone has commit rights to the repository. There are a couple of exceptions, but we can fix that if we want a really low barrier to entry. And as Mark said, even those with commit rights need to submit patches for review.
Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.
+a bazillion :)
I pretty much agree with everything else Mark said, as we come from the same school of thought. I really think the review process is valuable, but we should write it down.
On 08/04/2011 04:53 PM, Mark McLoughlin wrote:
Hi Simon,
On Fri, 2011-08-05 at 00:24 +1000, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process,
Yes, there is. A tradition of opening up every commit to review and discussion on the mailing list.
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code.
Note that even those who have commit rights must get an ack before pushing.
What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people,
I could be wrong, but I think that actually is our model. If it's not, then I agree with you.
or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
I think that having patches go across the mailing list before committing is still a fine idea.
But I do agree that we should have a low bar for giving people push rights. The patch review process and the ability to revert give us enough of a safety net. We should trust and empower new contributors.
I also agree folks should be a lot more proactive about acking. What might be part of the problem here is that we seem to require patches be applied and tested before being acked on this project. I don't see why people shouldn't ack patches after just reading them.
Note that some of us aren't hugely experienced and thinks do slip through out attention.
An important part of the review process is actually testing the functionality and making sure that nothing breaks. I think that's a good thing to have.
I agree that it may be overkill for small, obviously correct patches, but if it's at least a bit more complex, I don't think we should abandon applying and testing the code.
Another thing - we should trust the author of patches to decide whether an ack is absolutely necessary. If you've sent a simple and obvious patch, waited a day and no-one has anything to say ... then IMHO, push it and ask for forgiveness later.
Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.
Cheers, Mark.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
On 08/04/2011 09:26 AM, Tomas Sedovic wrote:
On 08/04/2011 04:53 PM, Mark McLoughlin wrote:
Hi Simon,
On Fri, 2011-08-05 at 00:24 +1000, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process,
Yes, there is. A tradition of opening up every commit to review and discussion on the mailing list.
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code.
Note that even those who have commit rights must get an ack before pushing.
What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people,
I could be wrong, but I think that actually is our model. If it's not, then I agree with you.
or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
I think that having patches go across the mailing list before committing is still a fine idea.
But I do agree that we should have a low bar for giving people push rights. The patch review process and the ability to revert give us enough of a safety net. We should trust and empower new contributors.
I also agree folks should be a lot more proactive about acking. What might be part of the problem here is that we seem to require patches be applied and tested before being acked on this project. I don't see why people shouldn't ack patches after just reading them.
Note that some of us aren't hugely experienced and thinks do slip through out attention.
An important part of the review process is actually testing the functionality and making sure that nothing breaks. I think that's a good thing to have.
Disagree.
If committers end up testing every review of every patch, committers become a single point of integration and become overloaded (and ultimately disinterested). An experienced maintainer should learn who to trust and apply their patches if they look sanitary.
To address your concerns of a regression introduced by a patch, projects typically implement a trailing commit regression testing system to identify specific patches that introduced a regression. If a regression was uncaught by automated testing requiring a developer to bisect the root cause of a regressoin, the patch offender should be held responsible for adding a regression test case.
Regards -steve
I agree that it may be overkill for small, obviously correct patches, but if it's at least a bit more complex, I don't think we should abandon applying and testing the code.
Another thing - we should trust the author of patches to decide whether an ack is absolutely necessary. If you've sent a simple and obvious patch, waited a day and no-one has anything to say ... then IMHO, push it and ask for forgiveness later.
Finally, we should write down exactly what our process is and use the same process across the various projects in Aeolus.
Cheers, Mark.
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
On Thu, Aug 04, 2011 at 12:16:30PM -0700, Steven Dake wrote:
On 08/04/2011 09:26 AM, Tomas Sedovic wrote:
On 08/04/2011 04:53 PM, Mark McLoughlin wrote:
Hi Simon,
On Fri, 2011-08-05 at 00:24 +1000, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process,
Yes, there is. A tradition of opening up every commit to review and discussion on the mailing list.
yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Do you specifically mean pull requests on github, or pull requests in general?
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code.
Note that even those who have commit rights must get an ack before pushing.
What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people,
I could be wrong, but I think that actually is our model. If it's not, then I agree with you.
or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
I think that having patches go across the mailing list before committing is still a fine idea.
But I do agree that we should have a low bar for giving people push rights. The patch review process and the ability to revert give us enough of a safety net. We should trust and empower new contributors.
I also agree folks should be a lot more proactive about acking. What might be part of the problem here is that we seem to require patches be applied and tested before being acked on this project. I don't see why people shouldn't ack patches after just reading them.
Note that some of us aren't hugely experienced and thinks do slip through out attention.
An important part of the review process is actually testing the functionality and making sure that nothing breaks. I think that's a good thing to have.
Disagree.
If committers end up testing every review of every patch, committers become a single point of integration and become overloaded (and ultimately disinterested). An experienced maintainer should learn who to trust and apply their patches if they look sanitary.
To address your concerns of a regression introduced by a patch, projects typically implement a trailing commit regression testing system to identify specific patches that introduced a regression. If a regression was uncaught by automated testing requiring a developer to bisect the root cause of a regressoin, the patch offender should be held responsible for adding a regression test case.
Regards -steve
Hmm this is a very interesting discussion, glad we're having it.
I am at least partly responsible for the "test before ack" policy on Aeolus. It is a throwback to oVirt, when we suffered many, many days of delay trying to integrate patches that failed not because the submitter hadn't tested them, but because they hadn't tested them on a clean install or their dev box was non-standard in other ways that masked failures. At this point the team has become completely accustomed to the routine of "apply this patch and smoke-test it before acking it."
We also give pretty much everyone on the project commit, once they've proven they understand what "tested" really means.
If the assembled multitude here agrees that test-before-ack is too onerous, given that we do have a reasonably good automated test framework in place, then I wouldn't object to a change. But it has served us fairly well for a long while, so don't consider dropping it lightly.
Take care, --Hugh
Hey Simon, thanks for the feedback w/ the project so far,
On 08/04/2011 10:24 AM, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process, yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
Not sure whats so 'arcane' about sending / reviewing patches, this is why git includes the 'send-email' and 'am' subcommands and I know many many communities that use this development methodology, such as the linux kernel itself [1] and puppet [2].
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.>
Yes but could potentially lead to reverting alot of commits. Since we'd want to review the code anyways (either before or after) it seems simpler to verify things as they go into the codebase.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people, or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Commit rights isn't denied to anyone. The entire process is very open, and as soon as someone has shown their commitment (haha pardon the pun) they get commit rights.
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
There are also tooling being developed aiming to help out with this, see Traapas [3] and a redmine plugin that some of us in the Syracuse Ruby community have been starting to hack on [4]
Of course nothing should ever be set in stone, if there are better ways to refine / run the process, would love to adopt them.
-Mo
[1] http://lkml.indiana.edu/hypermail/linux/kernel/
[2] http://projects.puppetlabs.com/projects/puppet/wiki/Development_Lifecycle#Ma...
On 08/04/2011 04:24 PM, Simon Harris wrote:
The whole patches sent to the list for review and approval process seems, how shall I put it, arcane. I certainly respect that there is an element of tradition to the current process, yet at the same time feel compelled to call 'Legless Turkey'. In my experience, a system that involves pull requests is easier to monitor, easier to review, easier to merge, etc.
I feel as though being a patcher without commit rights, I have to now also being a persistent nag in order to get something committed. This seems the reverse of what I would expect. IMVHO, if one has commit rights, one also has responsibility to be pro-active in reviewing code. What I see as good intentions -- ensure code is reviewed before committing -- leads to perverse incentives such as "you ack mine, and I'll ack yours."
I guess I grew up so to speak believing that if we share responsibility for the success of the project, we ought all be able to commit. If not, we probably have the wrong people on the project. Now in my case, you don't know me from a bar of soap so I respect that you won't just hand over commit rights. That said, it is called a version control system for a reason -- no matter what I did, it could always be undone. As a senior developer, I'd much rather monitor ALL commits and review them once committed.
In any event, may I humbly propose that we consider giving commit rights to a wider range of people, or at least moving to a model that provides the ability to perform pull requests. I don't even know if the hosting we have even supports them but if it doesn't, then perhaps it's time to find something that does?
Might I also humbly request that the onus of responsibility for getting patches reviewed shift from the patcher to the reviewer. I would, and do, certainly see that as my responsibility when I am in this position. As it stands, I see a bottlenecks where there really need not be.
I'm certain there are a bunch of organisational and/or political and/or historical issues that I have either missed, trivialised, and in some cases even poked in the eye with a burnt stick.
OK, go! :)
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
I do agree that we should give commit access to wider audience. It's not the case now (or at least it wasn't a few months back) and I think we should change that.
However, I do personally like the code -> patch -> review -> commit process.
This may stem from me being very young and inexperienced, but I have found incredible value in others' feedback to my patches. And thanks to me not pushing everything I've sent to the list, the git log looks much saner.
In turn, I have provided reviews to others' code that they found very helpful (or at least said so).
Doing this over the mailing list may be arcane and I'm all for better tooling (Mark noted there is one coming soon). But I still prefer reviews before the code is pushed to the master repo.
If only because it does incentivise people to do the reviews and makes the history look cleaner.
As for the "you ack mine, I'll ack yours" situation: anecdotally, I've never seen the pathological version of this happen here. It may become an issue as we grow but I haven't seen any evidence that it is one now.
Yes, I do trade reviews with the guys here, but as far as I can tell, they are no less diligent when they know I hold their patch in my hands. I don't think I'm softer either, though that may be a bias. Please point it out to me if that is the case.
thomas|shadow
On Thu, Aug 04, 2011 at 06:12:25PM +0200, Tomas Sedovic wrote:
I do agree that we should give commit access to wider audience. It's not the case now (or at least it wasn't a few months back) and I think we should change that.
Is there an actual policy here, or is it just informal? (I'm just curious.) I had always assumed that basically anyone who contributes was given commit access.
This may stem from me being very young and inexperienced, but I have found incredible value in others' feedback to my patches. And thanks to me not pushing everything I've sent to the list, the git log looks much saner.
I'm also young and inexperienced, but I totally agree with this. The other advantage is that it makes it easier to see what's going on -- if there are patches that need to be reviewed, I more or less *need* to pay attention to patches and thus (a) keep abreast of what's going on around me, and (b) have an opportunity to see others' work.
As for the "you ack mine, I'll ack yours" situation: anecdotally, I've never seen the pathological version of this happen here. It may become an issue as we grow but I haven't seen any evidence that it is one now.
I think it's more "review for a review" than "ACK for an ACK". It's certainly not ideal, but sometimes patches do built up and teaming up with someone else who has patches waiting is the most expedient way to get someone to look over a patch.
-- Matt
I completely respect and support wholeheartedly the desire to seek advice and feedback on commits and I understand that ACK is more about asking for a review though I don't believe mandating it (not that it has been necessarily) is appropriate unless it's actually been demonstrated to be a problem -- I've not ever experienced that) but perhaps it was on this project and I'm just coming on board too late to have seen it. Similarly, I don't see that the desire to keep abreast of, seek feedback for, and review commits as being predicated on sending patches to a list per se.
I view the argument that git supports feature X (eg send-email) therefore it's a good thing, as a somewhat circular one considering many features are in git mostly because that's what the linux kernel developers do as opposed to it being the most effective way to manage this or any other particular project. Git allows me to do many things I would consider bad practice.
I also don't personally feel the need to keep git history clean per se. In fact I actually like seeing commits reverted and the reasons for it. I think one source of history (as opposed to having to dig through a list) is actually a benefit.
All that said, I concede that the weight of support is behind the current process and I don't feel the need to argue for Just Another Process(tm). I'm too old for that but thankfully as yet not too old to change my habits to accommodate :)
On 08/05/11 - 03:28:59AM, Simon Harris wrote:
I completely respect and support wholeheartedly the desire to seek advice and feedback on commits and I understand that ACK is more about asking for a review though I don't believe mandating it (not that it has been necessarily) is appropriate unless it's actually been demonstrated to be a problem -- I've not ever experienced that) but perhaps it was on this project and I'm just coming on board too late to have seen it. Similarly, I don't see that the desire to keep abreast of, seek feedback for, and review commits as being predicated on sending patches to a list per se.
Just note that it is not *only* about receiving an ACK. It is also about exposing the change to a wider audience. Now, for your run-of-the-mill, purely technical patch that fixes a bug, there isn't very much to argue about. But for patches that change a particular behavior to one that may be controversial, it's much better to be transparent about it.
Case in point: the patch posted earlier today to re-enable automatic package installation in aeolus-configure. I think this is a particularly awful idea, but I never would have gotten to voice my opinion if someone just pushed the patch (or even if they had some review it in private for technical grounds).
aeolus-devel@lists.fedorahosted.org