sig-docs-community/rfcs/rfc-docs-review-permissions.md

RFC: Require a docs reviewer and a technical-reviewer for PRs to o3de.org

Summary

This RFC proposes that submissions to the o3de/o3de.org repo, in the /content/docs and /content/static/images directories, require at least one docs reviewer and at least one technical reviewer to review and approve the PR before it can be merged.

Informally, some of the o3de.org community already practice this, however, the o3de.org repo's configuration settings don't clearly reflect this behavior. The current PR process for o3de.org repo looks like this:

Two valid approvals are required before a PR that's against main or development branches can be merged. They can be from any of the teams that have write role or above. This allows PRs that have received two approvals from only docs reviewers or only technical reviewers to be merged.

This RFC proposes a solution to better enforce the requirement for one docs reviewer and one technical reviewer.

What is the motivation for this suggestion?

Why is this important?

Requiring a docs reviewer and a technical reviewer can ensure that incoming PRs for O3DE Documentation is reviewed by people with relevant expertise. Furthermore, limiting the reviewers to one docs reviewer and one technical reviewer at minimum can ensure efficient use of resources (reviewers). For more information on these roles, refer to sig-docs-community's Nomination Guidelines.

What are the use cases for this suggestion?

Any PR in the o3de.org repo that amends the /content/docs/ or /content/static/images directory by updating existing docs or creating new docs and supplemental media, such as images and videos. For example, a PR that adds documentation about a feature.

What should the outcome be if this suggestion is implemented?

For a given PR, as described in the previous use case, 2 approvals at minimum are required before the PR can be merged: one from a docs reviewer and one from technical reviewer. A docs reviewer has the expertise to ensure the PR is written well, and a technical reviewer has the expertise to ensure the PR is technically accurate.

Suggestion design description

A CODEOWNERS file

The o3de.org:main and o3de.org:development branches shall contain a CODEOWNERS file that dictates the docs-reviewers and docs-technical-reviewers teams as owners of the appropriate directories in the o3de.org repo. This implementation is described and handled by the corresponding, accepted RFC, https://github.com/o3de/sig-docs-community/issues/61, and will help support the implementation of this proposed RFC.

Review and approval permissions

The review and approval permissions of the o3de.org:main and o3de.org:development branches shall be amended to require two approvals: one from a docs reviewer and one from a technical reviewer. Docs reviewers and technical reviewers, can be derived from the CODEOWNERS file. More information is needed on how to set up merge permissions to require one docs reviewer and one technical reviewer. However, the following describes one possible way:

In GitHub's web interface, in o3de.org repo, you can amend the branch protection rules of the main and development branches by enabling Require review from Code Owners. When enabled, any PRs to these branches will require an approved review from the designated owners of the changed files. However, the CODEOWNERS file has a limitation: If multiple code owners are listed, the required approvals can be from any of those owners. For example, if the code owners are docs-reviewers and docs-technical-reviewers, then a PR that recieves at least two approvals from docs reviewers and none from technical reviewers, or vice versa, will be valid to merge. This is not the desired functionality and this RFC proposes to mitigate that. Further research and a proof-of-concept to work around this limitation is needed.

A way to work around this limitation can be to define merge configurations through a GitHub Action or App. One possible solution that's worth exploring is Mergeable, a GitHub App that offers configurations to automate GitHub workflows. Specifically, it's ability to configure approvals can help implement this RFC. Mergeable's approval configuration offers the setting, requested_reviewers, which requires all of the requested reviewer's approvals in order for a PR to be merged. In the case of this RFC, a Mergeable configuration can specify that the required reviewers be the designated owners as defined in the CODEOWNERs file and that all the requested reviewers approve the PR.

Technical reviewers

This RFC requires consideration for who technical reviewers and the process for becoming one. It's critical to establish a sufficient team of technical reviewers because it ensures that there are enough reviewers to support the quality of docs and impel an efficient PR process

There are two options for establishing the group of technical reviewers:

1. o3de/docs-technical-reviewer: A team of technical reviewers specifically for O3DE documentation. Choosing this options requires a plan to establish and maintain the team. The sig-docs-community would take initiative and work with members of other SIGs to establish this.
2. o3de/reviewers: An already established team of reviewers, that were initially created for reviewing O3DE source code. Choosing this option allows code reviewers to also be valid technical reviewers for docs. Reviewers are nominated in their respective SIGs.

The work to establish a team of technical reviewers may also include reassessing the current sig-docs-community Nomination Guidelines for technical reviewers.

What are the advantages of the suggestion?

• Automatically ensures a PR receives adequate reviewer approvals, at a minimum, before it can be merged.
• Helps improve the quality of the documentation, including how accurate the technical information is and how well it's written.
• Clarifies responsibilities among o3de teams in the PR process for o3de.org repo.

What are the disadvantages of the suggestion?

None are identified at this time.

How will this be work within the O3DE project?

This will be implemented in the PR process in o3de.org repo.

Scope of work

There are two main tasks for this RFC:

• The CODEOWNERS file and the o3de.org:main and o3de.org:development branches must be set up to enforce required reviews from one docs reviewer and one technical reviewer. For more information, see Review and approval permissions in Suggestion design description section.
• It must be decided on who will populate the team of technical reviewers. For more information, see Technical reviewers in Suggestion design description section.

Are there any alternatives to this suggestion?

An alternative to the suggestion proposed in this RFC is to keep the review requirement rules as they currently are. The impact of this would result in repeated issues that have occasionally occurred in the PR process for o3de.org repo. Issues include merged PRs that amend technical docs and did not receive an approval from an appropriate technical reviewer, or did not receive an approval from a docs reviewer. A lack of structure and unclear roles in the PR process is not scalable for both the quality of O3DE documentation and for the growing community of contributors to o3de.org.

What is the strategy for adoption?

If approved, sig-docs-community can explore, test, and implement a solution that enforces the proposed required reviewers for the o3de.org repo. As this directly involves members of other SIGs who may be technical reviewers, it's necessary to get their input. If necessary, the sig-docs-community can coordinate with other SIGs to establish the team of technical reviewers. Because members of the O3DE community have already regularly acted as technical reviewers on PRs relevant to their expertise, this RFC does not shift any workloads. It simply provides clarification on ownership of PR reviews.