Subject: Quick approval

From: Daniel van Vugt <[email-masked]>
Date: Tue, 09 Jul 2013 13:19:21 +0800

All,

I noticed some people are regularly only waiting for a single approval 
on their MPs and then top-approve themselves.

I think top-approving your own MPs is OK if there's already two or more 
reviews from others, and it's been idle for a few days. But merging with 
only one real review, on proposals which are not time critical, is 
probably not ideal. I think we should always be waiting for at least a 
second review, unless it's a time critical issue.

Maybe others in mir-team disagree?

- Daniel

---

From: Alexandros Frantzis <[email-masked]>
Date: Tue, 9 Jul 2013 10:16:52 +0300

On Tue, Jul 09, 2013 at 01:19:21PM +0800, Daniel van Vugt wrote:
> All,
> 
> I noticed some people are regularly only waiting for a single
> approval on their MPs and then top-approve themselves.
> 
> I think top-approving your own MPs is OK if there's already two or
> more reviews from others, and it's been idle for a few days. But
> merging with only one real review, on proposals which are not time
> critical, is probably not ideal. I think we should always be waiting
> for at least a second review, unless it's a time critical issue.
> 

I agree. If an MP is trivial (a typo, a simple fix), I think it's OK for
the first *reviewer* to top-approve it.

Thanks,
Alexandros

---

From: Alan Griffiths <[email-masked]>
Date: Tue, 09 Jul 2013 09:29:35 +0100

> Maybe others in mir-team disagree?

Daniel, thanks for flagging this - we shouldn't let our standards lapse.

Some MPs are trivial enough for it to be obvious there are no problems -
for these I don't care about the number of reviews or think we need to
wait "a few days". At the other extreme some MPs are complex enough that
a reviewer can only see that that there are no obvious problems - for
these it may take days and discussion to reach consensus.

I think it comes to trusting people to know the difference.

---

From: Kevin DuBois <[email-masked]>
Date: Tue, 9 Jul 2013 15:21:04 -0700

Right, if a review is big enough (say... for instance
~kdub/mir/surface-owns-renderer :D), it should at least get a few reviews,
and be up for 24hours so the whole planet can review it. However, if
smaller bug fixes go in with 1 review, thats ok. I think prudence/not
stepping on toes is ok for the size of the team we have at the moment

Cheers,
Kevin

On Tue, Jul 9, 2013 at 1:29 AM, Alan Griffiths <[email-masked]
> wrote:

> --
> Mir-devel mailing list
> [email-masked]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/mir-devel
>

---

From: Robert Ancell <[email-masked]>
Date: Wed, 10 Jul 2013 14:12:26 +1200

My thoughts on this are:

- If you are in ~mir-devel you are trusted to push changes into lp:mir. You
take responsibility for the changes you make.
- We don't want to push directly since this doesn't guarantee that the test
cases have been run for that commit, so we use merge proposals.
- Merge proposals are a tool to increase quality, not roadblocks to getting
changes in.
- The person who sets the merge proposal to "Approved" is taking
responsibility for that change. It can be a reviewer, or the proposer.
- There's no fixed number of reviews required. It's the responsibility of
the final approver to decide if there has been sufficient review of the
change. If you're in ~mir-devel, you are considered capable of judging how
much review is needed.

--Robert

On Tue, Jul 9, 2013 at 5:19 PM, Daniel van Vugt <
[email-masked]> wrote:

> All,
>
> I noticed some people are regularly only waiting for a single approval on
> their MPs and then top-approve themselves.
>
> only one real review, on proposals which are not time critical, is probably
> not ideal. I think we should always be waiting for at least a second
> review, unless it's a time critical issue.
>
> Modify settings or unsubscribe at: https://lists.ubuntu.com/**
> mailman/listinfo/mir-devel<https://lists.ubuntu.com/mailman/listinfo/mir-devel>
>

---

From: Daniel van Vugt <[email-masked]>
Date: Wed, 10 Jul 2013 10:16:46 +0800

I guess then, I would only ask mir-team to not be overconfident. We all 
make mistakes and we all fail to see some mistakes in proposals. It's 
best to have more pairs of eyes.

Waiting a bit longer won't affect throughput of landings, only latency.

On 10/07/13 10:12, Robert Ancell wrote:
> My thoughts on this are:
>
> - If you are in ~mir-devel you are trusted to push changes into lp:mir.
> You take responsibility for the changes you make.
> - We don't want to push directly since this doesn't guarantee that the
> test cases have been run for that commit, so we use merge proposals.
> - Merge proposals are a tool to increase quality, not roadblocks to
> getting changes in.
> - There's no fixed number of reviews required. It's the responsibility
> of the final approver to decide if there has been sufficient review of
> the change. If you're in ~mir-devel, you are considered capable of
> judging how much review is needed.
>
> --Robert
>
>
> On Tue, Jul 9, 2013 at 5:19 PM, Daniel van Vugt
> <[email-masked] <mailto:[email-masked]>>
> wrote:
>
>     All,
>
>     [email-masked] <mailto:[email-masked]>
>     https://lists.ubuntu.com/__mailman/listinfo/mir-devel
>     <https://lists.ubuntu.com/mailman/listinfo/mir-devel>
>
>

---

From: Robert Carr <[email-masked]>
Date: Tue, 9 Jul 2013 19:21:28 -0700

Sorry, I've been making a habit of this.

I thought the consensus was approve after one review except for 'large
changes'.

On Tue, Jul 9, 2013 at 7:16 PM, Daniel van Vugt <
[email-masked]> wrote:

> make mistakes and we all fail to see some mistakes in proposals. It's best
> to have more pairs of eyes.
>
>> <[email-masked] <mailto:daniel.van.vugt@**canonical.com<[email-masked]>
>> >>
>>
>> wrote:
>>
>>     All,
>>
>>     [email-masked] <mailto:Mir-devel@lists.**ubuntu.com<[email-masked]>
>> >
>>
>>     https://lists.ubuntu.com/__**mailman/listinfo/mir-devel<https://lists.ubuntu.com/__mailman/listinfo/mir-devel>
>>     <https://lists.ubuntu.com/**mailman/listinfo/mir-devel<https://lists.ubuntu.com/mailman/listinfo/mir-devel>
>> >
>>
>>
>>
> --
> Mir-devel mailing list

---

From: Robert Ancell <[email-masked]>
Date: Wed, 10 Jul 2013 14:26:26 +1200

> Waiting a bit longer won't affect throughput of landings, only latency.

That is only true if each change is independent, and I'd assert that's not
the case.

Note, if we take too much time to make each merge perfect we lose agility.
If a merge improves the current codebase then it is worth merging
regardless of how much better you could make it. You can always make a
second merge proposal to build on the previous one.

--Robert

---

From: Daniel van Vugt <[email-masked]>
Date: Wed, 10 Jul 2013 10:42:41 +0800

I agree that sometimes land-it-now-and-fix-it-later is the fastest path 
to quality. I'm not sure how to gauge when that holds true...

I am however sure that I've seen many merge proposals where it was only 
the 2nd, 3rd or 4th reviewer who spotted serious issues, that the prior 
reviewers missed. If we catch those issues in review then it takes hours 
or days to fix. If we let them land then we might not find the problem 
for weeks or months.

On 10/07/13 10:26, Robert Ancell wrote:
> On Wed, Jul 10, 2013 at 2:16 PM, Daniel van Vugt
> That is only true if each change is independent, and I'd assert that's
> not the case.
>
> Note, if we take too much time to make each merge perfect we lose
> agility. If a merge improves the current codebase then it is worth
> merging regardless of how much better you could make it. You can always
> make a second merge proposal to build on the previous one.
>
> --Robert
>