Skip to content
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

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

Open
dblock opened this issue Nov 28, 2022 · 3 comments

Comments

@dblock
Copy link
Member

dblock commented Nov 28, 2022

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?

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
Copy link
Member Author

dblock commented Nov 28, 2022

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

@dbwiddis
Copy link
Member

dbwiddis commented Dec 3, 2022

Add to your "what repositories have asked for this": opensearch-project/opensearch-sdk-java#255

@peternied
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants