Ubuntu git branch reviewers are not set usefully

[Christian Ehrhardt ]

Hi,
I was unsure at first but now have seen it multiple times so I think it might be a real issue.
Probably due to the new repository location.

In the past we did uploads like:
1. open MR
2. set needs review
x. review iteration
3. mark it "approved" to be clear on our team overview to not need more work
4. upload with tag
5. once completed the importer will set it to merged

There could be quite some time between 3->5 and the approved state did help to keep our overviews clear.

Now we seem to be unable to set the "approved" state
1. open MR
2. set needs review
x. review iteration
>> unable to mark it "approved"
4. upload with prepare-upload
5. once completed the importer will set it to merged

An example (until it is merged) is https://code.launchpad.net/~paelzer/ubuntu/+source/spice/+git/spice/+merge/411128

It would help if we would be able to get back to be able to set approved state.

[Christian Ehrhardt  (paelzer)]

Not sure, but I'd expect this is related to
https://lists.ubuntu.com/archives/ubuntu-devel/2021-October/041644.html

As I will have less authority over the new target I can't set "approved" anymore.
Is there a meaningful way to allow the set of "approved" state without allowing direct access to the new repositories?

[Robie Basak (racb)]

Launchpad has a "reviewer" that can be set per-repository. It defaults to the repository owner. I suspect this controls whether you can set a branch to Approved or not.

It might be possible for git-ubuntu to always set this to some team. But which team? The repository reviewer is something that should be set correctly for a package as it applies Ubuntu-wide, so can't just be something server-specific for server team workflow. Since it's repository-wide, using core-dev vs. motu won't work very well as packages move between archive components.

There was some scope in Launchpad's git design to allow for a "person who can upload this package" type ACL entity, although this hasn't been implemented. Maybe the same needs to be done for repository reviewer.

In the meantime, perhaps core-dev might work to at least reduce the pain for those who are core devs, even if that is a blunt instrument.

[Colin Watson (cjwatson)]

Launchpad has an `isPersonTrustedReviewer` method on repositories, which I think could be extended to allow for this sort of dynamic reviewer situation without too much trouble.

In fact, the method is defined on `GitRef` as well (although it currently just proxies through to the method on the containing repository), so if we could work out a way to define the mapping between ref names and series/pockets, there's at least theoretically scope for making it depend on per-suite upload permissions.

[Christian Ehrhardt  (paelzer)]

I think we have settled on a the new mode and it works fine.
The importer correctly closes things recently and the MP owner can still control MRs AFAICS lately.
If no one else spots a remaining issue I think we can consider this done as of today.