Hi Christian
Sorry I'm coming to this late but I have some feedback on your proposal:
* I agree on the need for a TF-M maintainers file and following the Linux style. TF-A already does this and might be worth copying. https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/about/...
* If the TF-A style doesn't work for TF-M then it would be good to figure out why and align both projects.
* The TF-A "Main maintainers" are the ones with +2/merge rights. Other listed maintainers (i.e. sub-maintainers) are there just to show who to chase for code review; they only have +1 rights like everyone else. For TF-M, I'm open to the idea of giving all listed maintainers +2 rights as long as this doesn't give them merge rights! However, I'm sceptical on the value of this since the person merging will still need to check who has given +1/+2 before merging. Also, this could grow to a long list of people if TF-A is anything to go by, which could easily lead to abuse/confusion.
* Regarding expanding the list of those with merge (or "Main maintainer") rights, I agree with Kumar that this should be a meritocracy. However, I don't think the TSC need to be involved in the first instance; this can be simply approved by existing "Main maintainers" via the public lists. I think the TSC only needs to get involved if someone wants to be a "Main maintainer" and the existing "Main maintainers" don't approve, or if someone objects to a new "Main maintainer" being added. Escalation to the board should be rare.
* I don't think TF members should automatically get "Main maintainer" rights, although some TF members may be ready to be "Main maintainers" now (if approved by the existing TF-M "Main maintainers").
Regards
Dan.
From: TSC tsc-bounces@lists.trustedfirmware.org On Behalf Of Christian Daudt via TSC Sent: 19 November 2019 23:32 To: Kumar Gala kumar.gala@linaro.org Cc: tsc@lists.trustedfirmware.org Subject: Re: [TF-TSC] adding maintainers to TF-M
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.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hi Dan, I believe we can standardize on the TF-A style list - my only concern with leaving the kernel format for a different one is that we will not be able to use existing automated parsers, but we don't need them at the moment so should be okay. Is there a definition of 'M', 'G', 'F' written up? As far as I can see those are the only keys in use. As for +2/merge rights: the purpose of the proposal was to eliminate arm as a chokepoint in reviews/merging. We can discuss in the meeting - I'm fine with starting with a list of 'Main maintainers' that resembles current +2ers and going from there. If things are getting held up, then we can revisit. On a separate topic, I'm looking through the TF-A recent merges, and it seems that code makes it in 'master' without being merged through gerrit (e.g. I can only find a merge into 'integration' in gerrit for the lastest commit in 'master': https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=ebff1... https://review.trustedfirmware.org/q/I46b21573ed25a0ff222eac340e1e1fb93b0407...). Did I miss it or is the flow different than submitting to master in gerrit for inclusion into TF-A?
Thanks, Christian.
________________________________ From: Dan Handley Dan.Handley@arm.com Sent: Tuesday, December 3, 2019 4:06 AM To: Christian Daudt Christian.Daudt@cypress.com; Kumar Gala kumar.gala@linaro.org Cc: tsc@lists.trustedfirmware.org tsc@lists.trustedfirmware.org Subject: RE: [TF-TSC] adding maintainers to TF-M
Hi Christian
Sorry I'm coming to this late but I have some feedback on your proposal:
* I agree on the need for a TF-M maintainers file and following the Linux style. TF-A already does this and might be worth copying.
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/about/...
* If the TF-A style doesn't work for TF-M then it would be good to figure out why and align both projects.
* The TF-A "Main maintainers" are the ones with +2/merge rights. Other listed maintainers (i.e. sub-maintainers) are there just to show who to chase for code review; they only have +1 rights like everyone else. For TF-M, I'm open to the idea of giving all listed maintainers +2 rights as long as this doesn't give them merge rights! However, I'm sceptical on the value of this since the person merging will still need to check who has given +1/+2 before merging. Also, this could grow to a long list of people if TF-A is anything to go by, which could easily lead to abuse/confusion.
* Regarding expanding the list of those with merge (or "Main maintainer") rights, I agree with Kumar that this should be a meritocracy. However, I don't think the TSC need to be involved in the first instance; this can be simply approved by existing "Main maintainers" via the public lists. I think the TSC only needs to get involved if someone wants to be a "Main maintainer" and the existing "Main maintainers" don't approve, or if someone objects to a new "Main maintainer" being added. Escalation to the board should be rare.
* I don't think TF members should automatically get "Main maintainer" rights, although some TF members may be ready to be "Main maintainers" now (if approved by the existing TF-M "Main maintainers").
Regards
Dan.
From: TSC tsc-bounces@lists.trustedfirmware.org On Behalf Of Christian Daudt via TSC Sent: 19 November 2019 23:32 To: Kumar Gala kumar.gala@linaro.org Cc: tsc@lists.trustedfirmware.org Subject: Re: [TF-TSC] adding maintainers to TF-M
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.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
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 -
Dan Handley