Let's reconsider approvals vs unresolved comments

[portante]

What is the point of approving a PR if you leave a number of comments to be resolved preventing the PR from being merged?

Why not instead don't approve the PR and leave your comments?

What are we gaining by the current behavior.

If I have all approvals but N unresolved comments, what is the difference between that and no approval?

[dbutenhof]

Yes; approving without resolving conversations is meaningless. I suspect the problem is more often that sometimes finding the open conversations is tedious at best, and in a long list of others' comments it can be easy to miss one.

I suspect it's also possible that I've sometimes approved a PR without bothering to check whether I've got unresolved conversations... and in one case, I had a single unresolved conversation that I deliberately left unresolved because "someone else" had replied to that conversation pushing it in a different direction and I thought that person ought to close it (but there's no official way to "hand off responsibility", so the resulting state is at best ambiguous).

[webbnh]

Assuming that we like and want to keep the restriction that all conversations must be resolved before the PR can be merged (which I do), the choice of Approve vs. Request-Changes vs. Comment loses almost all significance except what we attribute to it.

I use it to indicate my feeling toward the PR, as though it were the Reviewer-side analog of the Draft vs. "Ready" state: if I feel that the PR should not be merged, that there are significant bugs or other blockers in the proposed changes which must be fixed before I can offer approval, then I mark my review with "Request Changes"; however, if I mark it "Approved", it indicates that any open conversations are modest questions or suggestions which I want considered prior to merging and which really only require acknowledgement from the author -- that is, I would still approve the merge without further changes.

If this practice is frustrating or otherwise unhelpful, I can certainly change my approach. I can use "Comment" instead and reserve "Approved" for the point after I've received ack's and resolved all my conversations, but it's not obvious to me that that would be better or even significantly different.

As for conversations which linger, I try to be diligent and mark the ones that I started as resolved once the question has been settled or the suggestion/request has been implemented or declined. I'll grant that in a larger review this often requires more scrolling and clicking that might be desired, but it's not actually hard. I concur that we do run into some ambiguities when a single conversation includes more than one issue, particularly when raised by more than one reviewer (and, there's a GitHub pitfall which leads to this, in that you cannot have more than one conversation attached to the same line of code), but I think we can address this by being explicit in our responses: if you have an issue in play in a multi-issue conversation, post a reply when you think it has been resolved, and feel free to review open conversations which were started by other reviewers to confirm that resolving them hasn't fallen to you. My attitude is that the process of code review should be very much a team effort -- it's not just up to the author to gather reviews, and it's not just up to me to offer feedback and walk away -- if I'm involved in a PR, then I feel like it's incumbent upon me to see it through to a successful merge (assuming that the change is something that we agree should be done in the first place), and that includes supporting the other reviewers as well as the author.

[portante]

This discussion is coming up on a year still, and is one of the behaviors of our review process that leads me to the opinion that we should be using Gerrit for reviews because it provides the interfaces to accommodate the behaviors team members want to have for code review.

[webbnh]

This discussion is coming up on a year still

At the risk of launching a meta-discussion, I'm not sure what your expectations for this Discussion are/were. You asked some specific questions and received a couple of responses/answers, but there were no further exchanges...which might lead one to think that we were done here. (The fact that a year has passed since then doesn't speak to anything other than that nobody had anything to add and that there were no follow-up questions...and that no one decided to lock or delete the Discussion.)

[portante]

The discussion was not closed out, and the fact that there are 3 discussions open related to this topic, and that at times I still see evidence of the code review practices we have under GitHub negatively impacting the team is why I am poking at this bees nest.

I don't think we can discuss what we are going to do with the GitHub process of code-review until we decide, one way or the other, on switching to Gerrit.

[webbnh]

The discussion was not closed out

How exactly is that done? (And, how do we know when to engage that mechanism?)

the fact that there are 3 discussions open related to this topic

"Related", yes, but very distinct from each other.

I still see evidence of the code review practices we have under GitHub negatively impacting the team

At this point, I have no reason to believe that the root of those problems is GitHub!

I don't think we can discuss what we are going to do with the GitHub process of code-review until we decide, one way or the other, on switching to Gerrit.

That's a very sad statement: anything we do to improve our use of GitHub would be the imposition of conventions, procedures, or other sorts of discipline; and, it's quite likely to be orthogonal and/or transferrable to Gerrit.

Why would you put barriers in the way of improving the way of using the tool that we're already using?

[portante]

How exactly is that done? (And, how do we know when to engage that mechanism?)

Nobody closed the discussion, summarized it, and finalized any decision.

"Related", yes, but very distinct from each other.

But I don't believe they should be considered and resolved separately from each other because the relationship between the three of them is critical to each.

The typical cop shows use the tactic of separating known accomplishes in order to coerce one to turn on the other. Here, spending time to resolve them and then moving to Gerrit (if decided) is not worth the time or energy.

At this point, I have no reason to believe that the root of those problems is GitHub!

The tool does not help us, and too often we are sharing all kinds of work-arounds.

That's a very sad statement: anything we do to improve our use of GitHub would be the imposition of conventions, procedures, or other sorts of discipline; and, it's quite likely to be orthogonal and/or transferrable to Gerrit.

Not in this case. The review process under Gerrit is HELPED by the tool and facilitated. That does not happen with GitHub.

Why would you put barriers in the way of improving the way of using the tool that we're already using?

Potato, patato. Working on fixing a broken system instead of going to one that works is the barrier to improving our review process.

[webbnh]

How exactly is that done? (And, how do we know when to engage that mechanism?)

Nobody closed the discussion, summarized it, and finalized any decision.

Who was supposed to have done that?? And, when?? You posed some questions a year ago; you got nearly instant responses; and then nothing happened. Why didn't you close the discussion? Why didn't you summarize it? Why didn't you, based on the feedback you received, propose a decision?

The fact that none of these things happened is pretty much meaningless (unless your original post was some sort of stalking horse for getting us to Gerrit which, apparently failed).

"Related", yes, but very distinct from each other.

But I don't believe they should be considered and resolved separately from each other because the relationship between the three of them is critical to each.

The typical cop shows use the tactic of separating known accomplishes in order to coerce one to turn on the other. Here, spending time to resolve them and then moving to Gerrit (if decided) is not worth the time or energy.

This has nothing to do with "cops", and it should have nothing to do with getting people to turn on each other! The discussions were separated to allow them to be discussed coherently, and, they are, to a very large extent, orthogonal.

Yes, if we were going to go to Gerrit tomorrow, then the discussions would be moot. But, we've been discussing moving to Gerrit ever since I joined this project, and we're still discussing it. And, instead of making our existing processes more comfortable while we continue that discussion, you seem to be trying to ensure that we stay uncomfortable in our use of GitHub.

At this point, I have no reason to believe that the root of those problems is GitHub!

The tool does not help us, and too often we are sharing all kinds of work-arounds.

Huh. So, when did tips on how to make the tool work better for us become "work-arounds". I'm pretty sure that if we ever get to Gerrit we're going to find another set of foibles that we have to deal with....

The review process under Gerrit is HELPED by the tool and facilitated. That does not happen with GitHub.

Perhaps it doesn't happen for you with GitHub. I have managed to figure out how to use the tool, and I would be happy to share what I've learned with you to make it easier for you to use the tool, but every time I do so I'm met with a list of GitHub's faults and an explanation of how much better Gerrit is...but, as far as I can tell, we cannot use Gerrit, because, if we could, we would surely already be using now!

Working on fixing a broken system instead of going to one that works is the barrier to improving our review process.

Are you prepared to suspend all development on Pbench until we can get our Gerrit deployment up? Because, that's what I believe that it will take. (And, if we don't do something like that, then we're still going to be doing code reviews in GitHub while we perform the transition, in which case I would still like to make GitHub easier to use, so that we can get stuff done (despite our tool allegedly being "broken") while we're awaiting the arrival of the system which isn't broken.)

[dbutenhof]

I definitely agree about Gerrit; especially configuring with +1 vs +2 where +1 covers, literally, "looks good to me but someone else must approve", covering Webb's "yeah, maybe things could be improved but if others are satisfied it can go in anyway" while +2 (and we could configure to require 2 +2s if we wanted) means "ready to go".

Within the context of GitHub, however, we need to agree to some sort of protocol, and I don't know what to say here. I've even thought of putting non-essential comments into pre-resolved conversations, but that's not workable ... finding GitHub comments across commits (especially with rebases) is bad enough without expecting someone to find a resolved comment with useful context.

[webbnh]

As I said elsewhere (and probably repeatedly), I don't think it's worth extolling the virtues of Gerrit until we have a workable plan for getting there. I'm not hearing anyone say that we shouldn't switch to Gerrit, and so, if it were easy to switch, we would have done it by now!...so, if we want to discuss Gerrit, let's talk about how we get there, instead of talking about how bad we are at using GitHub.

Unless and until we commit to switching, I would be very interested in enabling us to use GitHub more comfortably, which I think we can do fairly easily. @portante is a big one for beating the "communications is key" drum, and I think that applies very well here: GitHub "conversations" are a communication mechanism; "approval" vs. "comment" vs. "changes requested" are a communication mechanism; we just need to agree on what they mean and what we expect of them.

I think that our major objections are driven by the facts that people seem to engage only sporadically; and, when PRs are too large, it then makes it harder to find lingering conversations. However, if we were to exercise a little discipline (keeping PRs small, and being responsive to open conversations), then I think most of the issues would go away.

[portante]

I definitely agree about Gerrit; especially configuring with +1 vs +2 where +1 covers, literally, "looks good to me but someone else must approve", covering Webb's "yeah, maybe things could be improved but if others are satisfied it can go in anyway" while +2 (and we could configure to require 2 +2s if we wanted) means "ready to go".

Exactly the benefit I have leveraged in the past with Gerrit.

But also: all comments are not lost with new commit updates, and when finalized the commit is still just one commit. You can always see unresolved comments easily, AND IN CONTEXT OF THE ORIGINAL COMMENT.

If you submit multiple commits for review they are tracked and treated as multiple commits.

Within the context of GitHub, however, we need to agree to some sort of protocol, and I don't know what to say here. I've even thought of putting non-essential comments into pre-resolved conversations, but that's not workable ... finding GitHub comments across commits (especially with rebases) is bad enough without expecting someone to find a resolved comment with useful context.

This is one of the reasons I would like to switch to Gerrit for reviews. I'd rather decide, for / against, switching to Gerrit, and only workout the GitHub protocol if we stay with it.

[webbnh]

I'd rather decide, for / against, switching to Gerrit, and only workout the GitHub protocol if we stay with it.

You say that like it would hard to come up with the "protocol", and I think that's a misconception.

That is, if we could actually discuss improving our present situation, without trying hard to highlight all of GitHub's faults and without constantly bringing up Gerrit (which I'm sure will be great if we can surmount all of the things which are preventing us from using it!), then I think we could make our lives much better without all that much effort.

[portante]

You say that like it would hard to come up with the "protocol", and I think that's a misconception.

You are entitled to your opinion. I am entitled to mine. Until GitHub changes we can't get around hidden context for discussions and review context. We don't have a way to encode the concepts of "been verified", "reviewed by somebody else has to approve", "approved officially."

Those are mechanisms of the tool that help. Creating work-arounds over GitHub just adds more complexity to developers. Teaching folks to use a very well-known process of code-review with Gerrit is straightforward.

That is, if we could actually discuss improving our present situation,

We are. We need to first accept or reject switching to Gerrit. If we accept, we can resolve in that environment. If we reject, we resolve in GitHub. Very straightfoward.

without trying hard to highlight all of GitHub's faults

They are the reason Gerrit is being considered.

and without constantly bringing up Gerrit

If there was another alternative that is viable, we could consider that. But the only other one is GitLab and that review process is not an improvement.

(which I'm sure will be great if we can surmount all of the things which are preventing us from using it!)

then I think we could make our lives much better without all that much effort.

Feel free to propose solutions that fix GitHub. I'll certainly consider them. I am personally not going to spend time coming up with solutions for GitHub since I'd rather spend my time on moving to Gerrit.

[webbnh]

You say that like it would hard to come up with the "protocol", and I think that's a misconception.

You are entitled to your opinion. I am entitled to mine.

Unfortunately, your opinion is moving you to say things like, "I don't think we can discuss what we are going to do with the GitHub process of code-review until we decide ... on switching to Gerrit," which sounds suspiciously like "shutting down discussion"; and, given that GitHub is the tool that we're using today, it presents a barrier to improving our immediate situation.

Until GitHub changes we can't get around hidden context for discussions and review context. We don't have a way to encode the concepts of "been verified", "reviewed by somebody else has to approve", "approved officially."

It seems to me that, if a reviewer "approves", then the PR has "been verified" (at least so far as that reviewer is concerned); we already have the repo configured to require two approvals, so if there is only one marked, then the PR has been "reviewed [but] somebody else has to approve"; and, if there are two approvals and no open conversations, then the PR has been "approved officially". We do this today, with GitHub as it is.

I'll grant that there are situations in which the context for discussions gets hard to deal with, but I think that there are several things we can do to address that.

• let's have smaller PRs, so that there is less stuff to get lost in (and less stuff likely to provoke comments);
• let's not amend our commits once the review has started;
• as reviewers, select an appropriate range of code lines (instead of a single line), so that anyone reading the comment (including the reviewer him-/herself in the future) has reasonable context;
• make use of the "Conversation" tab on the main PR page, which contains all of the conversations;
• be aware of what GitHub is actually doing and the ways in which it actually does support the reviewer/author;
• and, most of all, on the part of both the reviewer and the author, respond in a timely manner, so that the context doesn't get stale or muddled.

Those are mechanisms of the tool that help. Creating work-arounds over GitHub just adds more complexity to developers. Teaching folks to use a very well-known process of code-review with Gerrit is straightforward.

Please stop talking about Gerrit until we have a plausible way to adopt it.

We need to first accept or reject switching to Gerrit. If we accept, we can resolve in that environment. If we reject, we resolve in GitHub. Very straightfoward.

Not straightforward: we are using GitHub today -- except when we whine about how hard it is -- and we won't be transitioning to Gerrit this week (we don't even know if we can use Gerrit -- we have no proposal for how we would do so) -- so we would benefit from using GitHub better today, regardless of whether we transition to Gerrit in the future.

without trying hard to highlight all of GitHub's faults

They are the reason Gerrit is being considered.

But, they are not a valid point of discussion when what we are trying to consider is how to use GitHub more comfortably.

and without constantly bringing up Gerrit

If there was another alternative that is viable, we could consider that. But the only other one is GitLab and that review process is not an improvement.

But, alternatives are not going to help us this week, whereas some small bits of practice and discipline could.

Feel free to propose solutions that fix GitHub. I'll certainly consider them.

Really? Or will you just point out how we have multiple Discussions about GitHub, and about how my suggestions just highlight how bad GitHub is, and how we ought to move to Gerrit?

I am personally not going to spend time coming up with solutions for GitHub

Nobody is asking you to come up with solutions for GitHub. What I'm asking for is some willingness on your part to help improve the experience that we have with the tool that we're already using.

• Please stop proposing switching to Gerrit until it's actually a viable solution for us
• Please stop pointing out all of the shortcomings of GitHub for which Gerrit has a solution
• If there is something which is causing you pain in GitHub, please share that in a spirit of seeking help, and maybe we can ease your pain.