Add reviewers for gn, gni files

[niranjanyardi]

b/377271824

[andrewsavage1]

I actually don't think I'm a fan of this, as it would require reviewers of even very simple changes like creating a new source file and adding it to the build

[yell0wd0g]

I actually don't think I'm a fan of this, as it would require reviewers of even very simple changes like creating a new source file and adding it to the build

I see this as temporary. In Chromium there're no special GN maintainers, all approvals are done by the owners but until we (Cobalt) are accustomed to is_starboard and the likes I think this is needed. I would expect this to not be needed in ~3 months max from today, looking at the speed of development (basically, as soon as ~everyone has touched a gn file maybe twice).

[yell0wd0g]

As a temporary measure, LGTM.

(Maybe consider leaving a `// TODO(): remove when XYZ.

[niranjanyardi]

I actually don't think I'm a fan of this, as it would require reviewers of even very simple changes like creating a new source file and adding it to the build

Ahh ok, I feel this is similar to the google readability reviewers - we could add more people to the group to make it easier if you are worried about lots of reviewing work .

[andrewsavage1]

Ahh ok, I feel this is similar to the google readability reviewers - we could add more people to the group to make it easier if you are worried about lots of reviewing work .

I don't think that's a good comparison, because the majority of changes to build code (i.e. by people mostly making changes in source code) are very routine. This is reflected in the fact that there aren't google readability requirements for build code in g3.

I'm not worried about the required work load for people who are doing those reviews, I'm worried about slowing down the process for people making changes. Already today people constantly ping group chats for an approval for a 3P group member to be able to submit their code, even though someone from that group is automatically assigned to review that code. If we add another group and have similar issues with people in that group not actively reviewing PRs they are assigned in a timely manner, it's going to go right back to people pinging in gcs.

I'm more okay with this being a temporary measure and making it clear to reviewers in these groups that they should be reviewing these in a timely manner if they're automatically assigned, but right now it feels like a clunk process.

[niranjanyardi]

Ahh ok, I feel this is similar to the google readability reviewers - we could add more people to the group to make it easier if you are worried about lots of reviewing work .

I don't think that's a good comparison, because the majority of changes to build code (i.e. by people mostly making changes in source code) are very routine. This is reflected in the fact that there aren't google readability requirements for build code in g3.

I'm not worried about the required work load for people who are doing those reviews, I'm worried about slowing down the process for people making changes. Already today people constantly ping group chats for an approval for a 3P group member to be able to submit their code, even though someone from that group is automatically assigned to review that code. If we add another group and have similar issues with people in that group not actively reviewing PRs they are assigned in a timely manner, it's going to go right back to people pinging in gcs.

I'm more okay with this being a temporary measure and making it clear to reviewers in these groups that they should be reviewing these in a timely manner if they're automatically assigned, but right now it feels like a clunk process.

I added a comment to remove this in April 2025 (~3 months) . Hmm, the reviewers should definitely be onboard, also didn't we have something to bring more awareness about code review SLO's @hlwarriner ? maybe raising that in meetings might bring more clarity.

I'll do my best to review quickly if assigned to the code review . @oxve / @kaidokert (other build group members) are you on board with this PR ?

Also, to be fair I'm not a member of 3P reviewers so I lack that context as to why that's not effective inspite of there being 6 reviewers. Also things are moving very fast with chrobalt - so that could be a reason.

[kaidokert]

Don't have a strong opinion on this - pros and cons either way. I'm also worried about creating too much ping overhead from one side, but at the same time having extra eyes in this phase of development has its own upside. @niranjanyardi please send a calendar event for us in 3 months to re-assess this :)

[hlwarriner]

Ahh ok, I feel this is similar to the google readability reviewers - we could add more people to the group to make it easier if you are worried about lots of reviewing work .

I don't think that's a good comparison, because the majority of changes to build code (i.e. by people mostly making changes in source code) are very routine. This is reflected in the fact that there aren't google readability requirements for build code in g3.
I'm not worried about the required work load for people who are doing those reviews, I'm worried about slowing down the process for people making changes. Already today people constantly ping group chats for an approval for a 3P group member to be able to submit their code, even though someone from that group is automatically assigned to review that code. If we add another group and have similar issues with people in that group not actively reviewing PRs they are assigned in a timely manner, it's going to go right back to people pinging in gcs.
I'm more okay with this being a temporary measure and making it clear to reviewers in these groups that they should be reviewing these in a timely manner if they're automatically assigned, but right now it feels like a clunk process.

I added a comment to remove this in April 2025 (~3 months) . Hmm, the reviewers should definitely be onboard, also didn't we have something to bring more awareness about code review SLO's @hlwarriner ? maybe raising that in meetings might bring more clarity.

I'll do my best to review quickly if assigned to the code review . @oxve / @kaidokert (other build group members) are you on board with this PR ?

Also, to be fair I'm not a member of 3P reviewers so I lack that context as to why that's not effective inspite of there being 6 reviewers. Also things are moving very fast with chrobalt - so that could be a reason.

We can remind the team about go/cobalt-code-review if it seems like people generally aren't meeting the review SLO. I definitely would prefer a culture in which authors have enough confidence in timely reviews that they don't feel the need to ping reviewers each time they upload a PR. We do still have b/271474913 open for aggregating data to assess how well we, as a team, are meeting the review SLO.

[niranjanyardi]

Don't have a strong opinion on this - pros and cons either way. I'm also worried about creating too much ping overhead from one side, but at the same time having extra eyes in this phase of development has its own upside. @niranjanyardi please send a calendar event for us in 3 months to re-assess this :)

done

[niranjanyardi]

I'm submitting this PR for now.

I added a calnedar event on 1st april to reassess if this is still needed.

If it becomes too much, we can always revert this.