Reviewing prprocessor's review labels

Development RFCs
1

In Improve review labels for PRs · Issue #3155 · theforeman/foreman-documentation · GitHub it came up the labels are confusing. I’ll repeat the conversation here.

It’s important to know that there are 3 ways to submit a review:

  • Comment
  • Approve
  • Request changes

This shows up in the UI:
Screen Shot 2024-07-23 at 14.27.26

We then have the prprocessor app that applies labels. The code is hopefully telling:

You should notice we only act on Approve and Request changes, but in practice I often see use the other one: Comment. We can easily start treating that the same as Request changes, but we don’t do that today. Question is: should we?

1 Like
2

Problem with comment is, we do not have an automatic context. It could be “I comment because I have something to say but I do not want to block” (so more close to requesting changes) or it could be “I think I can approve it but I am not totally sure about” (so more close to an approval) or it could be something else where still a complete or second review is needed. So I am not sure if one of the two current options is correct or a third like only removing NOT_YET_REVIEWED would be better. But if one of the options feels correct for most users (especially those more active), I would say go for it.

3

I think ‘comment’ vs. ‘request changes’ is a personal or cultural difference. To me, it feels harsh to use ‘request changes,’ so I normally use ‘comment’ unless there is really something I feel would cause major damage if merged. But I know others don’t all view it the same way, which is fine.

After I have reviewed a PR with ‘comment’, I would consider it waiting on contributor in most cases. So I’m +1 to that change.

2 Likes
4

Btw, in PR label workflow it was suggested to drop the whole label workflow. Then there was a majority in favor of dropping it, but mentioned it was used during triage. That triage now no longer happens so I’d also consider the option to drop it altogether again, purely relying on GitHub workflows.

1 Like
5

I’ve seen comments used as approval as well so I dont think it makes sense as thats the opposite of request changes.

6

There seems to be a lot of variance in how people use PR reviews.

Another thing about labels, I remember before I had write access to Katello and Foreman I was frustrated because I couldn’t add or change labels either. So that also makes me less likely to look at them. For these reasons I’m +1 to dropping auto-labeling.

7

I’ll note that I quite often rely on the “has there been an update to this PR”. So what I often end up doing is:

  • Request changes
  • Label “Waiting on contributor” is applied
  • New version is pushed
  • Label “Waiting on contributor” is removed
  • Label “Needs re-review” is applied

Now I can see there has been a new version. GitHub doesn’t offer this natively.

Note you can also manually add the “Waiting on contributor” label and get the “Needs re-review” label applied. Very often I use the “Comment” feature to have a discussion and then the contributor needs to make changes I sometimes apply the labels manually.

For example, compare Pull requests · theforeman/puppet-foreman · GitHub and Pull requests · theforeman/puppet-foreman · GitHub. The latter is effectively a manageable TODO list while the former isn’t.

8

I do check daily foreman repo with these labels: label:UI,"Legacy JS" -label:"Waiting on contributor" so I am for keeping them.
I just wouldnt auto add “Waiting on contributor” after someone makes a comment

9

We could use “Comment” as a review, which implies removing “Not yet reviewed” and “Needs re-review” but not add “Waiting on contributor”.

1 Like
10

Disclaimer: this is just my personal opinion.

Thanks for starting this discussion. I agree that there are cultural differences and that’s why I think it’s important to openly talk about this. Having a shared understanding of “what to select when” helps everyone to collaborate more efficiently and evolve the open source project further.

I propose the following usage:

  • Comment: to add suggestions, ask general questions, give tips/hints on the change, propose follow-up changes. IMHO the default type of feedback that you give to open source contributions. IMHO we should encourage interaction and discussion on PRs.
  • Request changes: to block a PR if it would break currently supported features/workflows/automation. Example: Foreman/Katello cannot use Salt anymore to configure hosts. Not valid for things like “Package Actions implemented for Yum content, not yet for Deb content”. This should not be used for personal preference or out of scope requests.
  • Approve: to signal that this change is OK as is.

Regarding labels on GitHub: I have no strong opinion (yet).

Two good sources on how to contribute and review PRs which I see as the main ways to collaborate: Foreman :: Contribute and foreman-documentation/CONTRIBUTING.md at master · theforeman/foreman-documentation · GitHub

Slightly unrelated, but crucial nevertheless: IMHO we should be mindful when to request changes/block PRs; and we should actively help (new) contributors to achive their goals which normally ends with a merged PR.

4 Likes
11

Personally I see a big difference in “Request changes” and “Comment”. I take Request changes as something that must be addressed so the PR can be merged. The merge button is even blocked, if there’s at least one “Request changes” review. I take “Comment” more as a suggestion. With reading other’s interpretation, I think we need to come up with some good descriptions for each we will agree on.

I found labels generally problematic. Just yesterday, I saw a PR that had both “Needs re-review” and “Waiting on contributor”. Luckily the contributor has merge permissions and could have fix it, but relying on labels for the workflow is bad, as majority of contributors can’t change them. I don’t have a problem with labels for tagging purposes, e.g. the mentioned LegacyJS. It may be helpful to have more tags, such as “provisioning templates” etc.

Excellent points. While we operate in the current way for a long time, there are still many things in gray zone. I think we’d benefit from clarifying and documenting the expectations. I like the descriptions you put here a lot and would consider them as something we put to our contribution and maintainers guidelines.

I’d also say, we should set some expectations around commitment of a reviewer, who started with the review. Often times I see contributors waiting for re-review for too long. And/or we should think of some mechanism to unblock the contribution in case some reviewer who requested changes is busy and can’t get back to the PR. I think maintainers trust each other so that one maintainer can verify, if all requests from other maintainer were already satisfied. Today’s reality I believe is, we expect until every reviewer clears their requested changes review, we very rarely dismiss the stale requests. And that prolongs the merge time dramatically.

3 Likes
12

This was exactly the reason I brought up the discussion. If we have labels, they should be correct. The more I think about it, the more I’m leaning to what I wrote earlier:

Reading the whole threat I think there’s broad consensus that a comment review doesn’t imply it’s waiting on the contributor but it does mean that someone (re-)reviewed it.

I think it should become more common to dismiss other reviews. For example, when I write “This needs a rebase and passing tests” I want to clearly signal to the author that it’s blocked on some changes. However, other reviewers can easily verify that it was rebased and the tests are passing. In that case it’d be perfectly valid to dismiss my review.

13

I thought I’d write up a PR that implements this, so I started with the docs. The webhook we recieve is pull_request_review with the action type submitted (see prprocessor/prprocessor/__main__.py at a01156d75c355d6e67af3aff0f06ca226f1443ad · theforeman/prprocessor · GitHub):

There it mentions the state, but it doesn’t describe the various possible values. It does for dismissed.

Digging through actual responses we can see (shortened for readability):

{
  "action": "submitted",
  "review": {
    "id": 2215190649,
    "node_id": "PRR_kwDOCyKKK86ECSB5",
    "user": {},
    "body": null,
    "commit_id": "1a2206e7b2d40a5d08cc30b7d08603ada486b22f",
    "submitted_at": "2024-08-02T09:45:25Z",
    "state": "commented", 
    "html_url": "https://github.com/theforeman/foreman-documentation/pull/3079#pullrequestreview-2215190649",
    "pull_request_url": "https://api.github.com/repos/theforeman/foreman-documentation/pulls/3079",
    "author_association": "CONTRIBUTOR",
    "_links": {
      "html": {
        "href": "https://github.com/theforeman/foreman-documentation/pull/3079#pullrequestreview-2215190649"
      },
      "pull_request": {
        "href": "https://api.github.com/repos/theforeman/foreman-documentation/pulls/3079"
      }
    }
  }
}

Note the state is commented. Another observation: today we don’t look at the author_association at all. We could, but so far it hasn’t been needed.

I’ve opened Handle comment reviews as a review by ekohl · Pull Request #197 · theforeman/prprocessor · GitHub so please leave a review there if you agree or disagree.

2 Likes