Add protected branches configuration for main branch#642
Add protected branches configuration for main branch#642
Conversation
Signed-off-by: Dave Fisher <dave2wave@comcast.net>
raboof
left a comment
raboof
left a comment
There was a problem hiding this comment.
LGTM but would be good to have an infra confirmation as well
.asf.yaml
Outdated
| features: | ||
| issues: true | ||
| protected_branches: | ||
| main: {} |
There was a problem hiding this comment.
Do we need to specify some options here?
https://github.com/apache/infrastructure-asfyaml?tab=readme-ov-file#branch-protection
for example, for another apache project, we use
protected_branches:
main:
required_pull_request_reviews:
required_approving_review_count: 1
There was a problem hiding this comment.
If not explicitly specified, these values will be used by default:
required_signatures: false
required_linear_history: false
required_conversation_resolution: false
required_pull_request_reviews:
dismiss_stale_reviews: false
require_last_push_approval: false
require_code_owner_reviews: false
required_approving_review_count: 0
required_status_checks:
strict: false
contexts: ~
checks: ~
i think we need to add some options, otherwise default doesnt enforce anything
There was a problem hiding this comment.
the default will disallow pushing to the main branch, I think, right? That would be a good start... though come think of it I wonder if that wouldn't also prevent the GHA automations that update approved_actions.yml and .github/workflows/dummy.yml from pushing to the main branch?
There was a problem hiding this comment.
Please commit changes to the branch. If there are problems after the merge we can revert and fix.
There was a problem hiding this comment.
By default, each branch protection rule disables force pushes to the matching branches and prevents the matching branches from being deleted. You can optionally disable these restrictions and enable additional branch protection settings.
There was a problem hiding this comment.
we can merge and give it a try too
.asf.yaml
Outdated
| features: | ||
| issues: true | ||
| protected_branches: | ||
| main: {} |
There was a problem hiding this comment.
If not explicitly specified, these values will be used by default:
required_signatures: false
required_linear_history: false
required_conversation_resolution: false
required_pull_request_reviews:
dismiss_stale_reviews: false
require_last_push_approval: false
require_code_owner_reviews: false
required_approving_review_count: 0
required_status_checks:
strict: false
contexts: ~
checks: ~
i think we need to add some options, otherwise default doesnt enforce anything
Agree -> required_approving_review_count = 1 likely. |
|
Pulsar has some interesting notes about CI workflows these go with their |
Add required status checks and pull request review settings for the main branch. Signed-off-by: Dave Fisher <dave2wave@comcast.net>
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
not sure if we want to set require_code_owner_reviews, everything else looks good to me
| required_status_checks: | ||
| # strict means "Require branches to be up to date before merging". | ||
| strict: false |
There was a problem hiding this comment.
this is already the default value, but doesnt hurt to be explicit
| strict: false | ||
| required_pull_request_reviews: | ||
| dismiss_stale_reviews: false | ||
| require_code_owner_reviews: true |
There was a problem hiding this comment.
should we set this? default is false, who are the code owners ?
| # strict means "Require branches to be up to date before merging". | ||
| strict: false | ||
| required_pull_request_reviews: | ||
| dismiss_stale_reviews: false |
There was a problem hiding this comment.
same as above, dismiss_stale_reviews: false is the default but doesnt hurt to be explicit
| required_pull_request_reviews: | ||
| dismiss_stale_reviews: false | ||
| require_code_owner_reviews: true | ||
| required_approving_review_count: 1 |
4 participants
As @potiuk suggested in #638