On Nov 18, 2019, at 5:36 PM, Christian Daudt via TSC tsc@lists.trustedfirmware.org wrote:
Hi TSC, As discussed in the last TSC meeting, we would like to add Cypress devs as platform maintainers for PSoC6 code in TF-M. Given that (afaik) we are the first non-ARM maintainer, I'd like to formalize the proposal.
Proposal to enhance maintainership of trusted-firmware-m repository to allow additional approvers
Step 1: Add a MAINTAINERS file to the root of the tf-m repository. This file will follow the same format as the linux kernel MAINTAINERS file, but will only support a subset of options. Exact subset TBD but likely 'M', 'L', 'S', 'F' options will be supported. This MAINTAINERS file will initially be populated with the current designated +2 devs as having maintainer status over the complete tree. It is assume that current devs are already being notified of gerrit reviews so they don't need that step.
Step 2: All tsc representatives will be granted +2/merge permissions into trusted-firmware-m. Allowable uses of this +2 are as follows: a) For gerrit reviews that exclusively involve changes to files+directories where the tsc rep's company is listed as maintainer in the MAINTAINERS file b) For gerrit reviews that involve changes to files+directories where the tsc rep's company is listed as maintainer in the MAINTAINERS file + other changes: only once other maintainers have +1 their portions (i.e. once all maintainers have +1, any of the companies involved in the patch can +2/merge it). Abuse of this privilege will be escalated to board for resolution.
Step 3: on any patch that requires a change to maintainers (i.e. a new platform being added): a) the patch will include an update to the MAINTAINERS file indicating it. b) the maintainers will add themselves to get notifications from gerrit for appropriate subtrees.
Responsibility of maintainers:
- Ensure MAINTAINERS file and your gerrit notifications are kept up-to-date
- Provide review in a timely fashion for any reviews that involve your subtree (preferably within a week)
- Notify your tsc rep once a review has sufficient approvals to be +2ed.
Let me know of any comments, concerns, issues. If there is agreement on the above, I'll outline the steps with more detail + examples (in phabricator possibly or as a documentation patch to the code itself). Thanks, Christian.
STEP 2/3: I don’t think TSC reps should be given this permission by default. I think the TSC should vote/approve any Maintainer. The reason for this is to have things be a meritocracy.
The rules for approval w/regards to +1/+2 should be around MAINTAINERs regardless who they work for.
I think at this point of the project that hopefully the TSC would approve a MAINTAINER proposed by a Silicon Vendor for the Silicon Vendor’s specific code.
- k
Kumar Gala wrote:
On Nov 18, 2019, at 5:36 PM, Christian Daudt via TSC tsc@lists.trustedfirmware.org wrote:
Step 2: All tsc representatives will be granted +2/merge permissions into trusted-firmware-m. Allowable uses of this +2 are as follows: a) For gerrit reviews that exclusively involve changes to files+directories where the tsc rep's company is listed as maintainer in the MAINTAINERS file b) For gerrit reviews that involve changes to files+directories where the tsc rep's company is listed as maintainer in the MAINTAINERS file + other changes: only once other maintainers have +1 their portions (i.e. once all maintainers have +1, any of the companies involved in the patch can +2/merge it). Abuse of this privilege will be escalated to board for resolution.
Step 3: on any patch that requires a change to maintainers (i.e. a new platform being added): a) the patch will include an update to the MAINTAINERS file indicating it. b) the maintainers will add themselves to get notifications from gerrit for appropriate subtrees.
Responsibility of maintainers:
- Ensure MAINTAINERS file and your gerrit notifications are kept up-to-date
- Provide review in a timely fashion for any reviews that involve your subtree (preferably within a week)
- Notify your tsc rep once a review has sufficient approvals to be +2ed.
Let me know of any comments, concerns, issues. If there is agreement on the above, I'll outline the steps with more detail + examples (in phabricator possibly or as a documentation patch to the code itself).
Thanks, Christian.
STEP 2/3: I don’t think TSC reps should be given this permission by default. I think the TSC should vote/approve any Maintainer. The reason for this is to have things be a meritocracy.
The rules for approval w/regards to +1/+2 should be around MAINTAINERs regardless who they work for.
I think at this point of the project that hopefully the TSC would approve a MAINTAINER proposed by a Silicon Vendor for the Silicon Vendor’s specific code.
- k
The intention around this permission is not to give auto-maintainer status to anyone - it is an attempt to bolt a linux-style maintainership with the realities of the tool (gerrit). It is a somewhat separate item from maintainership to try to scale ARM devs who are the only ones with +2 access to the repo at present. The intent here is that, for maintainers that are employed by a member company, the tsc rep can push the code (after the maintainer approves it) to reduce the load on the ARM +2ers. Non-member maintainers are not precluded from being added in any way - my original intention is that responsibility of +2/merging on behalf of the non-member maintainers would remain with ARM folks (as it it today for all patches) - but we can revisit those rules once we have some non-member maintainers ready to be admitted. If there is a better way to synchronize the intention (of distributing some of the maintainership) with the tool at hand (gerrit) I'd be happy to incorporate that into the proposal.
Thanks, Christian.
PS: I manually did the reply indentation - hope it worked. Sigh outlook...
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
On Nov 18, 2019, at 7:04 PM, Christian Daudt Christian.Daudt@cypress.com wrote:
Kumar Gala wrote:
On Nov 18, 2019, at 5:36 PM, Christian Daudt via TSC tsc@lists.trustedfirmware.org wrote:
Step 2: All tsc representatives will be granted +2/merge permissions into trusted-firmware-m. Allowable uses of this +2 are as follows: a) For gerrit reviews that exclusively involve changes to files+directories where the tsc rep's company is listed as maintainer in the MAINTAINERS file b) For gerrit reviews that involve changes to files+directories where the tsc rep's company is listed as maintainer in the MAINTAINERS file + other changes: only once other maintainers have +1 their portions (i.e. once all maintainers have +1, any of the companies involved in the patch can +2/merge it). Abuse of this privilege will be escalated to board for resolution.
Step 3: on any patch that requires a change to maintainers (i.e. a new platform being added): a) the patch will include an update to the MAINTAINERS file indicating it. b) the maintainers will add themselves to get notifications from gerrit for appropriate subtrees.
Responsibility of maintainers:
- Ensure MAINTAINERS file and your gerrit notifications are kept up-to-date
- Provide review in a timely fashion for any reviews that involve your subtree (preferably within a week)
- Notify your tsc rep once a review has sufficient approvals to be +2ed.
Let me know of any comments, concerns, issues. If there is agreement on the above, I'll outline the steps with more detail + examples (in phabricator possibly or as a documentation patch to the code itself). Thanks, Christian.
STEP 2/3: I don’t think TSC reps should be given this permission by default. I think the TSC should vote/approve any Maintainer. The reason for this is to have things be a meritocracy.
The rules for approval w/regards to +1/+2 should be around MAINTAINERs regardless who they work for.
I think at this point of the project that hopefully the TSC would approve a MAINTAINER proposed by a Silicon Vendor for the Silicon Vendor’s specific code.
- k
The intention around this permission is not to give auto-maintainer status to anyone - it is an attempt to bolt a linux-style maintainership with the realities of the tool (gerrit). It is a somewhat separate item from maintainership to try to scale ARM devs who are the only ones with +2 access to the repo at present. The intent here is that, for maintainers that are employed by a member company, the tsc rep can push the code (after the maintainer approves it) to reduce the load on the ARM +2ers. Non-member maintainers are not precluded from being added in any way - my original intention is that responsibility of +2/merging on behalf of the non-member maintainers would remain with ARM folks (as it it today for all patches) - but we can revisit those rules once we have some non-member maintainers ready to be admitted. If there is a better way to synchronize the intention (of distributing some of the maintainership) with the tool at hand (gerrit) I'd be happy to incorporate that into the proposal.
Thanks, Christian.
PS: I manually did the reply indentation - hope it worked. Sigh outlook…
I guess the question is if we separate the review/approval role from the merging role. If that’s the intent then I think we should define such roles.
Maintainer: Reviews/approves code to merge Merger: Merges code that has been approved, is aware of state of project if it makes sense to review (ie, if in a release window, might not make sense to merge code even if its approved).
We are actually going through a similar discussion / process in the Zephyr project.
PS: Outlook munged it, but its workable :)
- k
Kumar Gala wrote: I guess the question is if we separate the review/approval role from the merging role. If that’s the intent then I think we should define such roles.
Maintainer: Reviews/approves code to merge Merger: Merges code that has been approved, is aware of state of project if it makes sense to review (ie, if in a release window, might not make sense to merge code even if its approved).
We are actually going through a similar discussion / process in the Zephyr project.
Fair point. I'll update the proposal to separate the 2 as reviewer and merger roles. Any pointers to the Zephyr outcome that I can ... borrow ... from ?
Thanks, csd
This message and any attachments may contain confidential information from Cypress or its subsidiaries. If it has been received in error, please advise the sender and immediately delete this message.
-
Christian Daudt -
Kumar Gala