-
Notifications
You must be signed in to change notification settings - Fork 585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Reviewer & Contributor guides for docs #704
Changes from all commits
fa20807
79122cb
355af78
d521120
d91c2da
311bc77
c8c3b76
a5cc700
54662b5
0ebceb0
affdc2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
Contributor's Guide for docs.mattermost.com | ||
=========================================== | ||
|
||
Thank you for your interest in contributing to Mattermost documentation. Your contributions benefit thousands of organizations around the world deploying Mattermost. | ||
|
||
To start: | ||
|
||
Create a Ticket | ||
--------------- | ||
|
||
1. **Select a Help Wanted ticket** - Please select a `HELP WANTED ticket <https://github.com/mattermost/docs/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22>`_ that you'd like to address with your contribution, or create a new ticket to discuss an improvement you'd like to make and wait for a Project Member to approve and add the `Help Wanted` label so the change will be accepted. Project Members have a "Member" label on their comments. | ||
|
||
If your update is small and an obvious improvement, you can skip this step and submit a PR directly. | ||
|
||
2. **Become an Approved Contributor** - Please complete the `contributor license agreement <https://www.mattermost.org/mattermost-contributor-agreement/>`_ (a.k.a. "CLA"). This is a standard form for most open source projects and it adds your the `approved contributor's list <https://docs.google.com/spreadsheets/d/1NTCeG-iL_VS9bFqtmHSfwETo5f-8MQ7oMDE5IUYJi_Y/pubhtml?gid=0&single=true>`_ for the Mattermost open source project. There are answers to frequently asked questions in the link above. | ||
|
||
3. **Create your Pull Request** - Please create a pull request for the documentation change you would like to make. Make sure to review and follow the `Style Guide XXX <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_. Please include a link to the Help Wanted ticket if you have one. | ||
|
||
4. **(Optional) Meet the Documentation Community** - You can join the `Mattermost Documentation channel <https://pre-release.mattermost.com/core/channels/documentation>`_ on the community server and introduce yourself to other contributors and project members. | ||
|
||
Once your ticket is submitted it goes into a review process: | ||
|
||
PR Review Process for Contributors | ||
---------------------------------- | ||
|
||
1. **New PR submitted** - Each PR created sends a notification to the `Documentation Channel <https://pre-release.mattermost.com/core/channels/documentation>`_ on the Mattermost contributors server, alerting reviewers. | ||
|
||
2. **Triage** - Project Member assigned to triage reviews doc repo within 1-2 working days and applies the `appropriate "Needs ... Approval" labels to indicate required approvals XXX <XXX>`_. Use **Saved Reply** template linked to this page to let submitter know what to expect. | ||
|
||
3. **Reviews** - Within 5-10 working days, for each `"Needs ... Approval" XXX <link to labels explaination>`_ label, reviewers on rotation either a) leave feedback or b) mark PR "Approved" and remove the appropriate label. The reviewer requiring changes adds "Awaiting Submitter Action" label if it is not yet applied. After submitter addresses feedback, the 5-10 working day clock restarts. | ||
|
||
4. **Merge** - When all "approval required" labels are removed, the last approver reviewing merges the PR. | ||
|
||
5. **Verification and Acknowledgement** - Member assigned to Verification reviews recently merged PRs and adds the "PR verified" label and thanks the contributor in GitHub issue with a mention. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
Mattermost Documentation | ||
======================== | ||
|
||
Hosted at docs.mattermost.com, Mattermost Documentation is jointly developed by our community and Mattermost, Inc. to accelerate the adoption of Mattermost software based on the following goals and principles: | ||
|
||
Goal | ||
---- | ||
|
||
Increase Mattermost usage through more numerous and larger deployments via timely, effective, easy-to-find documentation delivered by talented people from community and Mattermost, Inc. who are attracted, trained, engaged and celebrated through our processes. | ||
|
||
Principles | ||
---------- | ||
|
||
Situation Awareness | ||
|
||
- Everyone involved with the documentation process should be aware of relevant processes, criteria and timelines. It's okay for processes to take time, it's not okay to leave contributors in the dark. | ||
|
||
Shared Responsibility | ||
|
||
- People approving PRs have a shared responsibility for its success and accurancy. Reviewers may be asked to make corrections in future, if PR authors are not available. | ||
|
||
Appropriate Review | ||
|
||
- Reviews of PRs should be specific PR needs (e.g. grammar, testing verification, technical review, etc.) and be conducted by reviewers skilled in those review areas. | ||
|
||
Acknowledgement | ||
|
||
- Contributors and reviewers should be acknowledged for their short term and long term contributions. | ||
|
||
Trust but Verify | ||
|
||
- Trust everyone to have the right intentions, spot check to find blindspots. | ||
|
||
Process | ||
---------- | ||
|
||
The processes and materials supporting the Mattermost Documentation goals and principles include: | ||
|
||
- Contributor's Guide for docs.mattermost.com XXX | ||
- Reviewer's Guide for docs.mattermost.com XXX | ||
- Documentation Style Guide XXX | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
Reviewer's Guide for docs.mattermost.com | ||
======================================== | ||
|
||
Review of pull requests to Mattermost documentation passes through the following high-level steps: | ||
|
||
1. Project Members and Approved Contributors submit PRs to the docs.mattermost.com repository. | ||
2. Incoming PRs are assessed and assigned review requirements (e.g. technical PRs, PRs requiring Sphinx changes, etc.). | ||
3. Reviewers with appropriate qualifications either approve and merge the changes, or leave feedback for the submitter to begin the review cycle again. | ||
4. Once merged, PRs are verified by a Project Member and the submitter's work is acknowledged. | ||
5. Reviewers with consistent performance are acknowledged with an increase in `skill level <link to below>`_. Reviewers with repeated inconsistency may have their level decreased (though this is rare). | ||
|
||
The following sections lay out the details for this process: | ||
|
||
Pre-reading | ||
------------- | ||
|
||
- Please read `Contributor's Guide for docs.mattermost.com XXXXXX <XXXX>`_ for the process by which Approved Contributors submit PRs and engage with reviewers. | ||
|
||
Review Labels | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not really sure I agree with the labeling concept, it seems very confusing for people external to the team. But I guess you've considered that and still think it's important to do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also do we only need one review then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I'm a little confused by the "Post-Merge Editing Required" label, why would we merge something that still needs to be edited? In what cases would it be appropriate to use this? What would a minor correction be? Do we want to be merging docs with spelling errors and fixing them afterwards? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I don't think the labels system is perfect, I think reviewers have different areas of expertise and levels of skill and aren't interchangeable. Some reviewers may be more technical, some have stronger grammar, and we need a system that delivers a process as the sum of our strengths. I think we should review the system, implement it as a pilot and evaluate the pros and cons vs. other options. I'm fine changing Style Guide to another name, as well as changing Needs ... Approval labels, etc. The goal is "sum of our strengths" in my mind. Editing would be after a merge--or maybe they're able to edit in a staging environment of some kind. I think the role is traditional editor |
||
------------- | ||
|
||
The following labels are used to indicate the type of review required for each doc PR: | ||
|
||
- **Needs Style Guide Approval** - Review for `style guide requirements <https://docs.mattermost.com/process/sg_mattermost-doc-style.html>`_, and specifically: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to add "Needs accuracy approval" for checking functionality matches documentation |
||
|
||
- Verify all links work | ||
- Verify grammar is correct based on US English requirements | ||
- Verify RST renders properly in GitHub preview | ||
|
||
- **Needs Technical Approval** - Technical change requiring developer review, e.g. change in system requirements. | ||
|
||
- **Needs Testing Approval** - Change requires manual testing, e.g. significant install process change. | ||
|
||
- **Needs Sphinx Approval** - Significant structural changes to CSS or Sphinx features, requires HTML generation and verification. | ||
|
||
- **Needs Structural Approval** - Approval required on changes to TOC or organization. | ||
|
||
- **Needs Verification & Acknowledgement** - docs.mattermost.com needs to be verified and acknowledgement added to PR. | ||
|
||
- **Post-Merge Editing Required** - PR can be merged and someone will follow-up with edits for minor corrections. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change to pre-merge edit, and clarify use case--add a clear comment of what is not matching style guide |
||
|
||
- **Awaiting Submitter Action** - Merge should not happen until feedback is addressed. Note: This label should be applied for any work-in-progress PR, a separate label is not needed. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the labels might be too granular. I think if you create roles such as Reviewer, Technical Reviewer, and Editor, the people with those roles should be responsible for making sure that, for example, the changes are tested or the Sphinx output is verified. And then the labels would be of a different nature... "Review Required", "Technical Review Required", "Copy Edit Required", and the like. |
||
Levels | ||
------------- | ||
|
||
The anticipated consistency of reviews by Reviewers is noted using the following levels: | ||
|
||
- **Level 1** - Self-identified expertise in a review criteria, and willing to help. | ||
- **Level 2** - 80%+ track record of good reviews, or subjective promotion to this level by Level 3 or above. | ||
- **Level 3** - 90%+ track record of correct reviews, or subjective promotion to this level by Level 4 or above. | ||
- **Level 4** - 95%+ track record of correct reviews or maintainer of doc repo. | ||
|
||
Extended periods of consistent performance are recognized by level increases. Repeated inconsistency reduces an reviwers level. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A few questions about the levels:
|
||
Roles | ||
------------- | ||
|
||
The following lists roles involved in the PR review process: | ||
|
||
- **Approved Contributor** - Anyone who's completed the CLA. This is the minimum role to submit a PR for review. | ||
- **Project Member** - Approved Contributor granted ability to apply labels. Often a staff member from Mattermost, Inc. | ||
- **Project Member Reviewer on rotation** - Member assigned to review PRs in certain approval categories. If there is more than one person who can do a review, a rotation system is put in place so it's unambiguous who is responsible for a given review. | ||
- **Project Member Verifier on rotation** - Member assigned to verify and acknowledge merged doc PRs. If there is more than one person who can do a review, a rotation system is put in place so it's unambiguous who is responsible for a given review. | ||
- **Project Member Editor on rotation** - Member assigned to edit PRs with "Post-merge edit required". If there is more than one person who can do a review, a rotation system is put in place so it's unambiguous who is responsible for a given review. | ||
|
||
Approved Contributors who are not Members can participate in the PR review process, and through consistent performance may be promoted to Members: | ||
|
||
- **Contributor Reviewer** - Approved Contributor who volunteers to review PRs prior to Member review. | ||
- **Contributor Verifier** - Approved Contributor who volunteers to verify PRs after merge. | ||
- **Contributor Editor** - Approved Contributor who submits PRs editing merged changes with "Post-merge edit required" label. | ||
|
||
Helping identify any items missed by Members is highly welcome and encouraged. | ||
|
||
PR Review Process for Reviewers | ||
--------------------------------------- | ||
|
||
The following process is used determine how reviewers on rotation leave feedback on doc PRs submitted: | ||
|
||
1. **Reviewers** - Reviewers on rotation should `browse open doc PRs <https://github.com/mattermost/docs/pulls>`_ at least once daily while on-call, and leave feedback, approve or merge any open PRs. | ||
2. **Verifiers** - Verifier on rotation should `browse query of PRs needing verification XXX<linked_needed>`_ at least once daily while on-call and verify and acknowledge PRs as appropiate. If there's an issue, Verifier should message Reviewer to submit a PR to correct issues. | ||
3. **Editors** - Editor on rotation should `browse query of PRs needing editing XXX<XXX>`_ daily and submit a PR with appropriate edits to pages needing edits to meet the Style Guide. This can be done on production shortly after a merge, and ideally it will be done on staging after a staging server is made available. | ||
|
||
Rotation periods should be assigned on alternate weeks among people responsible for reviews and post-merge editing. | ||
|
||
Reviewers | ||
------------- | ||
|
||
The following table summarizes Doc Repo approvers and levels: | ||
|
||
============================ == == == == == == == == == == == == == == | ||
Reviewer and Levels LB ES JB LI IT YC AM JW CS CH EN HH GG SJ | ||
============================ == == == == == == == == == == == == == == | ||
Style Guide Approval 3 3 3 3 3 3 3 3 3 3 3 3 3 3 | ||
Technical Approval 0 0 0 0 0 0 0 3 3 3 3 3 3 3 | ||
Testing Approval 0 0 0 0 0 0 0 3 3 3 3 3 3 3 | ||
Sphinx Approval 0 3 0 0 0 0 3 0 0 0 0 0 0 3 | ||
Structural Approval 3 0 0 0 3 0 0 0 0 0 0 0 0 3 | ||
Post-Merge Edit 3 3 3 3 3 3 3 3 3 3 3 3 3 3 | ||
Verify & Acknowledge 3 3 3 3 3 3 3 3 3 3 3 3 3 3 | ||
============================ == == == == == == == == == == == == == == | ||
|
||
- LB - lfbrock | ||
- ES - esethna | ||
- JB - jasonblais | ||
- LI - lindy65 | ||
- IT - it33 | ||
- YC - yangchen1 | ||
- AM - assadmahmoud | ||
- JW - jwilander | ||
- CH - coreyhulen | ||
- CS - crspeller | ||
- EN - enahum | ||
- GG - grundleborg | ||
- HH - hmhealey | ||
- SJ - shieldsjared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in practice, "5-10 working days" will end up being "10 working days", which seems kind of long.