[PROPOSAL] Allow repos to decide how many approvers (1, 2, ...) they want for code reviews

dblock commented 1 year ago

What/Why

What are you proposing?

Most repositories in opensearch-project require 2 code reviewers. Relax that to 1+. Enforce. Document in https://github.com/opensearch-project/.github/blob/main/RESPONSIBILITIES.md#maintainer-responsibilities and https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md#review-process that 1+ review is required.

What users have asked for this feature?

we don't have that many maintainers in OpenSearch clients, and feel that 1 maintainer CR is enough
https://github.com/opensearch-project/opensearch-hadoop/pull/29#issuecomment-1329399080

What problems are you trying to solve?

Increase PR review/merge velocity.

Make the branch protection rules more consistent. Current state is 11 repos require none, 17 require 1, 57 require 2:

$ for r in $(gh repo list opensearch-project --visibility public --limit 100 | cut -f1); do echo $r: `gh api repos/$r/branches/main/protection/required_pull_request_reviews | jq ".required_approving_review_count"`; done
opensearch-project/opensearch-testcontainers: 1
opensearch-project/observability: 2
opensearch-project/OpenSearch: 1
opensearch-project/opensearch-js: 2
opensearch-project/documentation-website: 2
opensearch-project/opensearch-build-libraries: 1
opensearch-project/security-dashboards-plugin: 2
opensearch-project/opensearch-ci: 1
opensearch-project/opensearch-build: 1
opensearch-project/opensearch-ruby: 2
opensearch-project/spring-data-opensearch: 1
opensearch-project/anomaly-detection: 2
opensearch-project/sql: 2
opensearch-project/notifications: 2
opensearch-project/dashboards-maps: 2
opensearch-project/opensearch-sdk-java: 2
opensearch-project/opensearch-py-ml: 2
opensearch-project/security: 2
opensearch-project/maps: 2
opensearch-project/index-management: 2
opensearch-project/opensearch-hadoop: 0
opensearch-project/opensearch-php: 2
opensearch-project/index-management-dashboards-plugin: 2
opensearch-project/OpenSearch-Dashboards: 2
opensearch-project/cross-cluster-replication: 2
opensearch-project/alerting-dashboards-plugin: 2
opensearch-project/opensearch-api-specification: 1
opensearch-project/oui: 2
opensearch-project/helm-charts: 2
opensearch-project/.github: 2
opensearch-project/data-prepper: 1
opensearch-project/opensearch-java: 2
opensearch-project/security-analytics-dashboards-plugin: 2
opensearch-project/opensearch-py: 2
opensearch-project/neural-search: 2
opensearch-project/opensearch-net: 2
opensearch-project/ml-commons: 2
opensearch-project/project-website: 1
opensearch-project/performance-analyzer: 2
opensearch-project/search-relevance: null
opensearch-project/opensearch-dsl-py: 2
opensearch-project/project-meta: 1
opensearch-project/common-utils: 2
opensearch-project/opensearch-cluster-cdk: 1
opensearch-project/opensearch-oci-object-storage: null
opensearch-project/simple-schema: null
opensearch-project/dashboards-reports: 2
opensearch-project/opensearch-plugins: 2
opensearch-project/security-analytics: 2
opensearch-project/dashboards-anywhere: null
opensearch-project/opensearch-go: 2
opensearch-project/job-scheduler: 2
opensearch-project/dashboards-search-relevance: 1
opensearch-project/performance-analyzer-rca: 2
opensearch-project/anomaly-detection-dashboards-plugin: 2
opensearch-project/geospatial: 2
opensearch-project/alerting: 2
opensearch-project/asynchronous-search: 2
opensearch-project/k-NN: 2
opensearch-project/opensearch-benchmark-workloads: null
opensearch-project/opensearch-dashboards-functional-test: 2
opensearch-project/dashboards-visualizations: 1
opensearch-project/ansible-playbook: 2
opensearch-project/opensearch-rs: 2
opensearch-project/dashboards-desktop: null
opensearch-project/project-tools: 0
opensearch-project/terraform-provider-opensearch: 1
opensearch-project/logstash-output-opensearch: 2
opensearch-project/dashboards-docker-images: 1
opensearch-project/project-website-search: 2
opensearch-project/opensearch-clients: 2
opensearch-project/opensearch-catalog: null
opensearch-project/opensearch-benchmark: 2
opensearch-project/oui-docs-cdk: null
opensearch-project/opensearch-plugin-template-java: 2
opensearch-project/perftop: 2
opensearch-project/ux: null
opensearch-project/opensearch-net-abstractions: 2
opensearch-project/docker-images: 1
opensearch-project/opensearch-cli: 2
opensearch-project/i18n-plugin: 0
opensearch-project/opensearch-dashboards-test-library: 2
opensearch-project/logstash-input-opensearch: null
opensearch-project/piped-processing-language: 2
opensearch-project/opensearch-devops: 2
opensearch-project/dashboards-notebooks: 2

What is the developer experience going to be?

Repo maintainers decide how many approvals they want and document it in their CONTRIBUTING.md. Admins will help with branch protection rules.

dblock commented 1 year ago

@CEHENKLE I think we want to document that branch protection is required when creating new repos, 10 repos don't

dbwiddis commented 1 year ago

Add to your "what repositories have asked for this": https://github.com/opensearch-project/opensearch-sdk-java/issues/255

peternied commented 1 year ago

Great idea, we should definitely enforce a minimum project wide.

+1 for Relax that to 1+, if and only if CODEOWNERS have also been published in that project. Do you think we can do both once?

Rational for those that are curious; Bypassing required reviews using GitHub Actions TLDR: Someone crafty can use GitHub Actions can craft a pull requests that approves itself

opensearch-project / .github