Add code review document

[squidsoup]

Add an initial document for code review practices, with some guidelines around etiquette (as discussed in the dev catchup).

I've also added a heading for Suggestions for Effective Review, which perhaps we should discuss as a group.

[nottrobin]

I'm not sure the "coding" folder is the right place for this. To my mind that's for things that are more specifically about programming.

I think this content is more similar to https://github.com/canonical-webteam/practices/blob/master/content/copy-reviews.md, and perhaps both should be in "workflow/reviews.md" or something?

This document should also be linked from https://github.com/canonical-webteam/practices/blob/master/workflow/github.md#pull-requests, or something else should be done to bring those pieces of content closer together.

(However, in the spirit of this PR, I don't think this point needs to block merging. We can restructure afterwards.)

Thoughts?

[squidsoup]

However, if a branch requires changes, no matter how small, one should always "request changes" to block merging.

I think this is a bit problematic, particularly for remote devs. In my case, a 'needs fixing' for something trivial, even if it is required, means an additional 24 hours to get the branch landed. If the changes are simple, do they really require additional review, or can we trust the author to do the correct thing?

[nottrobin]

@squidsoup I think the problem is exactly that we can't necessarily trust the author to do the right thing. If they see a big green "Merge" button, I'd say (from experience) odds are about 50/50 that they'll click it without reading further. And I think even trying to educate the whole team on this will only reduce it to about 25/75.

Personally I'd rather that you have an explicit power to override those sorts of blocking reviews under certain conditions.

[nottrobin]

@squidsoup I also have both messed up incredibly small fixes and seen others do so. So I also don't agree that the final review should be skipped without good reason. But your predicament probably counts as a good reason.

[shivjoshi1996]

@squidsoup can you have another look at this please. (Written by a person running standup)

[squidsoup]

@shivjoshi1996 Yep, given we couldn't reach consensus I'll close this PR.

[nottrobin]

I'm rather disappointed by that outcome. I don't believe the onus here was on @squidsoup to do anything at all, but rather on the rest of the team to offer their opinions. No individual, myself included, gets to reject proposals unilaterally, so I really would like to hear other people's thoughts on this.

If the silence of the rest of the team really does mean that none of them agree with this proposal then perhaps closing this PR is the correct outcome, but the fact remains that @squidsoup has a problem of being blocked on his work, which deserves addressing, and however we decide to address should probably be written down as a practice. I'd especially love to hear @anthonydillon's thoughts on how @squidsoup's blockers could be addressed.

@shivjoshi1996 I really appreciate your help with pushing us stay on top of PRs, but could we consider this practices repo something of a special case? Proposals here should be considered carefully and shouldn't be rushed (which is why they are not supposed to be merged less than 24 hours after being opened), so I would like to suggest that it's fine and possibly encouraged for PRs here to remain open for quite some time.

[squidsoup]

@nottrobin thanks for your thoughts, and appreciate your concern. Perhaps we could discuss this in more detail at the next dev meetup, and then think about what might make sense to have in this doc?

[shivjoshi1996]

@nottrobin the message above wasn’t written by me (I’m on holiday), I assume it was by a person running standup (i’ve Edited the original post to show that).

I agree, PR’s in this repo shouldn’t be a part of the normal iteration commitments. & should therefore stay open as long as needed.

@squidsoup feel free to reopen if you see fit.

:)

[nottrobin]

I see, apologies @shivjoshi1996. We kinda need a stand-up account...

[pmahnke]

I think this has been a really busy time and I personally haven't had time to think about it. We should reopen.

I think the new github changes feature might help with tiny fixes; however, I think skipping a proper review should be a bigger decision as Robin suggests.

Also, since you can base a branch off a branch, can't you nest them and make sure that the first one is reviewed first?

[pmahnke]

oh, there is a stand-up account... people just forgot about it

[squidsoup]

We broached this topic again in the last developer meeting, and the general consensus seemed to be that it would be helpful to allow for code-review approval with minor changes requested. This would require good judgement on the part of everyone involved and somewhat more responsibility on the part of the branch author.

If we were to consider this, it would be imperative that only the branch author lands their branches after approval. There was some suggestion that perhaps we should generally enforce that anyway.

[nottrobin]

We broached this topic again in the last developer meeting, and the general consensus seemed to be that it would be helpful to allow for code-review approval with minor changes requested. This would require good judgement on the part of everyone involved and somewhat more responsibility on the part of the branch author.

This sounds like a good solution. I think it's basically our soft policy anyway, in that people have been doing this - I've been doing this. So what we're saying here is that the onus is on the reviewer to decide if the changes they've requested are safe to be completed without another review? I'd like it if we could add a note to ask them to err on the side of caution. Maybe we could explicitly mention that it can be used to help avoid blocking people in other timezones.

If we were to consider this, it would be imperative that only the branch author lands their branches after approval. There was some suggestion that perhaps we should generally enforce that anyway.

Interesting, has general feeling swung back the other way on this one? I'm happy either way =)

[anthonydillon]

I have stopped merging other people PRs recently and I have not found branches are not landing. Therefore :+1: on switching the policy back.

[barrymcgee]

Just to re-iterate the reason we moved to this policy in the first place;

If code has passed all checks and has been peer-reviewed, why should it not be merged asap? By leaving it unmerged, you're just the leaving an extra step for the branch author to complete which you (the reviewer) could complete in a single second when clicking the big green button.

The other big advantage of this approach is that it means develop always contains the latest 'approved' version of the codebase so if another person comes along and cuts a new feature branch from develop, there is much less chance of merge conflicts when they come to merge their code from another approved branch which has been waiting in the wings for someone to press the button.

[squidsoup]

If code has passed all checks and has been peer-reviewed, why should it not be merged asap? By leaving it unmerged, you're just the leaving an extra step for the branch author to complete which you (the reviewer) could complete in a single second when clicking the big green button.

This applies to the case where the branch is approved with minor changes or suggestions. The author needs to be responsible for making those changes as they see fit before merging. If you allow anyone to merge the branch, the author may not have been satisfied with their amendments. Only the author is the position to make that judgement, so they need to be solely responsible for merging their branch.

[nottrobin]

This applies to the case where the branch is approved with minor changes or suggestions. The author needs to be responsible for making those changes as they see fit before merging. If you allow anyone to merge the branch, the author may not have been satisfied with their amendments. Only the author is the position to make that judgement, so they need to be solely responsible for merging their branch.

This is a more specific version of the reason I originally pushed for keeping authors as the only people who could merge initially (all that time ago). I often would leave a PR that I thought was finished, and then have l'esprit de l'escalier moments a bit later on and want to come back and slightly restructure or tidy something up before merging. If I found that PR was merged, I'd probably just never bother making that small improvement.

[nottrobin]

@barrymcgee I do think both of the points you raise actually are covered by @anthonydillon's point:

I have stopped merging other people PRs recently and I have not found branches are not landing.

Both your points are to do with there being problems arising from an extra delay in merging PRs. I can completely see the points in theory, but I do think whether we ever see them in practice is the real test.

I also personally haven't noticed a problem with PRs going unmerged. But we could well have something in policy protecting your right to merge someone else's PR if it's approved and you need it landed for some reason.

[squidsoup]

@nottrobin I've tweaked the copy, could you have another look when you have a moment please?

[nottrobin]

:+1: thanks @squidsoup, heroic effort here.

[steverydz]

:+1:

[anthonydillon]

:+1:

[nottrobin]

@squidsoup ready to merge whenever you like