Open Stack

On Tue, Aug 19, 2014 at 11:47 AM, Daniel P. Berrange
<berrange at redhat.com> wrote:
> On Tue, Aug 19, 2014 at 09:31:48PM +1200, Robert Collins wrote:
>> Hey everybody - https://wiki.openstack.org/wiki/TripleO/SpecReviews
>> seems pretty sane as we discussed at the last TripleO IRC meeting.
>>
>> I'd like to propose that we adopt it with the following tweak:
>>
>> 19:46:34 <lifeless> so I propose that +2 on a spec is a commitment to
>> review it over-and-above the core review responsibilities
>> 19:47:05 <lifeless> if its not important enough for a reviewer to do
>> that thats a pretty strong signal
>> 19:47:06 <dprince> lifeless: +1, I thought we already agreed to that
>> at the meetup
>> 19:47:17 <slagle> yea, sounds fine to me
>> 19:47:20 <bnemec> +1
>> 19:47:30 <lifeless> dprince: it wasn't clear whether it was
>> part-of-responsibility, or additive, I'm proposing we make it clearly
>> additive
>> 19:47:52 <lifeless> and separately I think we need to make surfacing
>> reviews-for-themes a lot better
>>
>> That is - +1 on a spec review is 'sure, I like it', +2 is specifically
>> "I will review this *over and above* my core commitment" - the goal
>> here is to have some very gentle choke on concurrent WIP without
>> needing the transition to a managed pull workflow that Nova are
>> discussing - which we didn't have much support for during the meeting.
>
> If it is considered to be a firm commitment to review the code, then
> people will have to be quite conservative when approving blueprints
> lest take accept too much work. It is hard to predict just how much
> work will be involved in the code review from the spec though, and
> even harder to predict /when/ during the dev cycle the code will be
> posted.

The commitment is not just about planning for optimization of your
review load across a cycle. Anyone is free to review anything at any
time. Including +2'ing patches implementing specs that you didn't +2.
I don't think patches that implement specs from individuals who aren't
the primary/secondary assignee are going to be rejected on that point
either. The review commitment is roughly the same as the assignee
commitment.

There's likely to be some set of specs that, as a reviewer, you're
going to prioritize reviewing the implementing patches regardless of
how busy you are....that's what the commitment is about. Should core
reviewers really be +2'ing specs for which they *don't* intend to
commit to review the patches? I suppose the individual projects will
have to decide what they each think will work for them.

To me, a +2 on a spec implies a couple of things, one of which is that
you agree that some amount of the project's resources should work on
it in terms of implementation and reviews. If you're +2'ing a spec,
who are you signing up to do that review workload if not yourself?

That being said, there are always going to be human factors at play.
Not all commitments can always be met. I don't think we're going to
"punish" people who are acting in good faith.


-- 
-- James Slagle
--