Posted by jthorson on June 10, 2011 at 9:07pm
Throughout our discussions, there have been some talk of a code review 'nirvana', where we automate various portions of the code review process, eliminating the need for manual intervention/attention.
We now have a 'drupal.org' development environment which has been set up for 'projectapps' related development. I'm currently using it to work on adding a 'created' date to the project application issue queue (with the current result available here) ... but my thoughts are now turning to how else we could leverage this resource.
Along these lines, please share your ideas on areas of the code review process which could be automated via a custom module ... I'll post two items to try and stimulate the discussion.
Comments
Ensuring that applicants have included a sandbox link
One approach might be to run a daily cron job which scans through new project applications, loading the description and comments for each new application, and parsing through them looking for a personal sandbox link. If no link is found, the script would then automatically post a comment asking for the submitter to provide the link to their sandbox within the project application thread.
Automate 'Coder' module reviews
I don't think anyone would disagree with the value of having automated 'Coder' module reviews of new applications ... I'd envision an implementation similar to the automated patch testing which occurs in other drupal.org issue queues (i.e. A "Project Application failed coder review" or "Coder Review Passed" comment automatically added to the project application thread based on some sort of daily/weekly cron run).
The implementation of this, however, may require more work to complete than the manual run of each application through the Coder module; in which case the challenge may outweigh the benefit. A few things that I think would be require include:
Coder false positives, separate site?
The Coder warning on $ID$ has already been fixed in dev.
http://drupal.org/node/990940
That just needs a new stable release. I doubt we'll be able to get anything that isn't a clean stable release on drupal.org, as only stable releases are covered by the security team.
Given the overhead involved in running this on Drupal.org, maybe we should consider doing it on another site. That could get us the same benefits except the auto-posting comments.
qa.drupal.org
Just an FYI. I am not really sure where or if there is documentation on whats going on here, but there is indeed work being done on putting Coder reviews on qa.drupal.org. This may be in the scope of patches, but this will be the direction to go in getting this done on a project level.
--
zzolo
Automated coder
Do you know who would be best to talk to regarding where the status of this initiative is? (rfay?)
I'm going way out of my scope of knowledge here, but I suspect a potential implementation could look like this:
A cron job which:
A Drush installation to run project application code through coder
Custom 'Project Application'-specific coder rules, set up as per http://drupal.org/node/144172
A 'Jenkins' implementation to automatically pull down/update the sandbox git repositories for project applications, and trigger the Drush scripts
A Drupal install and new module to drive the cron job, host customized tweaks and optimizations, and define various actions/triggers based on the drush script returns.
Granted, I threw this together with no visiblity or knowledge of how qa.drupal.org is driven ... but does it look close? It seems to me that Project Applications makes a perfect 'limited use case' for testing and developing the automated coder concept before rolling it out to a larger 'core/contrib' usage base.
.
Had a brief talk with boombatower on IRC ... he said that qa.drupal.org already has support for sandbox projects, and already has coder support (it just throws up too many false positives to turn it on for most tests).
We would likely need to add a new hook to the PIFT module running on drupal.org to trigger sending a test request over to the qa.drupal.org environment (what would our triggering requirements be?), and a few extra tweaks to set up our response messages et al ... but from the looks of things, automating coder reviews of project applications may not be nearly as difficult as one might assume.
We're apparently expecting over 4 inches of rain this weekend ... so long as it doesn't come along with lightning, I may have a few good hours to spare to investigate further over the next couple of days.
Automated Coder Update
Had a dig through the PIFT and PIFR code, with the goal of investigating possible approaches to enable automated Coder Reviews for project applications. After a brief education on how qa.drupal.org actually worked, I figured there were two ways we could approach it:
OR
While the first approach probably caused a few people to cringe (and rightfully so), I thought it would be a good first goal for learning the environment, even if it didn't actually make it to any sort of official implementation ... so I started there. While I learned a lot from the code, I unfortunately didn't get very far on the proof of concept side.
The current qa.drupal.org infrastructure keys off of a hook_versioncontrol_git_refs_update() function implementation, inside of which the first step is to check whether testing is enabled for a given project repository. Testing can be enabled via a checkbox on the node/$nid/edit/issues page by anyone who has the appropriate permission ... unfortunately, this page only exists for full projects; and the 'enable testing' checkbox isn't even available for sandbox repositories. This "pift_project_enabled()" check is called in multiple places throughout the PIFT code; meaning that the first step towards Automated Coder runs is building a new way of enabling testing on projects sandbox projects - without access to the 'enable testing' checkbox.
Really, all this needs to do is add another entry into the {pift_project} database table to satisfy the pift_project_enabled() condition, which in itself is not difficult ... but this is where I'd like to solicit some feedback from the group. I'm willing to work on adding certain 'one-time request' or 'on-demand' code review triggers into PIFT ... but first, what should the workflow be regarding automated coder reviews for project applications?
As always, your comments/thoughts/suggestions are appreciated.
EDIT: Some further thoughts after another conversation with Boombatower:
* I think I'm close to having enough understanding to code a first draft of this ... my current vision is to add a new 'one-time, on demand' request functionality into the PIFT project, which would support a release review of sandbox repositories. However, in order to avoid swamping qa.drupal.org with test requests for every sandbox under the sun, I think we need to determine some workflow and governance around how and under what conditions we would trigger this review request ...
Awesome work, @jthorson. IMO,
Awesome work, @jthorson.
IMO, it would be ideal if:
--
zzolo
Moved to dedicated page
This could become quite a long conversation ... let's move discussion over to http://groups.drupal.org/node/158049 .
Cheat sheet
It seems every time I start a review on a new project application, I am finding the same issues over and over. I think its likely that many applicants are unprepared because of the often overwhelming amount of documentation on coding and documentation standards. Beyond that, many applicants do not take the time to research the application process before submitting their application to the queue. Considering this, I propose that we create a simple, short, easy to understand "cheat sheet" of common module issues and add a link to each new project application when it is created. Applicants will be watching the issue, and with a properly formatted message explaining that they must click the link and follow the steps before their application can continue, I think this could mitigate a lot of the common issues that arise at the beginning of reviews. Additionally, while it would be nice to automate checking for LICENSE and README files, this could be a temporary solution that may be nearly as effective in the mean time.
It wouldn't be difficult to put together a brief list of common issues of which to notify new applicants.
-Run your module through coder on minor (most).
-Add a README.txt file detailing installation and setup of your module.
-Remove any licensing documents, this will be added by git.
-Remove CVS (
// $Id$and similar) tags.-Ensure all functions are properly documented (http://drupal.org/node/1354).
-Review coding standards at http://drupal.org/coding-standards
There are many cases in which applicants have no knowledge of these requirements and resources, and that must be because current documentation is not being found or read before applying. Providing a simple and brief list that is displayed at the start of an application, applicants may be more likely to complete these simple tasks before any reviewer even sees the module. I'd be glad to put together a formal cheat sheet if anyone likes this idea as a median solution to more complicated automated tasks.
What say you?
.
Great idea ... I suspect that many reviewers have something similar which they use themselves already.
I'd set the goal higher, though ... rather than posting to the application, I'd link getting this information into the hands of applicants before they even submit their application. Such a cheat-sheet would be a great child page to add to http://drupal.org/node/1011698.
Well, I suggest posting it on
Well, I suggest posting it on the project application because I think most of the time these problems exist in applications it's precisely because they didn't find the correct documentation elsewhere. There is certainly documentation that talks about all this, but I suspect the problem is that either it seems lengthy, seems trivial, or applicants simply don't find it before submitting their applications. Posting it on the application itself, or at least a link to it, would all but guarantee they see it. Unless there is another suggestion that I'm not considering. For me, the purpose is not only to resolve issues before a reviewer ever comes along but also to help applicants along by showing them common problems that they otherwise might not have found.
If you think posting it the
If you think posting it the would get it enough attention then I'm all for it. I guess we could try it there first and see how it works out. That would be the easier solution at the moment anyways since we don't yet have automation of project application posts. I think it should just be made clear that it is important that these steps are taken with any new project application. Ultimately, this is certain to reduce the amount of review time for each individual project so I can't see any reason for objection on this one.
.
I wasn't suggesting to NOT post it on the application ... but IMO, the existing documentation which directs people to the application queue in the first place could also benefit from having more information regarding the things one could do to help expediate their application.
Edit: I mean this in the sense of having a 'concise list' of items, linked prominently at the forefront of the 'how to apply' section ... positioned as a "Before you apply" list. I think the current "things to expect" post has grown to the point where it could be broken up into a few individual pages.
Aye. I'll try to work on a
Aye. I'll try to work on a draft for this.
Yes, but ...
I think it's good to have a documented list of these things, but we need to be careful about how we present this list to applicants. First, I think it's worth looking back at this discussion in which we trimmed down the reading up front because it was way too long to expect anyone to actually read:
http://drupal.org/node/703116
One way to reduce this list is to remove anything Coder already tells people, since we're already telling them to run it through Coder.
If we do end up posting such a list directly in applications, I think it's incredibly important we restrict the items in that list to those relevant to a specific application, and not just post a generic list to every application. In other words, the item "add a README.txt" should never appear in an application that already has a README.txt. Waiting a really long time is bad; waiting a really long time while dealing with what feels like an automated response system is even worse.
All of that said, I think it's a great idea to document common issues.
Great link
Thanks for supplying that ... somehow I missed it in my earlier readings. :)
I think you make some good
I think you make some good points. Maybe the best way to go about it is to make any checklist of "what to do before your application is submitted" document concise and easy to read. I agree that having an automated response telling someone to do things that they might have already done seems like what happens when I call Bank of America, and we all hate that. I think we should eliminate the idea of posting a list like this directly in an application as customizing it for each applicant defeats the purpose of this idea. But I think the documentation and link to it (rather than placing all this information in their application issue) could be formatted in such a way that it doesn't seem like we're just trying to automate the review process by basically telling people to complete a list of tasks - and that's what I'll strive to do. Maybe more of an informative document on "What reviewers are looking for". I think this could serve dual purposes - to possibly resolve issues before they arise, and to keep applicants more informed as I continue to find that some are confused about the process. And considering that it would be informative and helpful to many applicants, I wouldn't think a link to it would be off-putting. We will see how it turns out, but I think it's definitely worth trying to format documentation better. The original reason that I posted this was because I find myself screening applicants that have not read any of the documentation on what to do before you submit your application, so the ultimate purpose of this is not only to improve the documentation and reduce common problems but also to ensure that applicants read the documentation in the first place. Thanks for your input!
Updated documentation
I did a fairly major rework of the articles under Applying for permission to create full projects tonight ... the new 'Tips for ensuring a smooth review' page might help with compiling a checklist.
Great title! Thanks!
Great title! Thanks!
A place already exists for this.
Please note that this technically already exists: http://drupal.org/node/1187664
The thing is that no one actually sees this or reads this. Here's my quick thoughts on the subject.
Short term: Application triage. Ensure that each new application has a stock response that says please read this and make sure you have described your project completely and looked at other possible similar modules.
Medium Term: Make this a block or otherwise consistent message on the this queue and applications.
Long Term: Make a workflow that ensures the applicants reads this.
--
zzolo
Well ... technically, that
Well ... technically, that particular link has only existed in that form since last night. ;)
But true enough, the majority of the information was already there, on the 'what to expect' page, and probably not being read by most applicants. Hopefully, with the new title and some better formatting, we'll have more people stopping by that page and start to see an increase in the quality of applications.
Of course, that brings up the question of how we would measure an 'increase in quality', as we're missing some basic metrics to measure where we are now!
my bad
Ah, sorry. Yeah I thought that was the What to Expect page.
--
zzolo
Should some of that cheat
Should some of that cheat sheet be put here? - http://drupal.org/node/add/project-issue/projectapplications
Perhaps
It would be nice to include at least a link to the cheat sheet on that page ... we might be able to throw an additional markup element in with a hook_form_alter() call. The rest of the page is generated directly with the project_issue module, so any changes we made there would impact all issue queue pages across Drupal.org.
The text comes from editing
The text comes from editing the submission guidelines for the project. That is unique for each project. I'm just not sure who has access to it to add anything there. I guess whoever has commit access to the projectapplications project.
Ah ...
I didn't realize that the text was actually project-specific ... if that's the case, then adding a link to the information should be as simple as filing a request in the drupal.org webmasters or infrastructure queue; though I think we've got a few folks around here who should have sufficient access as well.
Done
Submitted a Drupal.org Webmasters request to modify the 'project application' issue text to include a request for the intended core Drupal version, as well as adding the links to both the 'What to Expect' and 'Tips for ensuring a smooth review' documentation pages.
The issue for the request can be found at http://drupal.org/node/1198570.