Per-package merge permissions in foreman-packaging for plugin maintainers

Development RFCs
1

Ohai!

I would like to propose (well, really, repeat a proposal that we heard in the past) to give plugin maintainers the permission to merge packaging changes to their respective plugins.
Who, if not the maintainers, know best what changes need to happen to the package of a plugin?
Also, there is not really an extension of permissions – of people wanted to do something malicious, they already could do so by doing a malicious release that gets packaged.

As a practical example, this would mean that we’d give the @theforeman/remote-execution team the permission to merge PRs in rpm/develop if they only touch files in:

  • plugins/rubygem-foreman_remote_execution/
  • plugins/rubygem-smart_proxy_remote_execution_ssh/
  • plugins/rubygem-hammer_cli_foreman_remote_execution/

Implementation

The idea is to implement this using branch protection rules and CODEOWNERS.

We can configure GitHub to only allow pushes to rpm/develop (and others) if there is a PR and that PR is approved by a code owner.

Then we configure the CODEOWNERS file to read something like this:

* @theforeman/packaging
/packages/plugins/*remote_execution*/ @theforeman/packaging @theforeman/remote-execution

Which means that things that match *remote_execution* are owned by packaging and rex teams, the rest only by packaging.

We can now give @theforeman/remote-execution merge permissions to the whole repository (as you can’t give per-path permissions) and the branch protection will still disallow merges to things outside REX packages.

As a side-effect, this will also generate notifications to the REX team whenever someone (or something, like our automation) opens a PR to one of their packages.

Open Questions

Do we want this?

While my assumption is yes, I still want to hear if someone is against this proposal :wink:

Do we implement that for all teams, or opt-in?

Giving teams/people merge rights and adding them as code owners will subscribe them to changes in this repo and also generate notifications when a review a requested.
If we implement that for all plugins in one go, this can create quite some notification load on the humans involved. The alternative would be to start with an “empty” set and add people/teams as they think it makes sense for them.

5 Likes
2

Thank you for putting this together, this will make at least my life easier.

Does “and others” mean something like rpm/* or {rpm,deb}/develop?

I can’t speak for others, but if we start with an empty list, please do sign me up in there.

3

I’d assume {rpm,deb}/* but we should also set up owners for master for consistency.

I recall seeing a feature to lock older branches, so I’d suggest to add a CODEOWNERS for supported branches ({rpm,deb}/{3.1[0-2],develop} & master) or lock it (unsupported versions).

We can create sub-teams, so @theforeman/remote-execution/packaging. For smaller teams this may be overkill when there’s a complete overlap between the two, but for larger teams it can be a useful thing.

Note you can change the watch status for the repository from “All Activity” to “Participating and mentions”. Since a review was requested automatically, it should consider you as Participating.

1 Like
4

Yeah, I’d start with (rpm|deb)/develop and enhance to whatever “stable” we have running.
Others should be locked down (somehow).

One thing on the implementation side to consider: branch protection doesn’t copy over when you branch, so this will need adjustments to the process (and ideally automation).

1 Like
5

Fine by me.

We can implement a simple CODEOWNERS with a single * @theforeman/packaging entry for existing stable branches, but how do you imagine branching? CODEOWNERS will also be branched. Do you want to clean it up on branching or keep it?

GtiHub has 2 mechanisms: branch protection and rulesets. The former only has simple patterns (so you can use rpm/*). The latter can also define targets via multiple criteria. You can even add bypass rules.

This means we can define a rule that mandates our CI to pass (including repoclosure) but then allow packagers to bypass it when we know our repoclosure check is going to break.

I believe rulesets are newer and were intended to address the limitations of branch protection, but they haven’t started to push people to it that much. That makes me think we should look at rulesets first to see how we can define them.

6

How are other user roles handled in these schema? For example, are maintainers bound by the rules or still global to merge for the repository?

7

With branch protection it’s limited to include/exclude admins but rulesets allow you to specify users/groups are included or excluded.

As for ownership: because of ownership line * @theforeman/packaging I think packaging should still have ownership of all files and other teams are just added, not instead of.

8

With the setup that I tested (branch protection + codeowners), plugin maintainers would have merge permission on any PR IFF it’s approved by the correct codeowner. So an Ansible PR can be merged by the REX team if it was ack’ed by the Ansible team.

Do you see that as a problem?

Or what did you mean by “other user roles”?
People who are also in @theforeman/packaging? See below.

Nope, packaging needs to be added explicitly to all codeowner entries, as only the matching entry is evaluated.

If you want to match two or more code owners with the same pattern, all the code owners must be on the same line. If the code owners are not on the same line, the pattern matches only the last mentioned code owner.

9

Like @evgeni I also created a test repository, but using rulesets: GitHub - theforeman/foreman-packaging-ruleset-test: Testing out rulesets

I opened a PR that should roughly how I’d like to see it:

Since we may delete it, I’m kaing a screenshot for the future:

image
image1212×1311 162 KB

Some things to note:

  • It requests a review due to code owners (just like @evgeni’s example)
  • I set up required checks to mandate CI. Note I split build and repoclosure, which makes it easier to see if one or the other failed
  • Auto-merge is enabled because there are required checks. This allows approving a PR and merge it when CI is green. Very useful for the request 1 change, change is made, approved and don’t revisit
  • The rules are visible: Rulesets · foreman-packaging-ruleset-test · GitHub allows people to see them

As people can see, there are 2 rulesets I made: one base that applies to all branches we use (I purposely excluded the bump_{deb,rpm}/* branches). Then a specific one for RPM that mandates CI (which is probably different for deb).

Note that there are bypass rules set so repository maintainers are allowed to ignore the rules. We’ll have to see how this works in practice, but cases I can see are:

  • Repoclosure is sometimes known to fail, but I wouldn’t trust everyone to be able to judge that.
  • I restricted creation of branches, but release owners will need to create those branches at some point. They’ll need to be able to bypass that. We probably need a specific ruleset for that.

Overall I think we should go with rulesets.

1 Like
10

I love the idea; I feel like I’m just annoying when I ping someone about the merge. The Rocket team has quite a few plugins, so having permission for this would save us a lot of time.

Two questions:
Do you want a list of plugins and people that will be the adepts?

Will the people also have permission to merge even when some CI jobs are red? I wouldn’t dare to merge packaging without green CI, as I have zero idea what’s going on, so having this green condition might be a good thing.

11

Ideally, this should be team based (= GitHub Team!), so for you probably @theforeman/discovery, @theforeman/bootdisk etc.

No, we’d enforce “required checks” on that and wouldn’t allow merging without those passing.

2 Likes