Proposal: require review rules for config files

Hi, developers!

As discuss with @bb7133 @XuHuaiyu @BusyJay @tison and other contributors, we agree on that important variables and config files should be reviewed by domain experts to ensure the quality and compatibility.

TiDB

For TiDB, the following files are issued:

• https://github.com/pingcap/tidb/blob/master/sessionctx/variable/
• https://github.com/pingcap/tidb/blob/master/config/config.toml.example

changes on the file or directory should be reviewed by anyone of:

• @dongyuusa
• @bb7133
• @XuHuaiyu
• @cfzjywxk
• @morgo

TiKV

For TiKV, the following files are issued:

• tests/integrations/config/test-custom.toml
• etc/config-template.toml
• components/cdc/src/config.rs
• components/batch-system/src/config.rs
• components/pd_client/src/config.rs
• components/sst_importer/src/config.rs
• components/raftstore/src/store/worker/split_config.rs
• components/raftstore/src/coprocessor/config.rs
• components/encryption/src/config.rs
• src/coprocessor_v2/config.rs
• src/storage/config.rs
• src/server/gc_worker/config.rs
• src/server/lock_manager/config.rs
• src/server/config.rs
• src/config.rs

changes on these files should be reviewed by @BusyJay.

In order to add the rules in review PR, we plan to add the review required bot in pingcap/tidb and tikv/tikv repos.

I think it is still under discussion instead of an announcement.

I’d like to know whether it is in “or” or “and” logic?

How to? I don’t think a new bot is a good idea.

Is it duplicated with itself? All files under sessionctx/variable are issued so that we don’t list others, or in face only those listed files issued?

edited, it should be “or”

Yes. all files under sessionctx/variable, edited.

It is just a config or ti-chi-bot, we will not introduce a new bot

I think we can nominate another one TiDB committer to cover the scope of planner. I highly respect @dongyuusa and his experience on planner domain privately. However, we should follow some community principle that one earns authority by contribution, explicit and visible in the community.

I’d like to recommend @qw4990 to serve and cover planner as he has been TiDB planner veteran and contributed for years, and earned the trust from the community. I believe he would serve the planner and community very well.

Gotcha and thanks for the trust. Let me take on this responsibility ~ @tison

This PR will enable the required review rules for config files on TiDB repo:

We have enable review rules for config files in TiDB

It is reverted by https://github.com/ti-community-infra/configs/pull/432. @Mini256 & I think it is far away mature that we only blocked ourselves.

I propose to use Github’s CODEOWNERS mechanism as one of the alternatives, I want to know if this scheme is feasible.

cc: @tison @zhouqiang-cl @zhangyangyu

Use the branch-protection plugin to enable the “require review from code owners” option in the repository branch protection. Before enabling this option, you must enable “require approving review count” at the same time. (eg. https://github.com/ti-community-infra/tichi/pull/742)

image.png1614×1172 140 KB

(Notice: The individual or team as the code owner must have write permission in the repository.)

image.png1838×708 89.9 KB

Then we can define CODEOWNERS file for the repository.（About code owners - GitHub Docs）

image.png1280×236 17.6 KB

GitHub will prompt which reviewers are needed for review, and it will request code owners as reviewers to review.

image.png1880×944 116 KB

If other reviewers pass the review, after the /merge operation, the code owner still does not review, and the bot’s merge operation will be blocked.

image.png1898×686 66 KB

But the robot will continue to try to merge (every two minutes). Tide currently does not include “waiting for code owner review” as one of the merge required conditions.

Actually, we should make PRs that cannot be merged exit the merge queue by the /hold command.

image.png2830×758 156 KB

Thanks for your suggestion! It seems like a good direction. Could you create one or more PRs to see how much work we should do to implement it?