Do we want tom improve our branch protection?

[bossenti]

So far we don't have a lot of rules in place with respect to our git flow and branch protection.
Therefore, I've screened the documentation of the .asf.yaml file to discover what opportunities we are provided with from Apache INFRA side.
I've come up with some suggestions on how we could improve our workflow.

To be honest, some of them might be rather opinionated and some of them might not be suitable or practical in a small(er) open source project. That's why I would like to start a discussion on the proposal below and am happy to here your opinion.

config parameter	proposed value	outcome/consequences
del_branch_on_merge	true	Branches are automatically deleted when a PR is merged, this prevents a cemetery of stale branches
enable_merge_buttons.squash	true	Enables squash as merge strategy in PRs (related to required_linear_history, see below)
enable_merge_buttons.merge	false	Disables merge as merge strategy in PRs
enable_merge_buttons.rebase	false	Disables rebase as merge strategy in PRs
protected_branches	dev, master	Disables rebase as merge strategy in PRs
required_status_checks.strict	true	Requires branches to be up to date (no commits behind target) before they can be merged
required_status_checks.contexts	validate_pr	Checks (GitHub workflows) that need to be passed before a PR can be merged (enforces successful tests etc.)
required_pull_request_reviews.dismiss_stale_reviews	true	Approvals are withdrawn when commits are added to the PR (not sure whether this is applicable in a project of our size)
required_pull_request_reviews.require_code_owner_reviews	true	Reviews need to be performed from people with write access to the repository (
required_pull_request_reviews.required_approving_review_count	1	Minimum number of approvals required before a PR can be merged
required_linear_history	true	Enforces a linear history of commits

Besides the configuration changes itself it would like to hear your thoughts on:

• Do we want to have different branch protection rules for dev and master (e.g., we could only allow signed commits additionally on master)?
• Do we want to have the same rules for streampipes-website?

One outcome I'd like to get out of this discussion is a set (this might also be empty) of permission that we want to adapt soon.
Therefore, I'd create a follow-up issue to apply the changes and document the new workflow properly (e.g. in our contributing.md or developing.md)

[tenthe]

Hi @bossenti,
thanks for starting this discussion.
I really like rules above, but I am not sure if we should enforce them or rather set them as a default behaviour.
There may be situations where it is better to bypass the rules to reduce the overhead.

• del_branch_on_merge: true (+1)
• enable_merge_buttons.squash
  • I would like to have this as a default, but I would not completely disable the other options
• required_status_checks.strict: true (+1)
• required_status_checks.contexts:
  • The checks should always run, but I had situations before, where it was easier to merge the PR (even with failing checks). In such situations those problems were already fixed in another PR that couldn't directly be merged.
• required_pull_request_reviews.dismiss_stale_reviews
  • I would set this to false. As you stated we can activate it when the project is bigger
• required_pull_request_reviews.required_approving_review_count: 1
  • I also like this rule, but sometimes there are situations where a PR has to be merged directly. But this should happen only rarely.
• required_linear_history : true
  • Here I am not quite sure what the consequences will be

[bossenti]

Catching up on this discussion..

enable_merge_buttons.squash

I would like to have this as a default, but I would not completely disable the other options

Yeah, this strongly relates to how we want our git workflow to look like

* `required_status_checks.contexts`:
  
  * The checks should always run, but I had situations before, where it was easier to merge the PR (even with failing checks). In such situations those problems were already fixed in another PR that couldn't directly be merged.

But which value does it bring when you merge a PR that contains that does not run either before merging another PR?
In my opinion you should just wait in this case.

* `required_pull_request_reviews.required_approving_review_count`: `1`
  
  * I also like this rule, but sometimes there are situations where a PR has to be merged directly. But this should happen only rarely.

Might probably also be an option for the second iteration

* `required_linear_history` : `true`
  
  * Here I am not quite sure what the consequences will be

I wasn't entirely sure about it as well and did some research.
In short, it does prevent you from doing merge commits.
As this is moreless only for the sake of asthetics, I'm not in favor on enabling it.
This post provides some context.

So as a first outcome, we would consider the following options for a first iteration of improving our branch protection:

del_branch_on_merge -> true
protected_branches -> dev, master
required_status_checks.strict -> true
required_status_checks.contexts -> depends on the outcome of our discussion 😉

[dominikriemer]

I'm ok with this - do we need to pay any special attention to the master merge after releasing (this strange behaviour documented in the wiki)?

[bossenti]

Well I don't want to enforce these changes, it's just my take to improve our development workflow a bit 🙂
I think it does only make sense to apply the changes if we all agree on them

Would the suggested rules some break the release step?

[bossenti]

I have created a first PR (#1227) that covers the two basic concepts
Enforcing successful builds are postponed until we simplify our test setup
We still don't have any specific branch protection rules for our master branch