Skip to content

Conversation

@dleach02
Copy link
Member

@dleach02 dleach02 commented Jun 4, 2024

Clarification of reviewer expectations and role. Emphasis on clarifying when a PR can be closed.

Highlighting reviewers and assignee objective to guide PR to a mergeable state, if possible.

@dleach02 dleach02 added the Process Tracked by the process WG label Jun 4, 2024
@zephyrbot zephyrbot requested a review from kartben June 4, 2024 03:05
@dleach02 dleach02 force-pushed the pr_management branch 2 times, most recently from 425f6e7 to 3a27e3a Compare June 4, 2024 18:22
@henrikbrixandersen henrikbrixandersen self-requested a review June 5, 2024 14:17
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

This sounds reasonable and hopefully matches our already existing practice.

@keith-zephyr
Copy link
Contributor

@dleach02 - there was an issue where a reviewer closed the PR of a new contributor. This PR highlights that reviewers shouldn't do this.
Also wanted to highlight that top priority of the assignee is to drive PR to a mergeable state.
@kartben - should the incident been handled with the code of conduct path?
@dleach02 - new contributor added new code along with a reformat of the full file using clang-format and reviewer closed the PR in objection.
@jfischer-no - responsible for closing the PR. @dleach02 - this wasn't the right path.
#73360 is the offending PR.
@fabiobaltieri - blocking a PR is okay, but closing isn't the right approach
@nashif - Gerrit has a feature of -2 vote.
@dleach02 - wants to highlight that escalation process and stalebot are the normal mechanism for closing a PR.
@kartben - having a PR closed is a pretty negative signal to the PR author
@nashif - no objections to the proposed language. But agrees that we have an issue of too many open PRs.
@nashif - we need a mechanism to address PRs that statebot tags, but author reopens it. But this isn't part of the review process. Closing a PR to express disapproval isn't the right approach.
@jfischer-no Did close another PR due to code violation (copied from another project without attribution).
@dleach02 - suggest to still use the escalation process

nashif
nashif previously approved these changes Jun 5, 2024
simhein
simhein previously approved these changes Jun 6, 2024
kartben
kartben previously approved these changes Jun 6, 2024
Clarification of reviewer expectations and role. Emphasis
on clarifying when a PR can be closed.

Highlighting reviewers and assignee objective to guide PR
to a mergeable state, if possible.

Signed-off-by: David Leach <[email protected]>
@nashif nashif merged commit 6f7fd7a into zephyrproject-rtos:main Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Documentation Process Tracked by the process WG

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants