On Wed, Feb 24, 2010 at 3:10 AM, Alan Barrett <apb_at_cequrux.com> wrote:
> On Wed, 17 Feb 2010, dcz wrote:
>> Here is what I'm trying to do : some user (let's call them junior
>> developer) should require their commit to be authorized by other
>> (senior developer) before they would actually be committed.
>
> You could require the junior developer to put a line like this
> in the log message:
>
> Approved by: <insert name of senior developer>
>
> Then you could have a pre-commit hook that rejects commits from junior
> developers, unless that line is present in the log message. If the
> junior developer lies about whether the code has been approved, then
> you have a non-technical problem to solve outside svn. You could also
> extend this by requiring the log message to contain a reference number
> to an external database that tracks code reviews and approvals.
>
> --apb (Alan Barrett)
>
I know this is old but I wanted to chime in (actually found this
thread while searching for references to Review Board).
While my use case is slightly different (we want approvals for all
commits, not just from a specific type of user), if you're capable of
doing some coding, this is a solvable problem. Here's the method I'm
working on:
Developer:
1) Checks out repository and makes changes.
2) When changes are ready for review, generate a changelist.
a) A changelist ID (integer) is generated in an SQL database.
b) The changelist ID is used to create a local SVN client
changelist, with that ID as the name.
c) A custom SVN revprop is set with the changelist ID.
3) User emails out the diff, to a reviewer, and a conversation commences.
Reviewer(s):
4) When the review has been approved (including any changes requesting
during the review), the reviewer marks the change as approved.
a) The changelist ID, in the database, has an is_approved boolean
and this is set True.
b) An e-mail is sent to the developer, informing them that their
change has been approved.
Developer:
5) Developer commits their changelist.
a) Pre-commit script checks the changelist ID revprop and verifies
that is_approved and active are both True. (active gets set false,
after a successful commit, so that changelist IDs don't get reused).
b) Post commit script grabs the commit revision and emails out a
commit notice, with revision information, to the reviewer(s) and sets
the active boolean, in our DB, to False.
While this is a somewhat complex procedure, it's really simple to
implement if you have some coding skills. Since my last employer was
a junkie for Python, I did all of this using pysvn
(http://pysvn.tigris.org/). I'd just give out the code (and may
still, if I can get permission to OSS it) but, for now, it's the
intellectual property of my current employer. I attempted a couple
other methods but this one worked the best, with the fewest race
conditions. The best part though is that it's an easy sell. This
method is based on what I recall of Google's code review methods, from
2004-2006 (implemented without having ever seen their actual code...
so it could definitely be better).
Beyond the normal SVN subcommands, I had implement these:
abandon
-If the change is decided to be bad, abandon it completely.
approve
-Allow a reviewer to simply run "custom_svn approve --cl <changelist_id>".
change
-Generate a changelist, and changelist config, based on current
workspace modifications.
client
-We want people to be able to manage repos independently, for various
reasons. This could be left out.
mail
-A developer only has to run "custom_svn mail --cl <changelist_id>" to
send out a request for review. I'm starting work on getting this to
talk to Review Board now (thus the search).
I also had to hijack and slightly reimplement commit, so that we can
run "custom_svn commit --cl <changelist_id>" and work my magic.
If you really only want to require review and approvals for junior
developers, that wouldn't be that hard to add. You could have a user
list and have the pre-commit script only trigger when a user is/isn't
in a list. If I were to do it, I would probably maintain LDAP groups,
based on language, for people who you trust to not need a review.
Again, using Google as a reference, having a group (like
python-trusted) that people are added to, once they past some sort of
coding/style test, and skipping the hard verification test would
probably be ideal.
Wow that was long... Again, I know this is a month old but I wanted
to toss my thoughts into the ring. If I am able to OSS this, or parts
of it, I'll make an announcement here and/or on the pvsvn list (after
I make sure it's not against the rules). If someone wants to chat
more about my implementation, I'm perfectly willing to talk non
specifics and help where I can.
--
--tucker
Received on 2010-03-30 02:42:51 CEST