Merge ~rodrigo-zaiden/ubuntu-cve-tracker:security-signoff-add-assignee into ubuntu-cve-tracker:master

[Rodrigo Figueiredo Zaiden]

Adding a flag to make it possible for the current user to assign the security kernel sign-off task to their-self if the person is question is not the assignee yet.

Do you think this is a valid change?
Thanks!

[Steve Beattie (sbeattie)]

Apologies for the delayed review. I think the idea is okay; I should note that the kernel-sru-script that reports pending signoffs assigns the bug task to the person running the script as well (see https://git.launchpad.net/ubuntu-qa-tools/tree/security-tools/kernel-sru-check#n106 ) but only if the status is "Confirmed".

(Really, that script should probably be moved to UCT, there's additional improvements I'd like to make to it that would be a little easier if it was in UCT and not in ubuntu-qa-tools.)

I guess what I'm more interested is how you see this being used in the signoff / tracking process. AFAICT, it cannot be used independently of changing the task state either acking or nacking it for the security pocket; it might be worth being possible change the assignee independently so that at the beginning of signoffs, you could change self-assign for kernels you're starting the sign-off process on, in part to indicate you're starting on them.

[Rodrigo Figueiredo Zaiden (rodrigo-zaiden)]

Hi Steve,

> Apologies for the delayed review. I think the idea is okay; I should note that
> the kernel-sru-script that reports pending signoffs assigns the bug task to
> the person running the script as well (see https://git.launchpad.net/ubuntu-
> qa-tools/tree/security-tools/kernel-sru-check#n106 ) but only if the status is
> "Confirmed".

thanks for pointing this. I completely missed this part of the kernel-sru-check script.
One thing that caught my attention when re-reading the kernel-sru-workflow is that it seems like the "Security-signoff" task only moves to status "Confirmed" when "Verification-testing" task is "Fix Released", but I've seen cases where "Verification-testing" is "Invalid" and so, "Security-signoff" is not being moved to "Confirmed". Maybe that is something we can raise with the kernel team if this is a valid statement.

step10:
    Once all the bugs, requiring verification, listed in the changelog have been marked
    verification-done-<series>, the kernel team changes the state of the Verification-testing
    task to completed (status: Fix Released). Workflow Mgr. then detects this and sets the
    Certification-testing, Regression-testing and Security-signoff tasks to the ready-to-start
    state (status: Confirmed).

>
> (Really, that script should probably be moved to UCT, there's additional
> improvements I'd like to make to it that would be a little easier if it was in
> UCT and not in ubuntu-qa-tools.)
>
> I guess what I'm more interested is how you see this being used in the signoff
> / tracking process. AFAICT, it cannot be used independently of changing the
> task state either acking or nacking it for the security pocket; it might be
> worth being possible change the assignee independently so that at the
> beginning of signoffs, you could change self-assign for kernels you're
> starting the sign-off process on, in part to indicate you're starting on them.

Well, considering that I was not aware of the current kernel-sru-check script validation on the assignee, I was simply thinking that once someone is signing kernels off (that is, marking "Fix Released" or "Invalid" despite the current status), it would be useful to have that person being assigned to the task because we had cases where you were the assignee and I was doing the sign-off and the task remained assigned to you after the status was changed.
Now, I think that, this is probably a case where the "Security-signoff" task was not "Confirmed" and one of two things happened:
1. I was ahead of time when I wanted to sign things off and didn't properly wait until the task landed in the correct state.
2. "Security-signoff" never changed to "Confirmed" because "Verification-testing" didn't change to "Fix Released".

if any of these 2 happened and the second can be adjusted in the workflow, I think I can simply drop this merge as kernel-sru-script handles the assignee.

When I wrote this change, I was not thinking of using it independently, but I agree it could be used with a different purpose like re-assigning and changing to "In Progress".

For now, I'll move this to "Work in Progress" to make adjustments to be able to change the assignee independentl...

[Rodrigo Figueiredo Zaiden (rodrigo-zaiden)]

Update:
Gave a better attention to this topic, and, first of all, there is no such
thing as having a bug reported in the `kernel-sru-check` with
"Verification-testing" as "Invalid" and needing Security Signoff, I was
probably misreading the bug, sorry for that.

So, considering that the output of `kernel-sru-check` is already updating
the status to "In Progress" and assigning the bug for the person that is
running the script, there is no need (and probably does not make sense)
to change the assignee in `kernel-security-signoff.py`.

When I proposed this merge, I was simply running `kernel-sru-check` and
ACKing or NCKing the outputs, and what I wanted was to update the assignee
as it was assigned to the person that ran `kernel-sru-check` in the first
place.

Now, I think that, better than just changing the assignee when actually
doing the signoff, we could review when we update the status and assignee
in the `kernel-sru-check` script when we have more than one person running
it, specially if it runs in cron jobs and doesn't necessarily means that
the person running the script is working on it.

I'll reject this merge, any ideas or thoughts on the internal process of
assigning and changing status is really welcomed.
Thanks!

Unmerged commits

307ab6d... by Rodrigo Figueiredo Zaiden on 2022-09-13

scripts/kernel-security-signoff.py: add option to update assignee

 when running kernel signoffs with argument -a it is now possible
 to update the task assignee with the current lp user running the
 script

Signed-off-by: Rodrigo Figueiredo Zaiden <email address hidden>

diff --git a/scripts/kernel-security-signoff.py b/scripts/kernel-security-signoff.py
index 4af901d..62a2f8e 100755
--- a/scripts/kernel-security-signoff.py
+++ b/scripts/kernel-security-signoff.py
@@ -40,6 +40,7 @@ parser.add_argument("-n", "--no-change", help="Don't actually adjust bug state",
 parser.add_argument(
     "-i", "--no-security", help="Kernel does not contain security fixes, signoff to updates only", action="store_true"
 )
+parser.add_argument("-a", "--assignee", help="Update assignee with the current logged user", action="store_true")
 parser.add_argument("bugs", help="launchpad bugs to signoff on", nargs="+")
 args = parser.parse_args()
 
@@ -71,6 +72,10 @@ for bugno in args.bugs:
             print("[%s] (Warning) %s has status %s" % (bugno, t.bug_target_display_name, task.status))
 
     if not args.no_change:
+        if args.assignee:
+            if task.assignee != lp.me:
+                debug("[%s] assigning task to %s" % (bugno, lp.me.name))
+                task.assignee = lp.me
         if args.no_security:
             update_status = status_no_security
         else: