Discuss branch protection rules (based on node-wot PR)

[danielpeintner]

We recently merged #14 to setup branch protection rules for node-wot.
Some comments/concerns based on eclipse-thingweb/node-wot#1240

• Approvals need to come from 2 team members (approvals from externals like @Kaz040 in PR Handle AID terms on affordance level and form level node-wot#1240 do not work, which is somewhat okay, but see next point...)
• No way to overrule the protection (might be problematic in some cases if we need to merge a PR quickly)
  

I am not sure anymore whether this setting "as is" is very useful/practical...

@egekorkan @JKRhb @relu91

[relu91]

I found surprising that we can't override the rule, maybe there is some additional configuration value to set to allow override by core maintainers. I'll check it asap.

[JKRhb]

• Approvals need to come from 2 team members (approvals from externals like @Kaz040 in PR

Maybe we can adjust that to only require one team member? I think if you open a PR as a team member yourself, you do not count against the number of required approvals (if I am not mistaken), so requiring only one could make sense in general.

[netomi]

for some projects setting the approval count to 1 works better.

Also its possible to specify a list of bypass actors, however this does not work well with status checks, in such cases using a ruleset instead of a branch protection rule is advised.

[danielpeintner]

for some projects setting the approval count to 1 works better.

+1

Note: based on my own PR eclipse-thingweb/node-wot#1230 I checked whether I can approve my own PR. Apparently this is not possible. This means that other team members approval only counts. PR creators themselves are not allowed to approve.

---

This means even lowering the number to 1 means that there is no other possibility to do a quick merge for some very pressing PRs.

[netomi]

indeed, you cant approve your own PRs as this would render the whole idea of approvals useless.

Here is an example of a ruleset for a project that specifies that all people with write access can bypass it for the time being: https://github.com/eclipse-zenoh/.eclipsefdn/blob/main/otterdog/eclipse-zenoh.jsonnet#L4. While I would not advised that for something like the main branch, for a development branch this makes certainly sense. It also depends on the maturity of your projects. If its under active development, you probably want to integrate changes quicker. Maybe a change of the branching model helps to speed up of development. Once development is stable, merge changes to e.g. main and release from there.

[netomi]

also fyi: our main concern for setting up branch protection rules or rulesets for a project is to prevent force pushes to the main branches and we certainly do not want to hinder our development process.

[danielpeintner]

indeed, you cant approve your own PRs as this would render the whole idea of approvals useless.

I agree, it would definitely be BAD practice approving the own PR and SHOULD be discouraged.
Anyhow, I am just looking for an easy way to approve PRs that are pressing and others are simply not able to approve at the moment.

[JKRhb]

indeed, you cant approve your own PRs as this would render the whole idea of approvals useless.

Here is an example of a ruleset for a project that specifies that all people with write access can bypass it for the time being: https://github.com/eclipse-zenoh/.eclipsefdn/blob/main/otterdog/eclipse-zenoh.jsonnet#L4. While I would not advised that for something like the main branch, for a development branch this makes certainly sense. It also depends on the maturity of your projects. If its under active development, you probably want to integrate changes quicker. Maybe a change of the branching model helps to speed up of development. Once development is stable, merge changes to e.g. main and release from there.

Hmm, would be an option then to have a special role for the main maintainers of a repository, i.e. usually one or two people, and then use that a bypass rule? I would say that not everyone with write access should be able to bypass the rules, but @danielpeintner has a point that there may be scenarios where quick action could be necessary. However, that should of course not be the norm and could be handled in a stricter way after a 1.0.0 release.

[relu91]

If the bypass rule can be fine-tuned to allow only certain accounts, I would add @danielpeintner and myself there and keep the double review policy. What do you think?

[netomi]

we can certainly create a new team with a custom set of members that is added as bypass actors, this is exactly what we did for jetty:

https://github.com/jetty/.eclipsefdn/blob/main/otterdog/jetty.jsonnet#L375

the custom jetty-pmc group was created to support that.

[egekorkan]

We have talked about this in today's call:

• We agree that we will switch back to 1 approval as the main rule. -> @relu91 will take action on that.
• Informally we will need 2 approvals, especially for "big", "delicate" or "controversial" changes
• We can evaluate adding the bypass factors later (especially relevant for .md file changes)
• For other Thingweb components:
  • We do not see developer contributions to other components (they are used but no contributions we see).
  • Playground Web interface is easy to test via Netlify previews
  • For TD-Tools, we will need more stability if they get used.
  • Node-Red needs more tests at least since they are difficult to test by checking out a PR.

[egekorkan]

We can close this as #17 has fixed it