Hi Sandrine,

If my understanding is right, Code-Owner-Review is the equivalent to the old Code-Review+1 and therefore anyone can add a vote there, owner or not. If that is so, I think the current name might be misleading. Wouldn't it be better just "Code-Review", for instance?

As an example, I am not code owner for Measure Boot, but sometimes I am requested to review the patches for it, so in that case it might lead to confusion if I give Code-Owner-Review score.

What do you think?

Best regards, Javier

-----Original Message----- From: Sandrine Bailleux via TF-A <tf-a@lists.trustedfirmware.orgmailto:Sandrine%20Bailleux%20via%20TF-A%20%3ctf-a@lists.trustedfirmware.org%3e> Reply-To: Sandrine Bailleux <sandrine.bailleux@arm.commailto:Sandrine%20Bailleux%20%3csandrine.bailleux@arm.com%3e> To: tf-a <tf-a@lists.trustedfirmware.orgmailto:tf-a%20%3ctf-a@lists.trustedfirmware.org%3e> Subject: [TF-A] Announcing some changes around the code review label in Gerrit Date: Tue, 30 Jun 2020 13:29:00 +0000

Hello all,

If you recall, the tf.org Project Maintenance Process [1] advocates a

code review model where both code owners and maintainers need to review

and approve a patch before it gets merged.

Until now, the way we would record this in Gerrit was a bit cumbersome

and ambiguous.

* Code-Review+1 was for code owners to approve a patch.

* Code-Review+2 was for maintainers to approve a patch.

The submission rules in Gerrit were such that a patch needed a

Code-Review+2 to be merged. This meant that if a maintainer was first to

take a look at a patch and approved it (Code-Review+2), Gerrit would

allow the patch to be merged without any code owners review.

To align better with the Project Maintenance Process, I've done some

changes in our Gerrit configuration. You will notice that the

Code-Review label is gone and has been replaced by 2 new labels:

* Code-Owner-Review

* Maintainer-Review

Stating the obvious but code owners are expected to vote on the

Code-Owner-Review label and maintainers on the Maintainer-Review.

Note that maintainers might also be code owners for some modules. If

they are doing a "code owner" type of review on a patch, they would vote

on the Code-Owner-Review label in this case.

Any doubt, please ask. I hope I didn't break anything but if you notice

any permissions problems (things you used to be able to do and cannot do

anymore, or vice versa!) please let me know.

One annoying consequence of this change is that if you've got some

patches in review right now and got some votes on the former

'Code-Review' label, these have been partially lost. The history of

review comments in Gerrit will still show that somebody had voted

Code-Review -1/+1 on your patch but this will no longer appear in the

labels summary frame and won't count towards the approvals required to

get the patch merged.

I could not think of a way to avoid this issue... This should only be

temporary, while we transition all in-flight patches to the new code

review model.

Roughly:

* Existing "Code-Review -1/+1" votes will have to be replayed as

"Code-Owner-Review -1/+1".

* Existing "Code-Review -2/+2" votes will have to be replayed as

"Maintainer-Review -1/+1".

Sorry for the inconvenience. I hope, once we've gone through the

transition period, these changes will make the code review process

clearer to everybody.

I will update the TF-A documentation to explain all of this in the

coming future.

Best regards,

Sandrine

[1]

https://developer.trustedfirmware.org/w/collaboration/project-maintenance-pr...