GitHub user zhztheplayer edited a comment on the discussion: Infra: Remove the approval restriction of merging PRs
@zhouyuan > I guess your ask is to fast merge on some patches(e.g. big refactors, which > may easily get conflict) Fast merging is not the goal. Indeed merges can be faster because of the change, but only for the ones committer is confident with them. I am always not confident with my PRs. For example the changes that involve CH code, https://github.com/apache/incubator-gluten/pull/8198, https://github.com/apache/incubator-gluten/pull/8008, etc. Review is always required from the guys I pinged. For the objectives mentioned: > Basic Code Quality Control: Ensuring that the code meets our quality > standards. I know this is the purpose of the restriction, but I don't think our code quality is controlled because of the rule. I review a bunch of PRs daily, and I still have to do [many follow-up works](https://github.com/apache/incubator-gluten/pulls?q=is%3Apr+follow+is%3Aclosed+author%3Azhztheplayer+) to maintain the code quality of the code base. The key of avoiding the corruption of code is to encourage all the committers to take more responsibility of useful code reviews, this will be a team work consisting of individual efforts. Simply setting a rule doesn't help any. > Onboarding and Knowledge Sharing: Helping team members familiarize themselves > with other components (e.g., RAS). Code review is a good way for getting more knowledge about the code. But it's not related to the review approvals. One is aways encouraged to learn code through reviewing others' PRs, no matter we have a restriction of approvals or not. > These are the two critical growth factors currently missing for Gluten IMO. > Once we establish a process for thorough and effective reviews, every patch > will benefit from meaningful feedback. Again, rules are good, but they don't go with the expected way as you wished. If you set a rule to consider a review with 5 comments is a effective review, then some people may leave unreasoned comments on the smallest PRs which will give you side effect. The community is all about people. Committers and the PMC members were elected with good reasons, we should trust the people we invited, then build a effective model based on the trust. If we don't have enough experienced committers who we can trust, that will be the community's failure. Adding rules will not cover this kind of failure. > BTW The branch protection(required review) feature is added on 2020. Git-to-GitHub migration was just [done in 2019](https://news.apache.org/foundation/entry/the-apache-software-foundation-expands) in ASF. ASF was founded before 2000. Projects worked pretty well without GitHub in the first 20 years of ASF. If such rule is required, it should already be documented like "+1 votes are needed for code change" in ASF's documentation before 2000. As I said, the ASF encourages [meritocracy](https://www.apache.org/foundation/how-it-works/#meritocracy). This is the reason why there is not such restrictions from the ASF. GitHub link: https://github.com/apache/incubator-gluten/discussions/8556#discussioncomment-11865324 ---- This is an automatically sent email for [email protected]. To unsubscribe, please send an email to: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
