Talk:Gerrit/Privilege policy

Jump to navigation Jump to search

About this board

Clarify reference to "Gerrit Administrators"

5
DKinzler (WMF) (talkcontribs)

The policy currently refers to "Gerrit Administrators" as being responsible or empowered to perform certain actions by this policy. It would however be more useful to associate these powers and responsibilities with gerrit permissions rather than a specific group, allowing the granularity of groups and permissions on gerrit to be improved as discussed on phab:T219012#5057232.

I propose to replace all references to "Gerrit Administrators" with "people with the necessary permissions on gerrit", (or an equivalent but less clunky phrase).

Hashar (talkcontribs)

On one side there is the administration of the Gerrit software itself. That is the Administrators built-in group in Gerrit which is https://gerrit.wikimedia.org/r/#/admin/groups/1,members This is the equivalent of having read/write access everywhere (disk or database).

On the other hand, the Gerrit Managers group https://gerrit.wikimedia.org/r/#/admin/groups/119,members is for administration of policy / access lists / repositories etc. It is less privileged than the administrators of the software itself, but is still a very large set of permissions. The group was originally named Project and Group Managers but got renamed to Gerrit Managers in Feb 2018 by https://gerrit.wikimedia.org/r/plugins/gitiles/All-Projects/+/10107cf5056eb6ef3903f49d59cd27387831c5b5%5E%21/#F0 It is semantically broader and might be confusing :D

Jdforrester (WMF) (talkcontribs)

I think changing it to "Gerrit Managers" would be the most reasonable change.

DKinzler (WMF) (talkcontribs)

> I think changing it to "Gerrit Managers" would be the most reasonable change.

It sounds reasonable, but in T219012 there is talk of creating a mediawiki-administrators. And further groups may be created in the future, for other repos, or with different rights.

This is why I want to avoid any group names in the policy. The policy should apply to whoever has the respective rights.


Jdforrester (WMF) (talkcontribs)

Then call it "the 'Gerrit Managers' group of individuals, or whatever it's called this week". Let them bike-shed in peace.

Reply to "Clarify reference to "Gerrit Administrators""

Pair Programming Exception

4
Summary by DKinzler (WMF)

The fact that there were "four eyes" on the code should be documented on gerrit. That's the essential part.

TCipriani (WMF) (talkcontribs)

Under the bulleted exceptions to Merging without review, there is a point that "A commit in Gerrit may have two authors" mentioning "an owner and a reviewer who uploads and amendment"; however, there is no exception for pair programming explicitly. I am not sure how common this is for MediaWiki core and Extensions (cf: scope?), but it is common for several repos I work in.

That is, a patch could be written during a hangout/pairing session between two people, uploaded by one, and merged by the same. As a recommendation, it could contain a Co-Authored-By: snippet in the commit or two Signed-off-by: tags.

Jdforrester (WMF) (talkcontribs)

Isn't there a risk that pair-programmed code suffers from the same individual "group" think that we worry about for sole-author patches? Do we want to encourage the practice of pair-programmed code being merged by one of said pair?

TCipriani (WMF) (talkcontribs)

I think code review and pair programming both ensure that two people understand and endorse a particular patch. I view pair programming as a real-time review of code being written, whereas code review is asynchronous.

I think code review and pair programming are both equally subject to groupthink and/or power inequalities. I am unsure if one is more so than the other.

DKinzler (WMF) (talkcontribs)

I agree that pair programming is kind of a live review process. There is no good way to represent shared authorship on gerrit, so one person should upload the patch, and the other can approve it right away. MAybe it would be nice to mention the fact that the patch was done in a pair programming session, but I don't think it's necessary.

In any case, the fact that there were "four eyes" on the code should be documented on gerrit. That's the essential part.

Summary by DKinzler (WMF)

The scope of the policy extends over all repositories on WMF's gerrit instance. However, there are several bits that only apply to MediaWiki, or only to code deployed on the Wikimedia cluster.

Smalyshev (WMF) (talkcontribs)

The document talks mostly about Mediawiki deployment and Mediawiki extensions, but there are more projects in Gerrit than this. I think it should be clarified whether these policies are to apply to them too and then they need to be written in a bit more generic way or just to "big Mediawiki" (i.e. Mediawiki plus extensions plus puppet, etc.) but not other code hosted on WMF Gerrit.

Tim Starling (WMF) (talkcontribs)

It's intended to apply to all projects that are in WMF Gerrit. There are some bits that are fairly specific to MediaWiki, do you think they should be removed?

TCipriani (WMF) (talkcontribs)

FWIW, I initially had the same reaction, many of the arguments surrounding the importance of +2 make direct reference to the particular configuration of MediaWiki and extensions Gerrit and Jenkins setups.

Putting that aside, the section on merging without code review feels particularly prescriptive; however, it makes the point towards the end:

For extensions (and other projects) not deployed to the Wikimedia cluster, the code review policy is up to the maintainer or author of the extension.

I think I would prefer if that were easier to glean from an inspectional reading of the policy rather than a close reading.

Also, it's important to note that self-merge is common in some repositories (integration/config for instance), and there may be differences between projects in what exceptions there are to merging without code review; however, in these projects, +2 is still "a strong expression of trust", only the expectations are different.

Trusted organizations and the mediawiki group

5
Summary by DKinzler (WMF)

In effect, this means that WMDE staff can added to the mediawiki group per default at onboarding, but can also be removed from this group again without removing them from the WMDE group.

DKinzler (WMF) (talkcontribs)

The event that triggered the drafting of this policy was the request from Wikimedia Deutschland to have new hires be added to the mediawiki group immediately. According to this policy, that would not be the case: WMDE hires would still have to go through the request process to get +2 on core. Is this intentional? I seem to recall that there was agreement that it would be ok to have staff of trusted orgs get +2 per default. The alternative would be to include the wmde group in the mediawiki group, but I would advise against this - WMDE may want to grant access to their own stuff to a volunteer, without granting access to *everything*. Also, WMF may withhold access to core from specific people who work at WMDE, while still allowing them to be a member of the WMDE group, and have +2 on repos managed by WMDE.

Mobrovac-WMF (talkcontribs)

That's not how I read it. New WM Deutschland employees would be automatically added to the wmde group, which by my interpretation of this document, is assumed to automatically have +2 on the whole mediawiki/ tree.

Nikerabbit (talkcontribs)
Leszek Manicki (WMDE) (talkcontribs)

@Nikerabbit is right wmde LDAP group is not yet included under mediawiki project.

My understanding was, that based on the new policy, as a next wmde group will be added there. This is a technicality, which I am not sure has not be made more explicit in the policy.

Regarding one point from @DKinzler (WMF): wmde LDAP group should only be for WMDE staff IMO. Same as wmf LDAP group is in my understanding for WMF staff only (with "staff" I also mean active contractors etc). for possible volunteer access, some other groups would be used etc. But this again seems to me be a detail, which is not really in the scope of the policy.

DKinzler (WMF) (talkcontribs)

@Leszek Manicki (WMDE) originally, the idea was to make the WMDE group an "included" group of the MediaWiki group. However, this would make it impossible to have people in the WMDE group but not in the MediaWiki group. There have been cases in the past of WMDE staff being denied +2 on the mediawiki group, and that possibility should still exist.

Instead, the idea is to have a list of "associated" groups for trusted organizations, for which these organizations can request members to be added without the need for prior public discussion.

In effect, this means that WMDE staff can added to the mediawiki group per default at onboarding, but can also be removed from this group again without removing them from the WMDE group.

Automatic revocation for staff

4
Summary by DKinzler (WMF)

Automatic revocation at the end of employment protects the individual's privacy. No fault is implied.

Tgr (WMF) (talkcontribs)

It is WMF policy to revoke all privileges when staff members depart, even if those privileges were granted prior to the beginning of employment by virtue of volunteer work. Consistent application of this policy helps to protect the privacy of departing staff members: no fault is implied.

Automatically revoking privileges that were given as part of someone's job makes sense. Revoking privileges received as a volunteer seems pretty hostile and not a great way to encourage further volunteer contributions. I don't get the privacy reference at all. Is that about not disclosing the fact that someone might have been fired in a way that makes us not trust them with +2 any longer (and there might be legal restrictions on disclosing even that fact)?

Anomie (talkcontribs)
DKinzler (WMF) (talkcontribs)

> I don't get the privacy reference at all. Is that about not disclosing the fact that someone might have been fired in a way that makes us not trust them with +2 any longer (and there might be legal restrictions on disclosing even that fact)?

Yes, this is indeed the reason. I don't see a good way around this, especially since there is a lot of grey between "fired for bad faith behavior" and "quit because they found a nicer place".

Tgr (WMF) (talkcontribs)

Ask people to indicate whether they need the permissions in the future, let them keep it if they do, and suggest people leaving under a cloud that they should let them lapse and spare themselves the embarrassment of being specifically denied. Unless you are considering the situation where the person leaving is not trusted but unaware of it, but in that case you will probably have to deal with it anyway when they re-request them.

Phab projects for privilege requests

3
AKlapper (WMF) (talkcontribs)
Tim Starling (WMF) (talkcontribs)

We are proposing to rename "repository ownership" to "Gerrit privilege requests". This change in terminology has been made in the draft, and reflects a substantive policy change, so I think it should also be reflected in a change to the Phabricator project. Also, we are proposing to split off requests for access to the MediaWiki group into its own phabricator project, since that was requested in TechCom. Is that clear enough, or would you suggest a different scheme?

AKlapper (WMF) (talkcontribs)

Thanks, sounds good.

Nikerabbit (talkcontribs)

Three things, which are tangential to the actual point of this proposal, so feel free to ignore:

  • When picking up (stale) patches from others, I often seek at least +1 from someone else for my additional changes before I self-merge. Assuming this is acceptable, maybe it can be documented under self-review.
  • It would be kind for code bases with clear maintainers to give the maintainers a chance for review. I have seen patches created and merged by non-maintainers while maintainers were asleep. Sometimes these do have issues that could have been resolved before merging if the maintainers had the possibility to give a look at them. Can we encourage this in the policy?
  • A non-criticizing observation: due to these rules, I often ask others to create patches I could do quickly myself, because getting someone to review my patch would take even longer than having the other person figure out how to create the patch, and I sometimes basically tell how it needs to be written, which is actually a self-review if one thinks about it very strictly.
Tim Starling (WMF) (talkcontribs)

I said something about the first point, and I also renamed the section and changed some language to make it clear that the problem is merging changes that have not been reviewed by someone else, not "self-review", whatever that is. You might naively think that self-review means reading your own code before you upload it to Gerrit, which seems OK to me.

Anomie (talkcontribs)

I've also encountered all three of these situations, particularly the first and third. Along these lines, I'm reminded of a few more edge cases:

  • The "virtual +2": If I'd be willing to merge the patch from another +2er as-is, but I also want to leave a suggestion for minor improvement, I'll leave the comment and tell the author to feel free to self-merge the patch if they don't want to make further changes, instead of having to wait for me to come back to do it.
  • Transient Jenkins errors: I've sometimes self-merged a patch when a previous non-self +2 failed to merge because of some transient Jenkins error, for example when npm occasionally fails to download its packages.
    • I know I've been tempted to do the same if Jenkins failed only because of a conflict in the RELEASE-NOTES file, but I don't recall whether I've ever actually done it. I have sometimes +2ed after doing the needed manual rebase of someone else's patch for RELEASE-NOTES or blatantly obvious documentation typos.
Tim Starling (WMF) (talkcontribs)

I've hopefully covered this now. Rebasing RELEASE-NOTES could count as a trivial change not needing review.

Tgr (WMF) (talkcontribs)
  • puppet and mediawiki-config are mentioned under the heading "Deployment branches", is that a mistake? I imagine for deployment branches (and more generally backports) we'd want to say that self-merging is fine because these patches already passed code review elsewhere.
  • I think self-merge after a virtual +2 or real +2 and simple rebase / CI failure is normal and does not contradict the policy as currently phrased (note it talks about self-review, not self-merge).
  • To generalize Nikerabbit's last point, what's the difference between someone with +2 self-merging a patch that has been +1-ed by someone without +2, and someone with +2 merging a patch from written by someone without +2? In both cases the code is reviewed by exactly one person who is trusted with and responsible for enforcing conventions etc.etc. and another person who is less trusted. So the arguments given in the self-review section don't fully make sense to me.
    • And then sometimes that leads to merging code written by a less experienced developer that's OK but not great, because directing them through all the necessary changes would take forever and would not be a great experience for either of us, and changing the patch would mean I'd have to find another person with +2.
  • "Inline comments can be used to indicate issues with the code that should be addressed before it is merged. Exercising -1 or -2 is just as important as +2." - this should probably be in the previous section, not self-review?
Tim Starling (WMF) (talkcontribs)

I removed the bit about -1 being as important as +2, because it seems a bit glib and confusing. Hopefully my change to the language to emphasize code review over merging covers this anyway. That change also hopefully answers your first point.

TCipriani (WMF) (talkcontribs)

puppet and mediawiki-config are mentioned under the heading "Deployment branches", is that a mistake? I imagine for deployment branches (and more generally backports) we'd want to say that self-merging is fine because these patches already passed code review elsewhere.

I was given pause in reading the same bulletpoint. I made a note to myself that perhaps the wording, "In deployment branches and the Puppet repository, changes are merged by the person doing the deployment" could be modified to indicate that self-+2 is common in any repository (or branch within a repository) where +2 indicates deployment rather than review; e.g., Deployment branches, integration changes, puppet repositories.

I don't know how to improve that wording, or whether the wording necessarily needs changes. I will say I did not understand the current wording quickly.

DKinzler (WMF) (talkcontribs)

> In both cases the code is reviewed by exactly one person who is trusted with and responsible for enforcing conventions etc.etc. and another person who is less trusted

Not really - in both cases the code has been viewed by one who is trusted with and responsible for conventions. But spotting one's own mistakes is a lot harder than spotting other people's mistakes. So I do think it makes sense to require review by another person with +2 rights who is not the author.

Smalyshev (WMF) (talkcontribs)

For some smaller projects, we can be in a situation where only one person has enough knowledge on the project to sensibly do +2, or, even more frequently, only one such person is available (e.g. there may be two but one is on a month-long leave, or buried under urgent workloads, otherwise inaccessible). In this case, if the person who has +2 authors a change, there would be no way to merge it without self-merging.

DKinzler (WMF) (talkcontribs)

@Smalyshev, I think that's covered by "For extensions (and other projects) not deployed to the Wikimedia cluster, the code review policy is up to the maintainer or author of the extension".

Reply to "About +2"

Specify application process for people who have been denied or lost privileges in the past

1
DKinzler (WMF) (talkcontribs)

On phab:T218135, Leszek raised the question of trusted orgs requesting access for people who had requested but had been denied merge rights in the past, as well as people who have held but lost merge rights in the past.

I propose to amend the policy to state that gerrit admins(*) should be advised to not use the "abridged" process without community discussion for people who have been denied before or lost privileges. Trusted orgs should, to the best of their knowledge, inform gerrit admins about such cases.

There is one case for which this would be annoying, though: people who lost their privileges per default, due to their contract or employment ending. It would be nice if they could be exempt from this rule, but that problematic as well: The policy requires privileges to be revoked per default so no information is exposed about whether the revocation happened due to any fault. So there might indeed have been an issue, we can't know.

I cite myself as an example: had this policy been in place when I left WMDE and joined WMF in October, I would have lost all privileges per default, and, with this amendment, would have to go through a community discussion before re-gaining them. Perhaps that could have been avoided if the gerrit admin in question had checked back with WMDE to ensure that there were no trust issues with me re-gaining the privileges. That however raises the question whether it would be legal for WMDE to share information about any issues I might have had working there, or the reasons for leaving.

(*) "admins" being gerrit users who have the necessary permissions to grant or revoke privileges.

Reply to "Specify application process for people who have been denied or lost privileges in the past"

Allow trusted users to be owners of repos

3
Summary by Jdforrester (WMF)

No, thank you.

Paladox (talkcontribs)

Hi, we should allow trusted users (long time/wmf staffers) to be able to be owners of there repos.

Paladox (talkcontribs)

Bump.

Tim Starling (WMF) (talkcontribs)

I don't think so.

mediawiki/extensions.git submodules additions/removals

2
MarcoAurelio (talkcontribs)

Can we continue to self-merge this kind of changes? Given that mediawiki/extensions.git changes mostly involve adding or removing submodules to track newly added or archived extensions, can I consider this, in light of this new policy, that this qualifies as trivial to self-merge? (exception for the python and shell files which do indeed require review and I wouldn't self-merge). It looks (and thus why I've been doing that) that most patches are self-merged there when it's just adding or removing a submodule as it's purely routine. Thanks.

Edit: other cases I need to self-merge is to when I am creating a new Gerrit repo or adjusting its permissions.

Tim Starling (WMF) (talkcontribs)

If you're doing it already and nobody cares, it's probably fine. The section on self merging is merely descriptive of the current situation. It has plenty of wiggle room to allow things that are happening already.

I also want to emphasize that we're not going to suddenly revoke +2 access of a valued contributor for violating some narrowly interpreted clause in the policy. I can't speak for all situations or all committee members, but if someone complains that you shouldn't be doing a particular variety of self merges, the obvious outcome of that is to ask you to stop doing it.

The policy defines an escalation path for complaints which allows them to be handled with common sense and without needless public humiliation.

Reply to "mediawiki/extensions.git submodules additions/removals"