DOC Update maintainer guidelines #383
Merged
Conversation
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
GuySartorelli
commented
Oct 18, 2023
Comment on lines
-67
to
-74
| ### Triage | ||
|
|
||
| Triage of issues and pull request is an important activity: Reviewing issues, adding labels, | ||
| closing stale issues, etc. This does not require write access to the repository as a "Contributing Committer". | ||
| This is a great way for active community members to help out, and start a path towards becoming a Core Committer. | ||
|
|
||
| Triage roles may be granted by any Core Committer, | ||
| who should give other Core Committers an opportunity to weigh in on the decision. |
There was a problem hiding this comment.
We don't have a standalone triage role at the moment. We might again in the future - if that happens, we can re-document it.
GuySartorelli
commented
Oct 18, 2023
Comment on lines
-58
to
-64
| * Complex or impactful changes need to be reviewed and approved by one or more Core Committers. This includes any additions, removals or changes to commonly used APIs. If that's not possible in the team, ping `@silverstripe/core-team` to get other Core Committers involved. | ||
| * For these complex or impactful changes, Core Committers should be given 1-2 working days to review. Ideally at this point, the API has already been agreed on through issue comments outlining the planned work (see [RFC Process](request_for_comment]). | ||
| * More straightforward changes (e.g. documentation, styling) or areas which require quite specialised expertise (e.g. React) that's less available through most Core Committers can be approved or merged by team members who aren't Core Committers. | ||
| * Self-merges should be avoided, but are preferable to having work go stale or forcing other team members to waste time by context switching into a complex review (e.g. because the original reviewer went on leave). Any self-merge should be accompanied by a comment why this couldn't be handled in another way, and a (preferably written) approval from another team member. | ||
|
|
||
| This role may be granted by any Core Committer, | ||
| who should give other Core Committers an opportunity to weigh in on the decision. |
There was a problem hiding this comment.
CMS Squad work doesn't tend to get reviewed by core committers (except in-so-far as there are core committers in the CMS Squad), and enforcing that requirement would introduce a massive bottleneck to getting any changes in.
GuySartorelli
commented
Oct 18, 2023
Comment on lines
-22
to
-39
| ## What is Silverstripe Core | ||
|
|
||
| Silverstripe CMS is powered by a system of components in the form of Composer packages. These packages will be categorised as either a _module_ or a _recipe_. | ||
|
|
||
| The "core" of Silverstripe refers to the module packages owned by the "silverstripe" Packagist vendor that fall under one of the following recipes: | ||
|
|
||
| * [silverstripe/recipe-core](https://github.com/silverstripe/recipe-cms) | ||
| * [silverstripe/recipe-cms](https://github.com/silverstripe/recipe-cms) | ||
| * [silverstripe/installer](https://github.com/silverstripe/silverstripe-installer) | ||
|
|
||
| ## What are Supported Modules | ||
|
|
||
| In addition to Silverstripe Core, there are many [Supported Modules](https://www.silverstripe.org/software/addons/supported-modules-definition/) | ||
| which have the backing of Silverstripe Ltd. While it's a good idea to apply the rules outlined in this document, | ||
| work on these modules is guided by the | ||
| [Supported Modules Standard](https://www.silverstripe.org/software/addons/supported-modules-definition/). | ||
| Commit access in Supported Modules is handled by agreement of the repository maintainers, | ||
| or any additional guidelines outlined via `README` and `CONTRIBUTING` files. |
There was a problem hiding this comment.
This is all documented in the dedicated supported modules page now
GuySartorelli
commented
Oct 18, 2023
GuySartorelli
commented
Oct 18, 2023
| * Treat issues according to our [issue guidelines](/contributing/issues_and_bugs/), and use the [triage resources](/contributing/triage_resources/) | ||
| * Don't commit directly to a release branch, raise pull requests instead | ||
| * 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/) |
There was a problem hiding this comment.
Follow the Supported Modules Standard
We don't really follow that, most likely - but as long as that page exists, I don't think we should stop referring to it. We should instead holistically reconsider how we approach supported modules.
emteknetnz
requested changes
Oct 24, 2023
| * Be friendly, encouraging and constructive towards other community members | ||
| * 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 a release branch, raise pull requests instead |
There was a problem hiding this comment.
Suggested change
| * Don't commit directly to a release branch, raise pull requests instead | |
| * Don't commit directly to existing branches, raise pull requests instead |
There was a problem hiding this comment.
Done - and moved the "Use forks to create feature branches for pull requests" guidelines to be directly following this one
| * 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 | ||
| * Public API changes and non-trivial features should not be merged into release branches. |
There was a problem hiding this comment.
Suggested change
| * Public API changes and non-trivial features should not be merged into release branches. | |
| * Public API changes and non-trivial features should merged into the branch for the next minor release, not patch release branches |
There was a problem hiding this comment.
That's not the intent - changing to specify breaking API changes as per #383 (comment)
There was a problem hiding this comment.
Actually I've just removed this point altogether, since it's covered in Follow [Semantic Versioning](/contributing/code/#picking-the-right-version) by putting any changes into the correct branch above that.
GuySartorelli
force-pushed
the
pulls/4.13/maintainer-guidelines
branch
from
b6e77ca to
ca972af
Compare
emteknetnz
approved these changes
Oct 24, 2023
- don't use brand icon - update language to reflect current state of affairs - update links (looks like this used to be in "contributing" and the links didn't get updated when it moved) - Remove unrelated information that already exists elsewhere - Swap out outdated "contributing committers" reference for the CMS Squad (we're publicly using that terminology already) - update "house rules" to reflect the things we actually do now
GuySartorelli
force-pushed
the
pulls/4.13/maintainer-guidelines
branch
from
ca972af to
26497c1
Compare
|
Rebased and resolved |
emteknetnz
approved these changes
Oct 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue