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 docs on when reviews should be cleared #1556

Merged
merged 3 commits into from
Mar 5, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Feb 18, 2021

Resolves #1555

@MrAlias MrAlias added Skip Changelog PRs that do not require a CHANGELOG.md entry documentation Provides helpful information labels Feb 18, 2021
Copy link
Member

@punya punya left a comment

Choose a reason for hiding this comment

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

I learned today that PR authors who aren't maintainers don't have the ability to dismiss reviews 😞 At most, they can re-request review (but any existing approvals still stand).

@Aneurysm9
Copy link
Member

I learned today that PR authors who aren't maintainers don't have the ability to dismiss reviews 😞 At most, they can re-request review (but any existing approvals still stand).

Hmmm, does that mean we need a different approach? Do approvers have the ability to dismiss reviews or is it only maintainers?

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 1, 2021

Hmmm, does that mean we need a different approach? Do approvers have the ability to dismiss reviews or is it only maintainers?

It looks like we can explicitly set who can dismiss reviews for the repo:

20210301_091138

Should we add OpenTelemetry members to this list? Or restrict it to go-approvers?

Also, I want to verify the statement that I made in the SIG a while ago: that the branch protection rule "Dismiss stale pull request approvals when new commits are pushed" will clear merges from the main branch. I remember this being the case when I originally tried it out, but the description of that setting ("New reviewable commits pushed to a matching branch will dismiss pull request review approvals.") makes me question this might have changed.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 4, 2021

I've updated to allowing the go-approvers to dismiss reviews. I could not figure out how to have the interface allow all members of the org to dismiss reviews (it would not auto-complete a selection).

20210304_114609

@MrAlias MrAlias merged commit 875a258 into open-telemetry:main Mar 5, 2021
@MrAlias MrAlias deleted the review-clear branch March 5, 2021 22:26
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Provides helpful information Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add documentation on when PR reviews should be cleared
5 participants