Stream: project leads (public)

Topic: rust-lang/team Reviewers


XAMPPRocky (Feb 18 2020 at 21:40, on Zulip):

Hey, in getting more people to sign up and use rust-lang/team, I've seen more and more people ask about who they should ping to merge the changes, because they'll ping their team leads who approved the PR, but they often aren't the same people who can merge it. Can we document who to ping? Right I think it's been defacto core (mainly pietro, & niko afaik), and could we increase the bus factor of people with merge permissions?

pnkfelix (Feb 18 2020 at 21:41, on Zulip):

hmm the people with review rights should all have merge permissions, no?

XAMPPRocky (Feb 18 2020 at 21:41, on Zulip):

Ideally maybe we could have a bot that merges when team lead(s) approve.

pnkfelix (Feb 18 2020 at 21:41, on Zulip):

maybe I'm out of touch with the current setup of other teams.

XAMPPRocky (Feb 18 2020 at 21:42, on Zulip):

@pnkfelix I mean the number of people who review rust-lang/team repo specifically, sorry if that was unclear.

pnkfelix (Feb 18 2020 at 21:43, on Zulip):

oh oh

pnkfelix (Feb 18 2020 at 21:43, on Zulip):

sorry i get it now

simulacrum (Feb 18 2020 at 21:52, on Zulip):

I think just core is fine for now. Once team lead approves (who realistically is likely at least somewhat on core).

XAMPPRocky (Feb 18 2020 at 21:53, on Zulip):

@simulacrum Not working group leads, which are the larger amount of traffic on team.

simulacrum (Feb 18 2020 at 21:56, on Zulip):

I think the majority of traffic is from formation of the ice breaker and code cleanup WGs, and that'll (I suspect) die down to near-zero within days.

simulacrum (Feb 18 2020 at 21:56, on Zulip):

I personally believe that there is not enough reason to write a bot, and I would not be entirely comfortable with giving a bot that kind of power.

XAMPPRocky (Feb 18 2020 at 22:19, on Zulip):

@simulacrum Well my preferred solution would be to just add more people who can merge. I don't think leaving it to core is any kind of solution, when core is already so overwhelmed. There are issues old enough to walk and talk blocked on core.

Pietro Albini (Feb 18 2020 at 22:21, on Zulip):

@XAMPPRocky the team repo is critical, because anyone with unconditional write access to it can escalate permissions up to core and infra-admins

Pietro Albini (Feb 18 2020 at 22:21, on Zulip):

I had an idea to ease the problem, but I didn't have time to implement it yet

Pietro Albini (Feb 18 2020 at 22:22, on Zulip):

basically what we want is:

Pietro Albini (Feb 18 2020 at 22:23, on Zulip):

so my idea was to split the team files into two files, like:

teams/infra/people.toml
teams/infra/meta.toml

and use github's code owners feature to make the relevant team leads and core the "code owners" of their team, and just core the "code owners" of any other file in the repo

Pietro Albini (Feb 18 2020 at 22:23, on Zulip):

and configure github to require approval from code owners to allow merging changes

Pietro Albini (Feb 18 2020 at 22:25, on Zulip):

if github works the way we want (any change other than to a people.toml requires core team approval, even if the other file changed is a people.toml), that would allow us to safely give write access to all team and wg leads, while being assured that they can't escalate privileges

Pietro Albini (Feb 18 2020 at 22:25, on Zulip):

this needs more testing in a dummy repo to make sure the feature works the way we expect (I don't have much experience with it, I mostly just read the docs) and then the actual implementation work

Pietro Albini (Feb 18 2020 at 22:27, on Zulip):

let me know if something is not clear, and I'll try explain it better when it's not late here :)

XAMPPRocky (Feb 18 2020 at 22:27, on Zulip):

@Pietro Albini I understand it's critical, I still think it's worth the potential risk.

Pietro Albini (Feb 18 2020 at 22:28, on Zulip):

I absolutely don't want any wg lead to be able to redirect admin@rust-lang.org emails, that would be horrible

XAMPPRocky (Feb 18 2020 at 22:28, on Zulip):

Like what you said you proposed is very complicated compared to just trusting someone.

Pietro Albini (Feb 18 2020 at 22:28, on Zulip):

it's not just trust in the leads themselves, but also in their security practices

Pietro Albini (Feb 18 2020 at 22:28, on Zulip):

and limiting the attack surface of the project as a whole

Pietro Albini (Feb 18 2020 at 22:30, on Zulip):

I'd love to fix the problem, but the solution needs to avoid allowing leads to escalate privileges

Pietro Albini (Feb 18 2020 at 22:31, on Zulip):

if you have better ideas I'm all ears :D

XAMPPRocky (Feb 18 2020 at 22:36, on Zulip):

@Pietro Albini Well we could have a train model, similar to the website or rust with master as a protected branch.

XAMPPRocky (Feb 18 2020 at 22:38, on Zulip):

Have a new branch called pending, and leads can merge whatever into that, and then someone from core commits a block of changes at once instead of each one individually.

Lokathor (Feb 18 2020 at 22:39, on Zulip):

I'm not clear how that is better from a lot of small PRs getting merged as a batch

XAMPPRocky (Feb 18 2020 at 22:40, on Zulip):

@Lokathor What you said is what I proposed so I'm not sure what you mean.

Lokathor (Feb 18 2020 at 22:41, on Zulip):

I mean I don't see how merging 5PRs to a pending branch and then merging that branch to master once is better than just merging 5PRs each to master

XAMPPRocky (Feb 18 2020 at 22:42, on Zulip):

@Lokathor Because any lead could merge the 5 PRs, and then someone from core only has to merge and review one PR consisting of 5 commits.

XAMPPRocky (Feb 18 2020 at 22:45, on Zulip):

It reduces the number of tasks required by someone in core from 5 to 1.

Pietro Albini (Feb 18 2020 at 22:49, on Zulip):

hmm

Pietro Albini (Feb 18 2020 at 22:50, on Zulip):

the downside of that would be increased friction when someone from core wants to merge just a single PR

Pietro Albini (Feb 18 2020 at 22:50, on Zulip):

as we would have to first merge into pending and then merge pending into master

Pietro Albini (Feb 18 2020 at 22:51, on Zulip):

potentially having to review other changes in pending in the meantime, even though we only wanted to merge a single PR

Pietro Albini (Feb 18 2020 at 22:52, on Zulip):

the way I see it we only have this problem if there is a large influx of new members

Pietro Albini (Feb 18 2020 at 22:52, on Zulip):

so we could create tooling to easily rollup PRs, and the lead of a icebreakers crew could rollup the tens of PRs related to that into a single one for core to review

Pietro Albini (Feb 18 2020 at 22:52, on Zulip):

like we've been doing manually these days

XAMPPRocky (Feb 18 2020 at 22:57, on Zulip):

I feel like the increased friction is quite minimal, it's rare that any change to rust-lang/team is urgent, and it's fine if some of the time it's a bit harder for core, when it's easier for everyone else most of the time.

XAMPPRocky (Feb 18 2020 at 23:03, on Zulip):

I don't feel like tooling will solve this problem, that's adding more dependencies, as the tooling needs to be maintained.

simulacrum (Feb 18 2020 at 23:03, on Zulip):

to be clear, I think if someone wants to create batch PRs then that'd be fine IMO, though it may not be that much easier than just individual PR review anyway

XAMPPRocky (Feb 18 2020 at 23:06, on Zulip):

@simulacrum Well the main part of that is having master protected, and giving leads write permissions, which someone can't just do, it has be someone on core.

XAMPPRocky (Feb 18 2020 at 23:07, on Zulip):

Just making batch PRs wouldn't make any sense.

simulacrum (Feb 18 2020 at 23:07, on Zulip):

batch PRs are equivalent to the staging branch?

simulacrum (Feb 18 2020 at 23:07, on Zulip):

I'm not opposed to having a staging branch that is periodically merged into master by someone from core, via a PR that pings someone on core

simulacrum (Feb 18 2020 at 23:07, on Zulip):

presuming we can do that in github without granting everyone write permissions

XAMPPRocky (Feb 18 2020 at 23:08, on Zulip):

@simulacrum Yes equivalent to that.

XAMPPRocky (Feb 18 2020 at 23:08, on Zulip):

Well I believe we already have that situation on other repos, e.g. I can write to the master branch of the website but not deploy.

simulacrum (Feb 18 2020 at 23:29, on Zulip):

hm, well, something to think about. I don't personally see current situation as a big problem.

XAMPPRocky (Feb 19 2020 at 00:14, on Zulip):

@simulacrum I'm not saying it's a big problem. I don't know if we have any really big problems, we have a lot of little ones.

simulacrum (Feb 19 2020 at 00:16, on Zulip):

Sure. I guess something we could fix. Probably won't have time soon though :)

spacekookie (Mar 23 2020 at 13:25, on Zulip):

(deleted)

Last update: Jun 05 2020 at 22:55UTC