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.

6 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
12

So, 3 weeks passed and so far I’ve only seen overall positive responses and a couple of implementation questions. Yay :tada:

I would propose we continue with an actual live test using rulesets.

Based on Ewouds testing, I think the following rulesets make sense:

  1. Base
    • Bypass list: “Maintain role” (= packaging, release engineers)
    • Target branches: all branches, exclude bump_rpm/*, bump_deb/*
    • Rules:
      • Restrict creations
      • Restrict updates
      • Restrict deletions
      • Require linear history
      • Require a pull request before merging:
        • Required approvals: 1
        • Require review from Code Owners
      • Block force pushes
  2. RPM
    • Bypass list: “Maintain role” (= packaging, release engineers)
    • Target branches: rpm/*
    • Rules:
      • Require status checks to pass
        • Status checks that are required:
          • rpm-copr
  3. DEB
    • Bypass list: “Maintain role” (= packaging, release engineers)
    • Target branches: deb/*
    • Rules:
      • Require status checks to pass
        • Status checks that are required:
          • deb

And then adding the following CODEOWNERS:

  • master:
    * @theforeman/packaging
  • rpm/develop:
    * @theforeman/packaging
/packages/plugins/*remote_execution*/ @theforeman/packaging @theforeman/remote-execution
/packages/plugins/*discovery/ @theforeman/packaging @theforeman/discovery
/packages/plugins/*bootdisk/ @theforeman/packaging @theforeman/bootdisk
  • deb/develop:
    * @theforeman/packaging
/plugins/*remote*execution*/ @theforeman/packaging @theforeman/remote-execution
/dependencies/*remote_execution*/ @theforeman/packaging @theforeman/remote-execution
/plugins/*discovery/ @theforeman/packaging @theforeman/discovery
/dependencies/*discovery/ @theforeman/packaging @theforeman/discovery
/plugins/*bootdisk/ @theforeman/packaging @theforeman/bootdisk
/dependencies/*bootdisk/ @theforeman/packaging @theforeman/bootdisk

WDYT?

Once this has been verified to be working I’d push the same CODEOWNERS definitions to the currently supported rpm/* and deb/* branches and welcome more teams to join the list.

I thought about that, but our branching process says the release engineer is branching packaging, and those are already on the bypass list via the “maintain” role.

13

I just saw this thread and I really like the idea as well - I think this would ease life of many, many people.

Can we also get something like a specific group for merge rights for ATIX-maintained plugins for develop and stable branches? We do have a couple of plugins and periodically open PRs for them, so I think it would be worth having this.

14

You mean for things like scc manager etc? Sure, we can define teams even for things that are not in our Org and give them the correct rights.

15

You mean for things like scc manager etc? Sure, we can define teams even for things that are not in our Org and give them the correct rights.

That would be amazing. Which information do you need for that? A list of plugins?

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.

Totally agree to that. I feel bad every time I need to ping someone because I want to get a package in.

16

Looking at ATIX AG · GitHub I see

  • acd
  • scc_manager
  • resource_quota
  • cve_scanner
  • snapshot_management

For each of those a list of github usernames that should be listed as “maintainers”

17

There is also

  • hammer-cli-foreman-scc-manager

For each of those a list of github usernames that should be listed as “maintainers”

Ok, we’ll get one :slight_smile:

18

Sure, but I’d guess the same people as for the main scc_manager should be responsible here?

19

Sure, but I’d guess the same people as for the main scc_manager should be responsible here?

Sure. We would propose having a github group with a couple of Atix people who would be responsible for the packaging stuff for all the Atix plugins (as the number is limited). Would that work for you?

20

Sure. I am cool with ATIX people breaking ATIX plugins :slight_smile:

1 Like