Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add community-reviewed label is need to merge #2851

Merged
merged 13 commits into from
Jun 24, 2022

Conversation

tkoyama010
Copy link
Member

@tkoyama010 tkoyama010 commented Jun 24, 2022

Overview

The PyVista community has grown and also the developer community has grown. Nowadays, pull requests are reviewed from multiple locations around the world. Every time zone in the world should have an equal opportunity to review PyVista's pull request. In other words, reviewers should be granted their chance to review for at least 1 day. However, maintainers, myself included, tend to be aggressive in merging (because we want to improve PyVista quickly!). It is also painful to open an uncommented PullRequest after a day pass even though there is an enable auto-merge.

So I built a system to solve this problem with GitHub Action. This system is divided into two parts.

  1. Community Review: Here the label community-reviewed is added one day after the last action in the PullRequest. If someone takes some action, it will wait another day. If someone comments, the community-reviewed label is removed and a do-not-merge label is added. This label prevents merging, so please remove it when the discussion is over. You can also add it when you feel a long time discussion is needed.

  2. Pull Request Labels: Here we check the labels required for the merge, blocking the merge if they do not meet the criteria. The label community-reviewed, which is given if 1. is passed, is a required label. do-not-merge is a label that must not exist to merge. We also need a required label for release notes. This solves the problem of Fix GitHub Action's expression #2453 .

Please review as there may be use cases that I am not anticipating. Reviews are always important!!!

relate to #2245

resolves #2774

Details

@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jun 24, 2022
@adeak
Copy link
Member

adeak commented Jun 24, 2022

I won't be able to review this PR, but as someone who lives on the other side of the planet (well, active in time zones on the other side of the planet), this is much appreciated :) Thanks for all the work you put into this.

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #2851 (8224cfd) into main (1bd5689) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2851   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files          76       76           
  Lines       16399    16399           
=======================================
  Hits        15420    15420           
  Misses        979      979           

@tkoyama010
Copy link
Member Author

tkoyama010 commented Jun 24, 2022

@adeak Thanks. This PR was largely inspired by your "Improving review habit" thread.

@tkoyama010 tkoyama010 marked this pull request as ready for review June 24, 2022 11:44
@tkoyama010 tkoyama010 enabled auto-merge (squash) June 24, 2022 11:44
@tkoyama010
Copy link
Member Author

tkoyama010 commented Jun 24, 2022

As a matter of course, the Pull Request result is always red before community-reviewd label is added. I need to change my habits as I tend not to review until CI is green.

@tkoyama010 tkoyama010 disabled auto-merge June 24, 2022 12:01
@tkoyama010 tkoyama010 enabled auto-merge (squash) June 24, 2022 12:01
@tkoyama010 tkoyama010 changed the title Add community-review label is need to merge Add community-reviewed label is need to merge Jun 24, 2022
@tkoyama010 tkoyama010 disabled auto-merge June 24, 2022 12:09
@tkoyama010 tkoyama010 enabled auto-merge (squash) June 24, 2022 12:09
CONTRIBUTING.rst Outdated Show resolved Hide resolved
@tkoyama010 tkoyama010 disabled auto-merge June 24, 2022 12:09
@tkoyama010 tkoyama010 enabled auto-merge (squash) June 24, 2022 12:10
CONTRIBUTING.rst Outdated Show resolved Hide resolved
@tkoyama010 tkoyama010 disabled auto-merge June 24, 2022 13:24
@tkoyama010 tkoyama010 enabled auto-merge (squash) June 24, 2022 13:24
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I'm going to approve and see if automerge works after 24 hr.

@tkoyama010 tkoyama010 merged commit 5c36c18 into main Jun 24, 2022
@tkoyama010 tkoyama010 deleted the maint/github-action-required-labels branch June 24, 2022 18:40
@akaszynski
Copy link
Member

Whelp. Forgot to add the label as a merge requirement.

@akaszynski
Copy link
Member

"triage" added as a merge requirement.

@tkoyama010
Copy link
Member Author

@akaszynski Thanks.

tkoyama010 added a commit that referenced this pull request Jun 25, 2022
tkoyama010 added a commit that referenced this pull request Jun 25, 2022
* Revert "Add community-reviewed label is need to merge (#2851)"

This reverts commit 5c36c18.

* Apply suggestions from code review

* Apply suggestions from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable auto merge after 24 hours without any comments
3 participants