andygrove opened a new pull request, #6479: URL: https://github.com/apache/datafusion-comet/pull/6479
## Which issue does this PR close? No issue. This is a repository settings change, and the rationale is below. ## Rationale for this change Nothing currently checks that review comments were dealt with before a PR merges. A single approval satisfies branch protection, so a PR can be merged or queued while review comments on it are still open. Of the 487 PRs merged to `main` in August and September, 176 (36%) still had at least one unresolved review thread when they were merged or added to the merge queue. Most of those threads had been answered but were never marked resolved. Some had no reply and no follow-up commit, though. A few review comments were posted after the PR was already in the merge queue, and the PR merged without them being addressed. GitHub's "Require conversation resolution before merging" setting closes this gap. A PR cannot merge while any review thread on it is unresolved, whoever or whatever does the merging. ASF Infra exposes the setting through `.asf.yaml` ([docs](https://github.com/apache/infrastructure-asfyaml/blob/main/README.md#branchpro)), and Airflow, Pulsar and Ignite 3 already enable it. ## What changes are included in this PR? - `.asf.yaml`: set `required_conversation_resolution: true` for `main`. Release branches are unchanged. - `development.md`: a short paragraph in "Submitting a Pull Request". Authors resolve each conversation once it is addressed. Reviewers post feedback that must be fixed before merging as inline comments or as a **Request changes** review. What to expect: - Only inline review comments open a conversation. Top-level PR comments and review summaries do not. Feedback that has to block a merge should go inline or in a committer's "Request changes" review, which [already blocks merging](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews) until the same committer approves. - The PR author and committers can resolve conversations. - Review threads opened by bots, such as code scanning alerts, also have to be resolved. - GitHub does not document whether a PR that is already in the merge queue gets removed when someone opens a new conversation on it. ## How are these changes tested? The setting takes effect when this merges to `main`. Before opening the PR I checked four things locally: - `.asf.yaml` still parses. - The new key sits under `main` only, and the required reviews and `Required Checks` context are unchanged. - `dev/ci/check-ci-config.py` passes. It reads the `main` contexts from this file. - prettier passes on the doc change. After merging, the merge box of a PR with an open review thread should show merging as blocked. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
