Add a maintainers guide#2745
Conversation
GaryJones
force-pushed
the
GaryJones/wpcs-maintainer-guidelines
branch
from
c7fc3a6 to
23d2fb1
Compare
|
I'm not sure we need anything about AI in the maintainers guide. From my perspective, the contributors guide is enough. But since this PR does include an AI section, I think it's blocked by #2735. |
Document the conventions maintainers follow, so the practices we have relied on tacitly are written down and discoverable. Covers the "WordPressCS follows, it does not lead" principle for judging feature requests, moderation rules (never delete, hide with reason), PR review and merge expectations, branch handling, Dependabot triage, the release outline, and cross-repo and upstream relationships.
GaryJones
force-pushed
the
GaryJones/wpcs-maintainer-guidelines
branch
from
23d2fb1 to
350846c
Compare
|
Feedback addressed. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Hi @GaryJones, thank you for setting up this PR! Looking good.
I've done a very critical read and left lots of questions inline. Not all of these necessarily need to lead to changes, but they did all feel like things we should at least consider/discuss.
There was a problem hiding this comment.
Why is this file placed in the project root ? Considering that the CONTRIBUTING file and the release-checklist file are both in the .github directory, it would seem more logical to me to move this file to the .github directory too.
If there is a good reason to leave the file in the project root, then at the very least, the file should be added to the .gitattributes export-ignore list.
| - **Review feature requests through this lens.** When triaging a feature request, the first question is rarely "is this a good idea?" but "is this already in the handbook, and if not, has it already been raised on Make Core?" | ||
|
|
||
| There is a tension here. The handbook says the standards are meant for the whole WordPress community and not only Core, which in theory makes Core just one consumer among tens of thousands of projects. That argument has never worked in practice. A rule that Core itself does not follow looks odd, so proposals get judged against Core anyway. Core does still keep some exceptions of its own, and not only for bundled third-party packages, but do not count on the "just one consumer" point to unblock a change. | ||
|
|
There was a problem hiding this comment.
This section is confusing as it completely ignores that we also have the WordPress-Extra standard which:
- Is not covered by the "handbook first" rule.
- Contains some rules which are not relevant for WP Core.
- Contains some rules which still need to be proposed for WP Core, but are already being encouraged for the wider community.
Maybe this section should be preceded by a section which maps the rulesets we maintain to the handbooks ?
I.e. Core = PHP Coding Standards, Docs = Documentation standards, Extra = not in handbook, best practices and other helpful sniffs.
| ## Code of conduct and moderation | ||
|
|
||
| - Follow and uphold the [WordPress Code of Conduct](https://github.com/WordPress/.github/blob/trunk/CODE_OF_CONDUCT.md). | ||
| - **Never delete a comment, issue or pull request.** This is a WordPress-organization-wide rule. If a comment needs to be removed from view, hide it with an appropriate reason rather than deleting it, so the record is preserved. |
There was a problem hiding this comment.
Should this get an exception for blatant spam and/or malicious content ? This is mostly handled by GitHub itself and they will make those type of issues disappear.
It has been exceedingly rare for those issues to crop up in the WPCS repo, but it might be good to formulate a guideline for this type of situation (or to verify with others in the WP organisation if there already exists a rule for this).
We may also want to have a think about how to deal with zero-day security issues reported in public instead of via the security advisories portal.
| As a rule of thumb, never merge your own PR. There are two exceptions: | ||
|
|
||
| 1. **Release PRs**, which still need approvals but are merged on a planned date by whoever is running the release. | ||
| 2. **PRs that change a workflow.** Making the workflow change early would block other PRs, so the change should only be made *after* the PR has the required approvals and *just before* it is merged. |
There was a problem hiding this comment.
This needs to be rephrased. The workflow update should be in the PR, it's the branch protection rule update which needs to be made just before the PR is merged.
There was a problem hiding this comment.
On that note... should this guide include some guideline about adding/maintaining the branch protection rules ? And adding new GHA jobs to those rules ?
And why we still use the "old" type of rules (Settings -> Branches) instead of the new type of rules (Settings -> Rules -> Rulesets) ? (yes, this is deliberate)
| ### How many approvals a PR needs | ||
|
|
||
| - **Trivial PRs** — for example Dependabot PRs where CI passes and no further changes are needed — need approval from **one** maintainer and can then be merged straight away. | ||
| - **All other PRs** need approval from **two** maintainers, with the **second** maintainer doing the merge. The merging maintainer is also responsible for deciding whether the PR needs a squash-merge or not. |
There was a problem hiding this comment.
Maybe we need a third category: "Policy PRs" (like this one) which need approval from all active maintainers ?
| ## Working with branches | ||
|
|
||
| - The branching model is documented in [CONTRIBUTING](.github/CONTRIBUTING.md#branches): ongoing development happens in `develop` and is merged to `main` once considered stable; feature branches are prefixed with `feature/`. | ||
| - **If you have push access, push PR branches to the main repository, not to your fork.** This lets other maintainers check out your branch locally without adding an extra remote. Work-in-progress branches can, of course, still live on your own fork. |
There was a problem hiding this comment.
| - **If you have push access, push PR branches to the main repository, not to your fork.** This lets other maintainers check out your branch locally without adding an extra remote. Work-in-progress branches can, of course, still live on your own fork. | |
| - **If you have push access, push PR branches to this repository, not to your fork.** This lets other maintainers check out your branch locally without adding an extra remote. Work-in-progress branches can, of course, still live on your own fork. |
What with main being a common branch name, this may prevent some confusion.
|
|
||
| The full release procedure is documented in [`.github/release-checklist.md`](.github/release-checklist.md), which is the template used for release PRs from `develop` to `main`. In outline: | ||
|
|
||
| - Releases go out as a PR from `develop` to `main`; the PR carries approvals but is merged on the planned release date by whoever is running the release. |
There was a problem hiding this comment.
| - Releases go out as a PR from `develop` to `main`; the PR carries approvals but is merged on the planned release date by whoever is running the release. | |
| - Releases go out as a PR from `develop` to `main`; the PR carries approvals but is merged on the planned release date by whomever is running the release. |
| - Before release, dependency version ranges are reviewed, upstream releases (PHPCS, PHPCSUtils, PHPCSExtra) are checked for anything worth adopting, list-based sniffs and WP version properties are updated, every merged PR is confirmed to have a milestone, and the changelog is written. | ||
| - After merging, tag and create a release against `main` (note that GitHub defaults the target to `develop`), close the milestone and open the next one, then fast-forward `develop` to equal `main`. | ||
| - Afterwards, open a Trac ticket for WordPress Core, and publicize the release: a Make WordPress post for major releases, a note in the `#core` channel on the WordPress.org Slack (the `#core-coding-standard` channel receives an automated notification), and the optional marketing follow-ups listed in the checklist. |
There was a problem hiding this comment.
This feels like it's duplicating the release checklist with the risk of getting out of sync. Isn't the pointer to the release checklist enough ?
|
|
||
| ## Related repositories | ||
|
|
||
| Maintainers of the WordPressCS repository should also always be given commit/maintain access to the [`wpcs-docs`](https://github.com/WordPress/wpcs-docs) repository. |
There was a problem hiding this comment.
Should we mention something about how and when we promote people to maintainer ?
Also, might be good to mention here that the above access should be arranged by adding someone to the https://github.com/orgs/WordPress/teams/wpcs team once the decision has been made that they should be promoted to maintainer.
|
|
||
| Maintainers of the WordPressCS repository should also always be given commit/maintain access to the [`wpcs-docs`](https://github.com/WordPress/wpcs-docs) repository. | ||
|
|
||
| ## Upstream dependencies |
There was a problem hiding this comment.
IMO this section feels redundant and too much as a duplicate of info contained in CONTRIBUTING.
What I can imagine would have value would be something about how we decide whether or not to add a new external dependency ? I.e. what criteria do we use ?
4 participants
Summary
The way we run this repository has, until now, lived mostly in maintainers' heads and in scattered Slack threads: how many approvals a PR needs, who merges, when not to merge your own work, how we treat Dependabot bumps, and the principle that WordPressCS follows the WordPress coding standards rather than setting them. New and existing maintainers had no single place to read any of this, which made onboarding slower and decisions less consistent.
This adds a
MAINTAINERS.mdat the repository root that writes those conventions down. It opens with the most important one for triage — "WordPressCS follows, it does not lead" — explaining that behavioural changes need to land in the handbook first, and why the "Core is only one consumer" argument has historically not unblocked changes. It then covers moderation (never delete, hide with a reason), PR review and merge expectations (trivial vs. standard approvals, the second approver merging, when to squash-merge, re-review after significant changes, label and milestone checks), branch handling, Dependabot triage, an outline of the release process pointing at the existing checklist, and the cross-repo and upstream relationships.Funding is intentionally omitted, consistent with #2734 removing it as a vestige of WordPressCS being a separate project. There is no maintainer roster in this document, by design.