Main Branch Protection Rules in hiero-ledger Repos

[jwagantall]

As we get closer to the end of the repo transfer, we need to lock down the protection rules for
our repos.

This is what I have in mind, please let me know if you think these are too strict and let's get the conversation rolling.
(BTW, most of these rules are quite standard in other LFDT orgs but we can talk about any of them in particular if anyone thinks otherwise)

Main branch protection rules:

• DCO enforcement
• Require PR before merging
  - Require at least 1 approval
  - Dismiss stale PR approvals when new commits are pushed
  - Require conversation resolution (flexible, but good to have to avoid misunderstandings or missed concerns)
• Require linear history (to prevent merge commits to be added in the history) (Flexible too, but I think this can help our stats)
• Require status check pass: DCO and any other GHA related to Verify workflows
• Block force pushes
• Require signed commits
• Restrict branch creation
• Restrict branch deletion (On by default)

Please let me know any comments/ideas/opinions to have the right protection rules for this community.

[hendrikebbers]

I already was in a solution several times on that only a force push helped me move forward. Why it is not what you normally should do I ask myself if we should disallow it in general. Rest makes sense to me. Adding @rbarker-dev @nathanklick for additional input

[jwagantall]

IMO I like to avoid force pushes since it basically breaks people's clones of their repo.
Re-writing history in the main branch is not a good practice since it can spoil it's reliability (broken history). If not using it correctly, it can also do a lot of damage.
I only recommend doing this practice whenever accidentally something has leaked in the code and/or history needs to be re-written due to legal reasons which should be a TSC's call to approve.
I have not dig too far into the active workflows yet, but I would be concern of causing CI breakages too and/or loose traceability of releases.

[exploreriii]

Force push has helped me in the past too. There could be better ways out there. I've already learnt lots of tips from talking with other devs. Perhaps some kind of complimentary guides could help

[rbarker-dev]

Force push is blocked specifically to prevent members from rewriting history. In the extreme cases, as @jwagantall mentioned, there is a group of members who would be authorized to perform that action at the TSCs direction.

[rbarker-dev]

@jwagantall we have a series of standard rules that we've applied to our repositories in the hashgraph org. These include the following:

• require linear history
• restrict branch creation
• restrict branch deletion
• require signed commits
• block force pushes
• Require PR before merging
  • Require at least 1 approval
  • Dismiss stale PR approvals when new commits are pushed
  • Require conversation resolution
  • Allowed merge methods: Squash 🚧
• DCO enforcement
• Require Status Checks to complete: 'Code Compiles' (on actions enabled repositories) 🚧
• Require Status Checks to complete: 'conventional-commits title check' 🚧

We include additional checks on actions-enabled repositories for coverage analysis and unit test status

We also include the following tag limits: 🚧

• Restrict creations
• Restrict updates
• Restrict deletions
• Require linear history
• Require signed commits
• Block force pushes

Note: Everything annotated with 🚧 is a difference from the list you came up with.

[andrewb1269hg]

The SDK-TCK-repo has a single rule. Applying the hashgraph standard rules as Roger listed above allows for granularity and sets the right permissions. Should we start in this repo?

[jwagantall]

These rules look fair to me. We should get more eyes on this discussion and obtain an official vote

[jwagantall]

Thank you for your inputs @rbarker-dev!
It makes sense to me and I agree with the rules that you guys have implemented already.
My only comment:
I usually don't feel too comfortable with squash commits since it can lead to messy commit history and could potentially make branching models look wonky but I am not opposed to that, they are just a little difficult to work with.

[andrewb1269hg]

We squash each PR into a single commit, since each PR is a unit of work that has been reviewed and tested. We are able to cherry-pick these single commits to other branches as needed.

[jwagantall]

Thank you Andrew! Will update my proposed rules in the next comment.

[jwagantall]

Based on the previous comments, this is the proposed rules for branch protections:

• Require linear history
• Restrict branch creation
• Restrict branch deletion
• Require signed commits
• Block force pushes
• Require PR before merging
• Require at least 1 approval
• Dismiss stale PR approvals when new commits are pushed
• Require conversation resolution
• Allowed merge methods: Squash
• DCO enforcement
• Require Status Checks to complete: 'Code Compiles' (on actions enabled repositories)
• Require Status Checks to complete: 'conventional-commits title check'
• Require Status Checks to complete: Tests (designated by the repo maintainers)
• Restrict tag creations
• Restrict tag updates
• Restrict tag deletions
• Require tag linear history
• Require tag signed commits
• Block tag force pushes

[exploreriii]

I generally agree with the list, offering a good balance of improved security without too much extra faff.

I disagree with, for now:
Require conversation resolution - I think does more harm by adding extra layer without benefit
Possibly: Force pushes

The lists are quite long now and it is a lot more difficult to get through a pull request. Can create quite a bit of spam if users are not fully trained. A gradual build up with documentation could be nice.

[fessmm]

IMO at least 2 approvals should be required

[hendrikebbers]

@andrewb1269hg @rbarker-dev I agree regarding all repos we moved from Hedera to Hiero and other repos that will be maintained by Hashgraph. But let's assume we add an already existing project that has it's own workflows (maybe even naming standards) for commit messages and creating release nodes to Hiero. For that project the rule will be a problem. All other rules have a security benefit or help to not "destroy other peoples work" (like avoid force push). The conventional-commits title check on the other hand is a different story. I see the benefits of it and do not want to say that we should remove it for repos that use it now. I just disgree that it should be a must for any project under Hiero.

[hendrikebbers]

@fessmm While I agree with you in general, we can have smaller projects that are added to Hiero with only a handful of initial committers. Here, having two approvals can become complicated. Maybe we define it for a specific subset of projects. Like we need that on all core projects (need to run a network) but not on ecosystem projects (like the GitHub Action for testing).

[jwagantall]

IMO at least 2 approvals should be required

2 approval will be implemented in bigger/riskier repos like the consensus node.
By default 1 approval, but 2 approval needed for those project owners that would like to implement it.

[jwagantall]

@hendrikebbers @andrewb1269hg @rbarker-dev @fessmm @exploreriii .. is this a better list?
In bold are the changes to reflect the new changes with the previous list

Require linear history
Restrict branch creation
Restrict branch deletion
Require signed commits
Block force pushes
Require PR before merging
Require at least 1 approval (2 approval for bigger repos and up to the project owners to adopt it)
Dismiss stale PR approvals when new commits are pushed
Require conversation resolution
Allowed merge methods: Squash
DCO enforcement
Require Status Checks to complete: 'Code Compiles' (on actions enabled repositories)
REQUIRED Status Checks to complete: 'conventional-commits title check' (on actions enabled repositories)
Require Status Checks to complete: Tests (designated by the repo maintainers)
Restrict tag creations
Restrict tag updates
Restrict tag deletions
Require tag linear history
Require tag signed commits
Block tag force pushes

[rbarker-dev]

I still think this should be required on all actions-enabled repos:

NOT REQUIRED Status Checks to complete: 'conventional-commits title check'

Otherwise the list looks good to me.

[jwagantall]

I still think this should be required on all actions-enabled repos:

NOT REQUIRED Status Checks to complete: 'conventional-commits title check'

Otherwise the list looks good to me.

I agree too.. I will let @hendrikebbers cast his input on this one. I am not opposed to removing it but would like to have it enabled

[hendrikebbers]

Let’s do a short call on that single open point :)

[hendrikebbers]

@rbarker-dev @jwagantall and I had a call, and we agreed on all the rules. We will set up a special handling for the TSC repo to make the preparation of minutes files more manageable.

[jwagantall]

Presented this to the TSC and I will go ahead and implement these rules.

[jwagantall]

Audit in place, meeting with the team to evaluate the results

[jwagantall]

These rules were audited and put in place in today's sync up with the team