-
Notifications
You must be signed in to change notification settings - Fork 67
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
f708c40
commit 98c10e6
Showing
1 changed file
with
101 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,10 @@ Refer to the [triage and peer review](/contributing/triage_resources/) for infor | |
|
|
||
| ## Maintainer Roles | ||
|
|
||
| Note that these roles are specifically for maintaining [supported modules](./supported_modules/). Unsupported modules in the `silverstripe` GitHub organisation are not necessarily maintained by these roles. If there is an unsupported module in the `silverstripe` GitHub organisation that you would like to maintain, please email <[email protected]>. | ||
|
|
||
| All maintainers must follow the [house rules](#house-rules). | ||
|
|
||
| ### Core Committers | ||
|
|
||
| The people looking after the Silverstripe Core modules. | ||
|
|
@@ -36,23 +40,117 @@ often working alongside Core Committers. | |
|
|
||
| CMS Squad members have write access to core repositories in order to work effectively with GitHub issues. They are expected to use those permissions with good judgement for merging pull requests. | ||
|
|
||
| ### Peer Reviewers | ||
|
|
||
| [info] | ||
| This is a new role, and is currently in a trial period. It is likely to change over time as we learn what works and what doesn't. | ||
| [/info] | ||
|
|
||
| This role is made up of community members who have demonstrated a willingness to improve Silverstripe CMS through their contributions to the open source supported modules. It empowers community members to have a more active role in ensuring pull requests get merged, and builds more momentum for community-created pull requests. | ||
|
|
||
| While anyone in the community can review a pull request, only reviews from maintainers have weight of authority - and only maintainers can meaningfully approve changes. The Peer Reviewer role ensures members of the community have that authority. | ||
|
|
||
| #### Responsibilities {#peer-reviewer-responsibilities} | ||
|
|
||
| * Review pull requests, ensuring documented processes and best practices are being followed | ||
| * Refer to [how to review pull requests](/contributing/triage_resources/#how-to-review-pull-requests) and [contributing code](/contributing/code/) | ||
| * Escalate pull requests to Core Committers if the contributor is rude/argumentative/hard to deal with | ||
| * If you feel confident doing so, politely request the contributor behave in accordance with our [code of conduct](./code_of_conduct), and close the pull request if they refuse to do so | ||
| * Escalate pull requests to Core Committers if the PR is very complex and you don't feel confident reviewing it | ||
| * If another reviewer feels confident, they can offer to review it instead | ||
| * After two maintainers have approved the pull request, merge it | ||
| * The person who merges the PR must also understand and approve the changes | ||
| * The person who contributed the PR must not be included as one of the two approving reviewers | ||
| * If you don't feel confident clicking merge, ask for a second opinion. Don’t approve the pull request if you wouldn’t feel confident merging it - instead, say where you got to with it (e.g. tested, worked well locally couldn’t find any problems but want another opinion) and get another reviewer to check it | ||
| * If no other reviewer feels confident about it, but it seems like it’s probably a good change in general, escalate to Core Committers | ||
|
|
||
| In addition to the above responsibilities, if a reviewer doesn’t review any pull requests in a 12 month period, a member of the Core Committers or the CMS Squad team leader will reach out and ask if they want to continue being a reviewer. If they don’t respond within a month (or they respond saying they don’t want to be a reviewer anymore), their permissions will be revoked and they will be removed from the team. | ||
|
|
||
| If the reviewer fails to contribute for 24 months in a row, they will be removed from the team until such a time as they can commit to performing the responsibilities of the role. | ||
|
|
||
| #### Onboarding new peer reviewers | ||
|
|
||
| Members of the community who have shown an interest in helping to maintain Silverstripe CMS, who have shown themselves to be trustworthy, who follow the code of conduct, and who have provided good contributions may be invited by a Core Committer or the CMS Squad team leader to join the peer review team. | ||
|
|
||
| Those contributions could be good quality bug reports, good quality pull requests, help refining issues and PRs, and unofficial pull request reviews. These contributions must demonstrate clear communication, adherence to our contribution guidelines, and good collaboration skills. | ||
|
|
||
| Anyone in the Core Committers or Peer Reviewers teams plus the CMS Squad team leader can put a name forward of someone they think would be a good fit. All core committers and reviewers and the CMS Squad team leader will have a (timeboxed) chance to object. Then if there’s no objections, the prospective member is invited to join the team. | ||
|
|
||
| ### Contribution Refiners | ||
|
|
||
| [info] | ||
| This is a new role, and is currently in a trial period. It is likely to change over time as we learn what works and what doesn't. | ||
| [/info] | ||
|
|
||
| Blah blah anyone can triage issues and pull requests (except for applying labels), somethingsomething having this role is a useful way for the maintainers to be aware of contributions people are making etc etc pathway to becoming a peer reviewer. | ||
|
|
||
| @TODO this is all directly copy-pasted from the proposal document right now - I need to rework it. | ||
|
|
||
| Having a role for refining issues and pull requests will help ensure those are in a good state by the time someone comes to work on the issue or review the pull request. It also gives community members a low-effort entry point to being involved outside of contributing code themselves. | ||
|
|
||
| By dealing with the obvious issues of a pull request before a full review is done, it will be easier for a reviewer to focus on the actual change being made rather than worrying about things like the commit message. This means that by the time a reviewer starts actually reviewing the pull request, it’s already in a good state and can be merged as soon as the code changes are approved. | ||
|
|
||
| Refiners will be enforcing a subset of the processes and best practices as a necessary part of the role, and will be exposed to more of them by seeing pull requests that they refined being reviewed. This makes the role an excellent way to start getting people familiar with our processes and guidelines. | ||
|
|
||
| As mentioned in the section about a new reviewer role, one way we can identify community members who might be suitable for or interested in the reviewer role is by having a role which requires few if any escalated permissions. Refiners who are particularly active might be good candidates for moving to the reviewer role - or they might only be interested in refining issues and pull requests, and remain in the refiner role. | ||
| Blah blah refine issue and pull request descriptions | ||
|
|
||
| @TODO Documentation includes process for how to identify, invite, and onboard people to the new roles | ||
|
|
||
| #### Responsibilities {#refiner-responsibilities} | ||
|
|
||
| Along with the specific responsibilities below, refiners could add appropriate labels to issues according to our [documented guidelines](/contributing/code/#labels) (aka triage - see ) | ||
|
|
||
| ##### Refine issues | ||
|
|
||
| * Perform triage tasks as documented in [how to triage](/contributing/triage_resources/#how-to-triage) | ||
| * Ensure bug reports contain valid reproduction steps and sufficient context to understand what the buggy behaviour is | ||
| * Ensure feature requests are within the scope (to be defined) of feature requests we are interested in, AND/OR | ||
| * Close feature requests if the person raising the issue indicates (either explicitly, or implicitly by not responding) that they aren’t going to raise a pull request implementing the feature (see the note about feature requests in our docs) | ||
| * Encourage people raising issues to also raise pull requests | ||
| * Close issues that aren’t in scope (e.g. spam, people seeking support, bug reports for non-supported versions, etc) | ||
| * Direct people opening such issues to the correct channels if there is one, e.g. for support | ||
| * Make sure the CMS Squad and Core Committers are aware of any critical or near-critical issues that need to be addressed in a timely manner | ||
| * If any issues seem to be reporting a security issue, immediately notify the CMS Squad and Core Committers | ||
|
|
||
| ##### Refine pull requests | ||
|
|
||
| * The following apply to pull requests that nobody has started reviewing yet. Once someone has started reviewing a pull request, the refiner should step back. | ||
| * Ensure pull requests have a clear description indicating the purpose of the change(s) | ||
| * Ensure pull requests link to an issue that the PR resolves | ||
| * If one doesn’t exist, either open one or ask the pull request creator to open one | ||
| * Ensure commit messages in pull requests are appropriate and meaningful | ||
| * Ensure pull requests target the correct branch | ||
| * If any pull requests seem to be fixing a security issue, immediately notify the CMS Squad and Core Committers | ||
|
|
||
| #### Onboarding new refiners | ||
|
|
||
| Maybe people can email <[email protected]> and ask to be added? Or we shoulder tap people who are particularly active in Slack, or who are contributing a lot of pull requests? And if someone’s already doing it despite not being in the group, we should definitely invite them. | ||
|
|
||
| ## Guidelines | ||
|
|
||
| With great power (write access) comes great responsibility. | ||
|
|
||
| First and foremost rule of a maintainer is to collaborate with other maintainers. Follow the house rules and remember that others also care about the modules they maintain. | ||
|
|
||
| ### House rules overview | ||
| ### House rules | ||
|
|
||
| * Be friendly, encouraging and constructive towards other community members | ||
| * Be familiar with our contribution guidelines, especially the [definition of public API](./public_api/) and how that ties into our branching and release strategies | ||
| * If you have any questions or doubts, always ask - there's a lot of process and domain knowledge, and it is important that you feel confident to fulfil your role | ||
| * Frequently review pull requests and new issues (in particular, respond quickly to @mentions) | ||
| * Treat issues according to our [issue guidelines](/contributing/issues_and_bugs/), and use the [triage resources](/contributing/triage_resources/) | ||
| * Don't commit directly to existing branches, raise pull requests instead | ||
| * Use forks to create feature branches for pull requests | ||
| * Only merge code you have tested and fully understand. If in doubt, ask for a second opinion. | ||
| * Follow the [Supported Modules Standard](https://www.silverstripe.org/software/addons/supported-modules-definition/) | ||
| * Ensure contributions have appropriate [test coverage](/developer_guides/testing/), are documented, and adhere to our [coding conventions](/getting_started/coding_conventions/) | ||
| * Keep the codebase "releasable" at all times (check our [release process](/contributing/release_process/)) | ||
| * Follow [Semantic Versioning](/contributing/code/#picking-the-right-version) by putting any changes into the correct branch | ||
| * If the contributor has trouble adding tests or documentation, you can raise your own pull requests that adds tests or documentation for the change and then merge their change | ||
| * Keep the codebase stable at all times (check our [release process](/contributing/release_process/)) | ||
| * If there are CI failures which are unrelated to the changes a given pull request, you can merge the pull request if all relevant tests are being run and passing | ||
| * Changes must be merged into the appropriate branches and follow semantic versioning (see [picking the right version](/contributing/code/#picking-the-right-version)) | ||
| * Be inclusive. Ensure a wide range of Silverstripe CMS developers can obtain an understanding of your code and docs, and you're not the only one who can maintain it. | ||
| * Avoid `git push --force`, and be careful with your git remotes (no accidental pushes) | ||
| * Assign yourself to the issue(s) you are reviewing. If you’re reviewing one pull request in that issue, you’re reviewing them all. | ||
| * Before merging a pull request, check the linked issue for related pull requests which should be reviewed and merged together. | ||
| * Don’t merge documentation for code changes until the code changes have been merged. | ||