Subsystem maintainer permissions on gitlab

Huge +1. It’s a major bottleneck that only full committers can handle this meta / admin work. I’d be thrilled to offload some of that effort as a subsystem maintainer. I was getting ready to try to make enough sense of the Gitlab codebase to try to open an MR upstream to support more granular perms to solve this. 😅 but using the protected branches feature seems like a fabulous work-around.

So long as the main branches aren't modifiable by subsystem maintainers, and so long as committers still have the ability to do everything we currently can to those main branches, we desperately want them to be able to close and retarget MRs. It's a good suggestion.

Allowing subsystem maintainers to alter stuff for MRs in issue forks seems inline with the current definition of the "maintain issues" permission, so it seems like just applying the existing governance to a new scenario, rather than a governance change.

+1

I'll be bold and move this to RTBC. Per @xjm in #3, this isn't a governance change. It's a clever work-around for a limitation in GitLab that will allow us to restore the existing permissions and workflow for subsystem maintainers. I don't believe this requires any infra team folks. Just someone that already has super-powers on the core repo on GitLab to reconfigure some things. That'd be the existing full committers, which are the folks that monitor the RTBC queue...

Thanks!
-Derek

I have had a look at protected branches but don't fully understand the implications of what might happen if I make changes to the repository settings. I think we should ask @drumm or someone else with GitLab admin privileges to at least help us plan out exactly what changes we should make here.

Currently the subsystem maintainers (alongside with other maintainers) are assigned primarily in project settings on d.o. Do we need to update the d.o. Gitlab integration so that we would not need to manage maintainers on two places (at least until the issues are migrated to Gitlab)?

I also support this change and ensuring "the main branches aren't modifiable by subsystem maintainers' as xjm stated.

@longwave: Fair enough. Some things to consider before/during that meeting:

1. Do all existing core committers have 'Maintainer' access on GitLab, or is anyone only a 'Developer'?
2. Should we configure the core GitLab project to protect all branches by default? Seems like yes.
3. If so, we could add a wildcard * rule. See attached screenshot (from a personal GitLab repo, but it's probably the same for the core repo).

Thanks,
-Derek

Re: #10.1: I just noticed this from reading #3227737: [Meta] GitLab Acceleration Initiative → :

We actually grant every user Developer access to every project - so that there isn't a manual step where a maintainer must grant that access - however, to prevent certain potentially destructive functions from being activated, we're blocking the urls to those configuration pages in GitLab

Is that still true? Even of the core repo? If so, I'm not sure how this proposal will work.

Can we convert all the core committers to 'owner' status, convert subsys maintainers to 'maintainer', leave everyone else 'developer', and reconfigure all the branch protections so that only owners can push and merge?

If that doesn't work, could we use the code owners GitLab feature, add a .gitlab/CODEOWNERS file that wildcards the entire repository to be owned by a group, and use a GitLab group to manage the core committers. That group might already exist. I thought I saw a way to give permission to push to a branch only if you were listed in CODEOWNERS, but I'm not finding it now. Is that feature only for approving MRs before they can be merged?

If it's only for approving MRs, perhaps we should open a child issue to explore mapping MAINTAINERS.txt to CODEOWNERS in some automated way, or converting over the CODEOWNERS entirely...

Yes please to this ticket :) Have seen several tickets recently with 4+ MRs open and all I can do is mark in the ticket which MR should be used.

I have had a look at protected branches but don't fully understand the implications of what might happen if I make changes to the repository settings. I think we should ask @drumm or someone else with GitLab admin privileges to at least help us plan out exactly what changes we should make here.

I’m not aware of any gotchas, I expect GitLab’s settings to do what they say. That said, I do not have much personal experience with these settings. You can always back out changes if they do something unexpected. The baseline Drupal.org restrictions, like no force pushes to branches with dev releases, are done with system-level hooks, so they can’t be bypassed regardless of any repository-specific configuration.

Currently the subsystem maintainers (alongside with other maintainers) are assigned primarily in project settings on d.o. Do we need to update the d.o. Gitlab integration so that we would not need to manage maintainers on two places (at least until the issues are migrated to Gitlab)?

Maintainers will be managed in both places indefinitely, or at least will get revisited after issues have moved. Edit project and Administer releases still directly apply to Drupal.org-only; it’s possible we make GitLab the source of truth and map those from GitLab. There is not a practical way to prevent project maintainers in GitLab from adding/modifying maintainers in GitLab.

Can we convert all the core committers to 'owner' status, convert subsys maintainers to 'maintainer', leave everyone else 'developer', and reconfigure all the branch protections so that only owners can push and merge?

GitLab’s project owner access is not something we can give out to anyone. It allows highly destructive actions like renaming or deleting the project.

GitLab’s project maintainer access does also allow adding & updating other maintainers in GitLab.

Going to take a chance and mark this, think the ability to close out of date MRs for a ticket will help.

But did notice the other day the ability to hide branches on a ticket. Leaves a lot of dangling branches out there but least it helps make the tickets clear.

There was a discussion on Slack, pasting a link here: https://drupal.slack.com/archives/CGKLP028K/p1701285278934459

This comment from @longwave is worth considering:

Developer can also add/rewrite/remove tags so we would need to protect those too

This will also grant provisional committers maintainer role and therefore an option to update maintainers in GitLab. Not saying it is a big issue, but I am picking it up here too:

GitLab’s project maintainer access does also allow adding & updating other maintainers in GitLab.

We should figure out the tags issue before going ahead - not because I don't trust people but because it's pretty easy to push changes to the wrong remote - I've pushed a couple of MR branches to origin before (fortunately the least destructive bad push, but still).

This will also grant provisional committers maintainer role and therefore an option to update maintainers in GitLab.

This sounds fine and possibly a feature overall.

GitLab’s project owner access is not something we can give out to anyone. It allows highly destructive actions like renaming or deleting the project.

@drumm just to confirm does this mean it is OK for all core committers to have or not at all?

Keynotes regarding provisional maintainers from the Slack thread(so it doesn’t derail this)

1. They should of already been vetted
2. They can be granted commit access while still in the developer role (where they can not add more users) nullifying the concern.

And yes protected tags is a feature too:

https://docs.gitlab.com/ee/user/project/protected_tags.html

@catch my reading (and understanding) is no one anywhere on G.D.O. (Except the super global GDO admins) has or will ever have the Owner role.

The issue summary should say which roles that committers and subsystem maintainers will end up with on gitlab, and whether branches and tags have been set to protected, or if not when they will be.

@drumm just to confirm does this mean it is OK for all core committers to have or not at all?

No one will be able to have owner access level.

Also noting from the Slack thread: we can test out this proposal with a non-core repo to make sure it's all working as expected. I'd be happy to do so on some of the projects I maintain, just need someone(s) else who wants to test with me. Unless anyone else already did the testing and can report their findings here.

Thanks,
-Derek

I can help dww. I was planning the same thing, but would've probably opted to make a temporary mirror of core and use some real people there. This would also help vor maintainers to check if they see weirdness.

Will this update also allow submaintainers close threads. Currently core committers could get an MR with 20+ open threads because no one can close them :(

Will this update also allow submaintainers close threads.

Yes.

Just following up on this one if there are next steps

I think this was discussed approx. month ago on Slack (Gitlab initiative) and the outcome was that it would be the best if we can test it + document. See: https://drupal.slack.com/archives/CGKLP028K/p1706272560536979?thread_ts=...

This was discussed and tested via Slack last month:
https://drupal.slack.com/archives/CGKLP028K/p1713382030886629

Attached instructions/procedures guide that was linked in that thread (no modifications since initially created).

Think his covers the issue summary changes so removing tag.

This has been extensively tested by now. The attached document is a great manual on how to setup.

It might need to be documenten on d.o. somewhere. So we have the setup documented.

I think all issues have been resolved now and it's time to setup. If we are a little scared, start with a small group.

I think for the actual introduction I'd personally choose to look at active subsystem maintainers from the last year and add those first. Although that might end up making it a mess.

Anyways, since everything is addressed I'm settiing this issue rtbc. Thank you to everyone working hard to making this happen. <3

PDF is really clear, nice job.

Of note regarding:

Provisional committers will be GitLab Developers with an explicit access permission to push (or if the Core team wants a more lax security policy they can skip this and just make them regular maintainers)

This might depend on the committer's role, also (a lot of our other permissions do for provisional committers), but based on the summary I think the former option is the closest match to the current r ole expectations for provisional committers → .

The PDF is a great help; thank you for that. We will need documentation updates based on this when we implement this practice.

I'm completely on board with the proposal here, but since it is a policy issue essentially about the implementation of core governance, we should probably follow the committer decision-making process to give all committers a chance to review the policy. So, I'm tagging with all the relevant role tags, and posting this to the committer's policy decision process.

Thanks everyone!

Saving credits for policy discussion and research.

@quietone raised that this would exacerbate an existing problem we have, in that our list of active subsystem maintainers is not well-maintained and we have a lot of people who have elevated permissions for Drupal core but who are not actually active as maintainers.

This is probably not blocking as it is just an extension of a problem we already have, but it does mean that it would be good to make sure that the granting and revocation of this access is automated, if possible (maybe tied to the "maintain issues" permission on the project node).

In the end we need a source of truth. I'd opt for gitlab for that eventually I think. But a maintainers.txt is also usefull for history.

Thanks for adding the related issue, that is usefull.

At some point we should also add codeowners possibly. For the different parts of Drupal I think. But that one for later, and very out of sco0e of this issue.

it would be good to make sure that the granting and revocation of this access is automated,

Once we learn to embrace GitLab most of this becomes much easier.

GitLab has well defined API's available, including user management API's. I would not expect it to be hard to write a script that looks at activity of all the users with privileges and downgrades those that have been inactive for a period of time to Reporters or less (reporters still have access to issues marked /confidential and as such may still be too high a privilege long term) .

Equally I would expect the steps in the user guide could be scripted after the policy is adopted.

Opened ✨ Sync (or otherwise de-duplicate) project maintainer permissions with gitlab Postponed for the de-duplication of role management between gitlab and Drupal.org

Last remaining things here I think are:

1. We actually need to grant the roles, I think that can be done before or after this being marked fixed.

2. We should document the process somewhere other than a PDF in this issue, maybe copy it to somewhere in https://www.drupal.org/about/core/policies/maintainers → ?

Removing the 'needs release manager review' tag'.

Removing the other tags too because this has been discussed between core committers and apart from the follow-up to make the permissions management more straightforward, it's been approved.

There was a question from the core meeting in August asking if the relationship between GitLab and d.o for permissions could be automated and make the management of this easier and less prone to errors. Is that possible and worth the effort in the expected remaining time to the full move of issues to GitLab?

The only planned changes around issue migration are #3262293: Grant GitLab “reporter” access to project maintainers with “edit project” or “maintain issues” → and taking advantage of GitLab’s growing support for custom roles. We can always make more changes at any time; permission changes are often a separate concern from the actual migration.

There is already some syncing back changes from GitLab:

• If you leave the project or remove a maintainer in GitLab, they are removed from Drupal.org.
• If you add a maintainer in GitLab, they are added with write to vcs access.

So everything is synced bi-directionally, except that Drupal.org does not have a place to store the GitLab maintainer vs. developer distinction.

Shall we move the documentation from the pdf in #26 to Core committer handbook → ?

The intial setup from the PDF needs to be done, make the branches and tags protected (wildcards) and give maintainers the permission to allow to create.

This needs to be done by someone who has those permissions in the Drupal project. I'm more than willing to create the guide then for adding and managing the subsytem maintainers.

We are all agreed this is a good idea, so all is fine, and as soon as we fix this we can start helping out with retargeting branches etc, taking off load from the committers.

(unhappy i forgot this issue during barcelona, would've just pulled someone to do it :P)

Anniversary today, issue opened 1 year and 19 hours ago (it was originally suggested on Slack quite some time before that).

To clarify #41 the following users have the necessary permissions in GitLab to make the changes outlined. This could be delegated to any one of these individuals.

@effulgentsia, @alexpott, @bnjmnm, @catch, @ckrina, @longwave, @mcruid, @goba, @xjm, @poker10, @laurii, @larowlan, @quietone, @nod_, @dries, @fabianx, @yoroy, @justafish.

I've just done this - I used the d.o interface approach so that everything stays in sync and for transparency on the d.o side.

Agreed with @quietone that the PDF should be moved to https://www.drupal.org/about/core/policies/maintainers → so marking needs work for that.

Would be great if a subsystem maintainer could try closing a thread on an MR they previously couldn't. And also someone should probably try a (harmless) test push to the 11.x branch just to make sure it really is still locked down.

That is awesome! Thank you! I will make some time, hopefully later today to have a look to this and see if this works as intended.

Step 1: tried retargeted a random MR successfully - https://git.drupalcode.org/project/drupal/-/merge_requests/5271

Step 2: tried pushing to 11.x and got a permission denied.

Step 3: tried resolving a random thread (and unresolving) which worked in this mr https://git.drupalcode.org/project/drupal/-/merge_requests/1036#note_40479

So seems to work as it should yay!

Next up is getting the docs into d.o.

The source document is here so we can copy paste

https://docs.google.com/document/d/1OcYqPrtkQDlFXQs6-xSbcCsEK85In5JSxEUW...

1. Clicked on the MR posted by Björn and could edit it. Didn't click save, though.
2. Tried to push to 11.x (that was scary!) and got the following: "remote: GitLab: You are not allowed to push code to protected branches on this project."
3. Could also resolve and unresolve threads on the other link Björn posted

So can confirm @bbrala's findings :)

Thanks for testing! It was also scary giving everyone 'write to VCS' permissions all at the same time, considering I've accidentally pushed development branches to the core repo at least a handful of times.

Is there anything else you'd like one of us (subsystem maintainers) to confirm? I can run some more tests this week if you want.

The things you both tested are all I can think of. Also @longwave did a release today which means we didn't lock ourselves out of tagging.

This triggered a GitLab integrity check fail, since Drupal.org didn’t fully process all the webhooks sent for adding maintainers.

To start correcting this, I saved maintainers at https://www.drupal.org/node/3060/maintainers → , not realizing it would reverse any of the maintainership that was not synced.

I will be correcting this now.

I believe this is corrected now. I’ve spot checked and everything looks better. I’ll continue verifying in more detail. Sorry for the disruption.

Hehe no worries drumm. :) New territory here

Not a big deal but think I still notice this random problem. If an MR is opened by a core committer then no one but a core committer can run the test-only feature.

I wonder if core comitters have to do something in the interface to allow all maintainers to edit or something. There are some settings around allowing edits, might be something that has a secure default.

If an MR is opened by a core committer then no one but a core committer can run the test-only feature

https://drupal.slack.com/archives/CGKLP028K/p1713458778208449?thread_ts=...

TLDR: we tested and confirmed they would be the case even with permissions added.

interface to allow all maintainers to edit or something

GitLab uses the rule on if a maintainer has access to commit code.

ci_allow_fork_pipelines_to_run_in_parent_project = false might solve this.
I’m not sure if it would cause pipelines to fail (GitLab UI tries to create the pipeline in the parent project and rejects) or if it coerces jobs back to the issue fork (in which case all you need to do is get push access to the fork).

Ref: https://docs.gitlab.com/ee/ci/pipelines/merge_request_pipelines.html#pre...

This all boils down to a simple “pipelines run by maintainers execute in the protected core project”. The project the pipeline runs in is determined at pipeline creation and stays with the pipeline and is not impacted by trying to run a manual job later. At the point your requesting to run the test job you do not have commit access to the protected branch and thus do not have the permission to run the manual job.

This could probaly be worked on as a follow-up discussion as at least one minor negative I can think of exists although it could be worked around.

Either way as noted in the above Slack thread this situation is still progress!

@drumm I checked all the checkboxes for all subsystem maintainers at once and submitted, that might have broken the sync queues, apologies.

I'll start work on the document in a couple of hours. It is time for a long break and some food.

The doc page is here, https://www.drupal.org/about/core/policies/maintainers/subsystem-maintainer → . What it needs most right now is the images uploaded.

I've uploaded all the images and made sure the structure (with step a and b) is actually readable.

I'd also add links, but I cannot since the settings pages are not available to my role. I think this is pretty good, although it good be better, I'd still say docs are updates.

One thing we might need to do is make a blogpost regarding this change, and perhaps have a better place to ask for retargeting and such. If we for example have a channel to ask for retargeting of issues we could check there and update where needed, streamlining the process.

Fun fact.

Issues now show the merge button in the D.O. interface. I dont dare to try it...

Issues now show the merge button in the D.O. interface. I dont dare to try it...

We use GitLab’s impersonation API to perform the merge as the maintainer clicking the button, so the MR is indeed merged by the maintainer. What’s missing is checking for access to the MR’s target branch, instead of general commit access which must not consider protected branches.

So this should be safe, and GitLab will deny actually merging.

I have been able to close merge requests from others which hopefully means can help the committers too when things get committed

This is working for me!

@bbrala, sorry but I see no images on the document. Shouldn't the images be on d.o and not lh7-rt.googleusercontent.com?

;(

Darn. Did not check after doing the whole thing in the editor.

If noonen does I'll Retro tomorrow. Did copy paste.

Only local images are allowed AFAIK

On it

Done, only thing missing is alt texts for the images.

Should the older <= D10.0 branches be protected as well?

While they are an older release series the branches have similar risk as the currently supported branches.

I think * is protected right? So all branches should be protected if understand correctly.

As a common user I can not see the actual config.

The GitLab UI was showing the “protected” icon only on the latest branches before I posted.

Now all but the 10.1 branch appear to have the flag and when I look on the branches page it appears to show 10.1 as protected while the drop-down does not.

Possible false alarm by a cached response. Realizing I also was not logged in when looking.

I think * is protected right? So all branches should be protected if understand correctly

That's how I understood it. Apart from protecting older branches, saves a manual step when we create new branches every six months.

I think that must just be a caching issue in the UI, I can see that * is protected and all branches have the protected label for me.

Curious what's left to review on this since it seems things have been implemented?

I think that the new documentation page: https://www.drupal.org/about/core/policies/maintainers/subsystem-maintainer → (see #43 and #62).

Yeah alt texts would be good to add. Think we should be all ready then

Believe we are good for this one now. Correct me if I'm wrong.