If the project has the pagure hook enabled (no idea if that's the case), then a possible way around this would be to wget the patch of the PR: wget https://git.centos.org/rpms/systemd/pull-request/1.patch apply it manually via git am and then edit the commit message via git commit --amend to add Merges https://git.centos.org/rpms/systemd/pull-request/1 before pushing.

The Merge https://git.centos.org/rpms/systemd/pull-request/1 will tell the pagure hook to close the PR as merged, but for this the pagure hook needs to be enabled...

as discussed in stand up, this will have to wait for a few days till Fabian can take a look (or someone else with Access)

@dcavalca the pagure-dist-git ACL in place allows RWC for people of a group to directly push to a branch matching SIG membership.
It seems (nobody ever requested that feature) that it doesn't allow someone to also merge a PR against a branch one has RWC access to.

@pingou : do you think that a RFE against pagure.io/pagure is the way to go to allow this ? or instead the method you described above (more complex though) and if so let's see what needs to be implemented ?

@pingou : do you think that a RFE against pagure.io/pagure is the way to go to allow this ? or instead the method you described above (more complex though) and if so let's see what needs to be implemented ?

Pushing this as it is now to pagure upstream would mean teaching pagure about
something it does not/cannot know about (the rules of pagure-dist-git).
One way around could be to add group as contributors with a regex, duplicating
things in pagure and pagure-dist-git, but allowing them to merge in certain
branches, but even this isn't supported upstream (yet).
Though this last option (the possibility for collaborators to merge PR into
branches they have access to) would be something that we will need to be
implemented upstream at one point.

@pingou thanks for the clarification so not something that can be implemented right now.
Do you advice SIGs to use another git repo somewhere else (like github ?) for the PR/review and once merged there, they'd push to corresponding branch on git.centos.org ?

@pingou thanks for the clarification so not something that can be implemented right now.
Do you advice SIGs to use another git repo somewhere else (like github ?) for the PR/review and once merged there, they'd push to corresponding branch on git.centos.org ?

If we could enable the pagure hook there, SIG could manually apply the patch add
the Merges <url> in the commit message and push. That would make the PR appear
as merged on pagure's side, and while not as easy as a one-button action, it's
pretty straight forward.

Ok, by default it's not enabled but I just enabled it (but haven't tested and ideally we should have tested it on git.stg.centos.org first)
@dcavalca : with the information that @pingou provided, do you think that it's possible to use it ?

Yeah, having to merge stuff manually is cumbersome, but I think we can make it work for now. I'll report back after we've tried this out. Thanks!

Is there anything stopping us from just setting a default branch on all the repos? I would think that would allow the URL to load, and then the submitter can use the normal drop down menu to set the target branch.

@carlgeorge that would solve the first issue about PR (and pagure not being confused) but not the other issue about allowing PR to be merged on specific branch[es] through pagure-dist-git ACL check.

What would be the good "default" branch to update everywhere btw ? c7 ? c8 ? (and hopefully there is a simple way to do that through pagure api calls directly and also to specify that at creation time, so something to ask Red Hat RCM to do when they create a new project in the rpms namespace)

c8s seems like the right choice for a default branch, as it's the latest. I don't know what API calls are available for setting that.

Just having a look at opened tickets and was wondering if this one can now be closed ?

Did we sort out the default branch as @carlwgeorge suggested?

the default branch for systemd was manually set to c8s through pagure web ui.
If we want to do this for all project in the rpms namespace we'll have to find a way : I just had a quick look at pagure api (https://git.centos.org/api/0/) but I don't see a way there to update through api request.
@pingou : can you confirm ?

If not, I guess we can still kick a .sql query directly into the pagure DB, but each time a new project/rpm will be created by Red Hat RCM, it would need to be done for it too

the default branch for systemd was manually set to c8s through pagure web ui.
If we want to do this for all project in the rpms namespace we'll have to find a way : I just had a quick look at pagure api (https://git.centos.org/api/0/) but I don't see a way there to update through api request.
@pingou : can you confirm ?

It is doable in newer version of pagure

If not, I guess we can still kick a .sql query directly into the pagure DB, but each time a new project/rpm will be created by Red Hat RCM, it would need to be done for it too

This is not a setting that is stored in the database but read from the git repo
directly

Between other tasks, I had a quick look and ran a massive update on all projects in rpms namespace (after having tested on git.stg.centos.org of course).
Applied setting: if c8s branch, exists, make it the default one, if not, checking for c8, and last c7 (priority order)
That should normally solve the PR issue checking for MASTER branch that doesn't exist (should be checking for refs/heads/$branch correctly now

Unfortunately that will probably have to be done again, if Red Hat is creating and push new projects for rpms in the future, so let's have a look.

@dcavalca : does it look better for you now ?

Setting the default branch seems to have worked, but I see bugs. Here's what I tried:

git clone https://git.centos.org/rpms/unbound.git
cd unbound
git remote add malmond ssh://git@git.centos.org/forks/malmond/rpms/unbound.git (I had clicked fork a while back)
git checkout -b simple_bump_1.13.1
git am ~/0001-Simple-bump-of-unbound-to-1.13.1.patch
git push malmond simple_bump_1.13.1 --force

I had to force because I've tried this over a few times. I don't think that's an issue. What I got was:

[malmond@malmond-x1 unbound]$ git push malmond simple_bump_1.13.1 --force
Enumerating objects: 13, done.
Counting objects: 100% (13/13), done.
Delta compression using up to 8 threads
Compressing objects: 100% (5/5), done.
Writing objects: 100% (7/7), 630 bytes | 315.00 KiB/s, done.
Total 7 (delta 2), reused 0 (delta 0), pack-reused 0
remote: Sending to redis to send commit notification emails
remote: * Publishing information for 1 commits
remote:   - to mqtt
remote:
remote: Create a pull-request for simple_bump_1.13.1
remote:    https://git.centos.org/fork/malmond/rpms/unbound/diff/None..simple_bump_1.13.1
remote:
To ssh://git.centos.org/forks/malmond/rpms/unbound.git
 + d382901...868a785 simple_bump_1.13.1 -> simple_bump_1.13.1 (forced update)

Issue 1: The left side of the PR is "None" which suggest a Python based problem
Issue 2: The same action in the Web UI: https://git.centos.org/fork/malmond/rpms/unbound/branches has a button for "Open Pull Request" for the branch, and it links to https://git.centos.org/fork/malmond/rpms/unbound/diff/master..simple_bump_1.13.1 which references a missing master. This should have default to c8s based on what I did.

Ok I just tried the manual merge flow and it doesn't work:

• @malmond put up https://git.centos.org/rpms/unbound/pull-request/1
• I ran wget -qO- https://git.centos.org/rpms/unbound/pull-request/1.patch | git am to apply it to the branch
• I then ran GIT_EDITOR="echo 'Merges https://git.centos.org/rpms/unbound/pull-request/1' >> $1" git commit --amend to add the magic string as suggested by @pingou
• Finally, I pushed it to https://git.centos.org/rpms/unbound/c/52b687556a16d119ce10ae220cdc07d110793d27?branch=c8s-sig-hyperscale-experimental
• The PR remains open and seems unaware of the commit :-(

something to discuss through a irc meeting, but I'm afraid that there is currently no easy way to have PR workflow working for SIGs, due to branch protection.
Worth knowing that other SIGs do their review/PR outside of git.centos.org, and then push back to the sig branch on git.centos.org (some like RDO/Cloud sig use their own gerrit review process)

Again, not something that can be easily fixed through a discussion on this ticket. And with Stream 9 itself being hosted on gitlab, where SIG will not be able to push on branches (afaik), that will be another discussion too

Worth debating through a SIG/CBS meeting one day

Sure, happy to discuss this in a meeting, just let me know. It'd be great to better understand what the blockers are so we can figure out a plan of attack (especially in the light of c9s and gitlab coming into the picture), and hopefully find a way that we can help out with implementation and/or resourcing.

If the issue is branch protection, one workaround could be to have a bot watch PRs and use a magic comment (from a list of approved authors) as trigger to automatically merge them to the right branch.

@dcavalca : reading open issues on tracker and having a look at this one. Tempted to closed as it's a feature that doesn't exist (yet ?) in pagure. So if you think that it's possible to add it upstream, see with @ngompa so that it can land in upstream pagure first and then we can see how to implement it ?

@arrfab I'm pretty sure this is already sorted out upstream in Pagure, and it should be solvable by updating the instance that powers git.centos.org to the latest version. cc @bstinson as we've discussed this a bunch of times during office hours as well.

@arrfab, I'm not sure what feature you think is missing in Pagure? Pagure supports branch protection through the dist-git auth plugin and certainly supports per-branch ACLs and PRs where merging is permitted to branch owners. It exists on src.fedoraproject.org, but it requires the latest version of Pagure to do it. We added those features in Pagure 5.12, I believe.

So getting Pagure upgraded and on CentOS 8 would allow us to resolve this issue.

@ngompa if you confirm that it's possible for pagure, through dist-git auth, to allow a non admin/maintainer of a project to still merge in a branch he has RWC rights on (computed 'on the fly' by pagure-dist-git), that's so something we can then investigate.
We first need (time permitting) to rebuild pagure and friends for 8-stream through infra tags on cbs.centos.org and give it a try on a dev and then stg instance. Once validated we can then eventually work on the reinstall but the open question about pagure vs gitlab is still a thing and I don't have the answer (don 't know if anybody has at this stage btw)

@ngompa if you confirm that it's possible for pagure, through dist-git auth, to allow a non admin/maintainer of a project to still merge in a branch he has RWC rights on (computed 'on the fly' by pagure-dist-git), that's so something we can then investigate.

My understanding is this should work with pagure 5.12+ and pagure-dist-git 1.10+ since @pingou added support for it: https://pagure.io/pagure-dist-git/c/2e491c9613b392234f900a2f721ee3bf2d53165c

So it should work with the latest versions of pagure and pagure-dist-git.

[backlog refinement]: Still blocked by #456

[backlog refinement]: Still blocked by #456

[backlog refinement]: Still blocked by #456

now that #456 is closed (and git.centos.org upgraded to pagure 5.13) I wanted to review this original request.
Actually, pagure itself still lacks the collaborator right (see https://pagure.io/pagure/issue/5297) but at least granting commiter right would permit to merge as the underlying pagure-dist-git would verify if you're allowed to do it or not (so not blocked at pagure web ui level but rather pagure-worker)

Wondering how we can proceed with this easily as it would need to update projects through pagure api to allow groups to be listed as commiter.

We can start with some tickets in infra but it would be good to eventually start working on automation around this : a git repo with a .yaml files about projects to create and which group to grant commit right and so on approved PR, it would reflect through pagure api the projects and rights.

@dcavalca : do you think we can now close this request ? for projects you'd want to have commiterrights, you can create ticket and then it would permit you to merge PR (as you tested on git.stg.centos.org).

Sounds good, I've filed #827 to get the appropriate permissions. Thanks!