Thread: commitfest management webapp
Back in January, there was some discussion of creation a web application to make it easier to manage CommitFests. http://archives.postgresql.org/message-id/20090127134245.GA6444@alvh.no-ip.org This was further discussed at PGCon, and I now have a working version for folks to play with. With the help of Dave Page, I was able to relearn how to use the BSD ports collection and get it up here: http://coridan.postgresql.org/ Feedback is appreciated, especially from people involved in previous commitfests as committers, reviewers, patch authors, or commitfest managers. The source code, in Perl, is available here: http://git.postgresql.org/gitweb?p=pgcommitfest.git;a=summary A few things to note: 1. You won't see a link anywhere to create a new CommitFest or edit the name of an existing CommitFest. This is not because the functionality doesn't exist, but because your community login account is not enabled with administrator privileges for this application. For the same reason, you won't be able to edit or delete comments other than your own. If you would like to have these great powers, please send me a private email with your community account name and I will power you up. If we decide to use this as official project infrastructure, then you might get un-powered up unless I or one of the CommitFest managers have some idea who you are. :-) 2. There are many things that this application doesn't do. One particularly glaring thing that it doesn't do is allow you to move a patch from one CommitFest to another CommitFest. This is basically a bug that I intend to fix, but it didn't seem necessary to fix it before putting the thing out there for people to look at and comment on. At any rate, if the application doesn't do something that you wish it did, please feel free to let me know what that thing is, or provide a patch. I'm very interested in making this better (unless of course you all hate it, in which case my interest in improving it will likely decline precipitously). 3. The integration with the community login system is currently rather poor. The problem is that we can't count on patch submitters to have a community login, and even if they do we can't count on the person adding the patch to the system to know what it is. We could of course require patch submitters to have a community login and to add their patches themselves, but I'm not really that keen on raising the bar for submitting a patch even to that modest extent. I'm open to suggestions on how to improve this situation, though, because it's definitely not ideal, and precludes things that reasonable people might want to do, like "contact the guy who submitted this patch", "contact the authors of all patches waiting for review", and similar. 4. The intent of this system is really just to get the data that is currently on the CommitFest pages into a database where, I venture to say, structured data about the development cycle of a database product properly belongs. I expect it to be possible to use this tool to build additional infrastructure to facilitate patch review, like an automated test to see whether all the latest versions of all the open patches actually still apply. (If we have a human being sanity check them to make sure they don't contain malicious code, we could also test for "compiles" and "passes regression tests", which would rock.) This infrastructure does not currently exist, but having the data in a database makes it feasible to think about doing it; suggestions are welcome, as is code. I know that there are some of you reading this who may think that we should convert to reviewboard or patchwork or some other system. I can say that personally I'm unimpressed by those suggestions because they will almost certainly require process changes that this does not, process changes which I suspect we're unprepared to make. But there's nothing to prevent you from setting up and advocating your system in this space. ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> Back in January, there was some discussion of creation a web
> application to make it easier to manage CommitFests.
> This was further discussed at PGCon, and I now have a working version
> for folks to play with.
Cool. Just reading your description, I have one thought:
> 3. The integration with the community login system is currently rather
> poor. The problem is that we can't count on patch submitters to have
> a community login, and even if they do we can't count on the person
> adding the patch to the system to know what it is. We could of course
> require patch submitters to have a community login and to add their
> patches themselves, but I'm not really that keen on raising the bar
> for submitting a patch even to that modest extent.
Agreed on that, though we have recently been asking people to do that
and most seem to have played along.
> I'm open to
> suggestions on how to improve this situation, though, because it's
> definitely not ideal, and precludes things that reasonable people
> might want to do, like "contact the guy who submitted this patch",
> "contact the authors of all patches waiting for review", and similar.
I don't understand why that bit would be based on community login at
all. Wouldn't contacting someone mainly need an email address?
regards, tom lane
On Tue, 26 May 2009, Robert Haas wrote: > I'm open to suggestions on how to improve this situation, though, > because it's definitely not ideal, and precludes things that reasonable > people might want to do, like "contact the guy who submitted this > patch", "contact the authors of all patches waiting for review", and > similar. Since you're taking the message-id where the patch was submitted at as an input, couldn't you scrape this information out of the archives? You probably want to do a bit of that regardless; having the program pull and display the author and subject line of the archived message is a good sanity check that you entered the message ID correctly. > I know that there are some of you reading this who may think that we > should convert to reviewboard or patchwork or some other system. I can > say that personally I'm unimpressed by those suggestions because they > will almost certainly require process changes that this does not We used Reviewboard a fair amount here at Truviso for a while. Lately a good chunk of that patch review has been happening more efficiently by passing pointers to private git branches around instead. I think you're right to focus on just the review workflow and not the patch review itself, let people use whatever tools they're already comfortable with for that part. I just spent a few minutes poking around your code and that quickly was able to see how things fit together, which is certainly not something I can say about Reviewboard etc. The interface looks good and the code easy enough to improve. The main concerns I'm left with after that review are with how to properly test the security of the code. Some maturity there is one major thing that more packages in larger use have going for them vs. rolling your own in this sort of situation. -- * Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD
Robert Haas escribió: > 3. The integration with the community login system is currently rather > poor. The problem is that we can't count on patch submitters to have > a community login, and even if they do we can't count on the person > adding the patch to the system to know what it is. We could of course > require patch submitters to have a community login and to add their > patches themselves, but I'm not really that keen on raising the bar > for submitting a patch even to that modest extent. Actually we already raised the bar -- people is supposed to add stuff to the commitfest pages on the wiki by themselves. Surely any patch submitter will find the two necessary minutes to create an account. I suggest you take it as a given that the submitter has an account already. For the cases on which this doesn't hold (i.e. some author just threw a quick oneliner to fix a typo in docs and is too lazy to follow procedure), somebody else will be responsibly and life will go on. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Tue, May 26, 2009 at 9:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 3. The integration with the community login system is currently rather >> poor. The problem is that we can't count on patch submitters to have >> a community login, and even if they do we can't count on the person >> adding the patch to the system to know what it is. We could of course >> require patch submitters to have a community login and to add their >> patches themselves, but I'm not really that keen on raising the bar >> for submitting a patch even to that modest extent. > >> I'm open to >> suggestions on how to improve this situation, though, because it's >> definitely not ideal, and precludes things that reasonable people >> might want to do, like "contact the guy who submitted this patch", >> "contact the authors of all patches waiting for review", and similar. > > I don't understand why that bit would be based on community login at > all. Wouldn't contacting someone mainly need an email address? Yes. Humor me be elaborating on your thought here? ...Robert
On Tue, May 26, 2009 at 10:53 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Robert Haas escribió: > >> 3. The integration with the community login system is currently rather >> poor. The problem is that we can't count on patch submitters to have >> a community login, and even if they do we can't count on the person >> adding the patch to the system to know what it is. We could of course >> require patch submitters to have a community login and to add their >> patches themselves, but I'm not really that keen on raising the bar >> for submitting a patch even to that modest extent. > > Actually we already raised the bar -- people is supposed to add stuff to > the commitfest pages on the wiki by themselves. Surely any patch > submitter will find the two necessary minutes to create an account. I > suggest you take it as a given that the submitter has an account > already. For the cases on which this doesn't hold (i.e. some author > just threw a quick oneliner to fix a typo in docs and is too lazy to > follow procedure), somebody else will be responsibly and life will go > on. There's a very good chance that the person who ends up "being responsible" will be me - and I have enough problems without people thinking that I own 25% of the patches in the CommitFest. ...Robert
On Tue, May 26, 2009 at 10:15 PM, Greg Smith <gsmith@gregsmith.com> wrote: > On Tue, 26 May 2009, Robert Haas wrote: >> I'm open to suggestions on how to improve this situation, though, because >> it's definitely not ideal, and precludes things that reasonable people might >> want to do, like "contact the guy who submitted this patch", "contact the >> authors of all patches waiting for review", and similar. > > Since you're taking the message-id where the patch was submitted at as an > input, couldn't you scrape this information out of the archives? You > probably want to do a bit of that regardless; having the program pull and > display the author and subject line of the archived message is a good sanity > check that you entered the message ID correctly. I think something like this might work. I had a suggestion previously of just checking that the message-IDs are even valid, which might be a good place to start, and then I could try to figure out how to extend it along these lines. I'm not totally keen on pulling the subject lines. I know that's what we've mostly been doing, but sometimes the subject line is something like "patch to improve the way that foo does bar", rather than "make bar use baz algorithm" or (even worse) "patch to add support for foo" rather than "foo". Also, I think we may also want to assign each patch a shortname that can be used as an argument to command-line tools. I'd really like to be able to do something like this: $ pgcf-patch foo ...and have foo.patch show up in $CWD. Even swankier would be to make this integrate with git somehow. >> I know that there are some of you reading this who may think that we >> should convert to reviewboard or patchwork or some other system. I can say >> that personally I'm unimpressed by those suggestions because they will >> almost certainly require process changes that this does not > > We used Reviewboard a fair amount here at Truviso for a while. Lately a > good chunk of that patch review has been happening more efficiently by > passing pointers to private git branches around instead. I think you're > right to focus on just the review workflow and not the patch review itself, > let people use whatever tools they're already comfortable with for that > part. Thanks and well said. > I just spent a few minutes poking around your code and that quickly was able > to see how things fit together, which is certainly not something I can say > about Reviewboard etc. The interface looks good and the code easy enough to > improve. The main concerns I'm left with after that review are with how to > properly test the security of the code. Some maturity there is one major > thing that more packages in larger use have going for them vs. rolling your > own in this sort of situation. Well, the nice thing about it is that it's not a ton of code, so visual inspection ought to buy you something. But I obviously can't and don't promise that it is free of bugs, security-related or otherwise. I was pretty dismayed when I realized that Template's "| html" filter did not think apostrophes needed to be quoted, which they obviously do if you're going to use them in a context like <a href='[% foo | html %]'>. Now that's the one I caught - question is - what did I miss? ...Robert
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, May 26, 2009 at 9:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't understand why that bit would be based on community login at
>> all. �Wouldn't contacting someone mainly need an email address?
> Yes. Humor me be elaborating on your thought here?
Um, what's to elaborate? I'm thinking you should track submitters
and other actors by email address. A login might be appropriate to
control who can modify the commitfest data, but that should be
seen as a secretarial function, not something that's necessarily
carried out by the patch authors or reviewers themselves.
regards, tom lane
Robert Haas wrote: > I know that there are some of you reading this > who may think that we should convert to reviewboard or patchwork or > some other system. I can say that personally I'm unimpressed by those > suggestions because they will almost certainly require process changes > that this does not, process changes which I suspect we're unprepared > to make. But there's nothing to prevent you from setting up and > advocating your system in this space. well fwiw the patchwork demo system(http://trackerdemo.postgresql.org/) is still up for people to look at. However going that route would also require some hackery on the code because patchworks MIME-parser is not really up to speed so it is actually missing some patches at times... Stefan
On Tue, 26 May 2009, Robert Haas wrote: > I'm not totally keen on pulling the subject lines. I know that's what > we've mostly been doing, but sometimes the subject line is something > like "patch to improve the way that foo does bar", rather than "make > bar use baz algorithm" or (even worse) "patch to add support for foo" > rather than "foo". I wasn't suggesting you pull the subject line and use it as a key for anything, was just thinking it would be nice to display it, as a way to double-check it points to the right place. -- * Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD