If I reply to a patch in my mailer to say "OH MY GOD THIS IS THE WORST THING EVER", I don't want to also have to go into patchworks to set the patch status to rejected. Some sort of magic in-line metadata such as putting "Patchworks-Status: Rejected" on a newline would be awesome. (also useful for "I sent a v2 of this but forgot to tag it as such, Patchwork-Status: Superseded).
I've developed a patch for parsemail.py to parse a comment's content and identify the presence of a defined string to trigger a patch status change. It was successfully tested in a patchwork test instance, however, I'd like to collect some feedback before testing this change in OE's staging patchwork: Currently, only the original patch submitter and the project maintainers (when logged as such) are able to change a patch status. Enabling the status-changing from an email does bypass patchwork's user authentication, unless we also include metadata to identify the user in the email (not sure of feasibility). From other requests, we concluded that this type of additional authentication would involve a complexity level that may not be worth the effort (to maintain security at least). One approach that I may suggest would be to only allow for certain statuses to be available for email-changing, for example: Under Review, RFC, Not Applicable, Changes Requested, Awaiting, Upstream, Superseded, Deferred and leave Accepted and Rejected as they currently are: to only be set either by authorized users from the Patchwork UI, or when a patch is merged. Let me know if we should include someone else in this discussion.
Jose and I discussed this and I think this is a reasonable feature to have. We can check if the email comes from the submitter or a person with an account in patchwork with sufficient permissions simply by looking at the from address. Regarding security concerns, when I analyse risks I consider likelihood and consequences: * Likelihood - it seems rather unlikely that someone would spoof the email of someone else on the list; as far as I can recall it's never happened within our project and there would be little benefit to doing so. * Consequences - patchwork doesn't directly control any other process, so this can't result in any malicious patch getting merged; the worst that could happen is a bunch of patches have their status changed which could easily be reverted. Furthermore, it's highly unlikely that this would go unnoticed - the email would have to go onto the public mailing list, where the person who'd been spoofed (and everyone else) would be able to see it. We would want to try to ensure it wasn't accidentally triggered, though. For example, we shouldn't be looking inside the body of patches - if the email is solely a patch, just the commit message (and text after ---).
I'd like the ability to set Rejected from a mail to be honest, so I can review with comments on the list and set the rejected flag without having to open a web browser. Changes Requested covers "you've made a typo" but doesn't cover "no, you're doing this wrong, don't do this".
I've sent a patch to allow patch state to be changed if the string "Patchwork-Status: <new-state>" is found in the message body and only if the comment author is recognized as a project maintainer: https://lists.yoctoproject.org/pipermail/yocto/2017-January/034143.html This has been successfully tested in a testing instance and is currently implemented in patchwork-staging: https://patchwork-staging.openembedded.org/
Ross: You can test this feature now by replying to a patch thread and including anywhere in the reply body a new line with the string (including square brackets): [Patchwork-Status: <the desired status>] where the desired status can be any of the existing ones, excepting Accepted. You will see no change at production, but at patchwork-staging the patch status will be updated.
It worked! What level of filtering does it do - if user A sets status Foo, user B sets status Bar, and then A replies to their original mail will the hook notice the quoted [Patchwork-Status] line and change the status again?
The script looks for all string matches, but reply lines (starting with any character other than "[" or a space/tab) are ignored. If there are multiple matches, the script uses the last found.
Sounds good, thanks.
This change is implemented in OE-patchwork. Project maintainers can update a patch status by replying to a patch thread and including in the email message a new line with the string (no leading spaces or tabs): [Patchwork-Status: <new-status>] where <new-status> can be any of Under Review Rejected Not Applicable Changes Requested Awaiting Upstream Superseded Deferred this feature is only available for emails submitted by project maintainers recognized as such in OE-patchwork.