[Proposal] Maintainer Approvals and SKARA

[Kevin Rushforth]

Without discussing whether I think this is a better approach than the 
existing label (it's certainly more work, and requires formalizing the 
role an "approver" in Skara, but I'll let others comment on the relative 
merits), I did want to comment on one aspect of your proposal. I don't 
think that having another state *after* integrate is the best way to go. 
Even if you don't use the bug database to record approvals, It seems 
cleaner to make the approval an integration blocker in the same way that 
the appropriate number of reviews, a matching title for the PR and bug, 
etc., are integration blockers. That way once Skara says "ready" to 
integrate, it really is.

So my recommendation is that, regardless of whether SKARA-1199 is 
implemented via JBS labels or some other approval mechanism, the 
"approval" label (or whatever it is called) is added initially when the 
PR is created, and is an integration blocker. An "approver" can then 
indicate approval, either in parallel with the review or after the 
review is done (just like CSR reviews). Once both the review and the 
approval are done, Skara would mark it as "ready".

-- Kevin


On 3/23/2022 1:13 PM, Andrew Hughes wrote:
> Hi all,
>
> We have been discussing how to implement and/or adapt the current
> approval process (jdk<x>-fix-request, jdk<x>-fix-yes, etc.) to SKARA
> on this bug: https://bugs.openjdk.java.net/browse/SKARA-1199
>
> At present, we have been continuing to approve bugs for update
> releases as we did in Mercurial; when the change is reviewed, the
> proposer should flag it as jdk<>-fix-request (or
> jdk<>-critical-request for a rampdown fix) and then an approver
> responds with the *-yes or *-no response on the bug.
>
> SKARA has introduced a new worry for me into this process, due to the
> way the PR bot directs the user through the change process. This in
> itself is a good thing. However, while it is currently not aware of
> the approval process, it can encourage someone to integrate a change
> which has not yet been approved. Hence why I feel some solution to
> SKARA-1199 is quite critical (especially with the potential move of
> the master 8u repository to SKARA) and it surprises me that other
> update projects have not pushed harder for this, prior to 8u adopting
> SKARA.
>
> In thinking through how to implement this in SKARA, it occurred to me
> that trying to continue with what we have now may not be the best
> solution. The issues I see are:
>
> 1. Having the approval process in the bug database separate from the
> proposed commit on GitHub creates a disconnect between the two.
> We had this before, with the separation between mailing list activity
> and the bug database, but now that pretty much everything else happens
> in GitHub/SKARA, it feels odd to have to go back and forth between
> JBS and GitHub to approve a change.
>
> 2. Access to add a label in JBS requires someone to have JDK authorship
> status, so, for non-authors, someone else has to do this on their
> behalf (as with sponsoring)
>
> 3. On the flip-side to #2, any JDK author can add jdk<x>-fix-yes or
> jdk<x>-critical-yes to a bug, as I believe has happened in the past.
>
> 4. From a technical perspective, this means JBS has to be regularly
> probed to pick up new labels. This also affects CSRs, which already
> have issues in this area, and the demand for backports would be
> much higher.
>
> 5. When we've dealt with backports which affect multiple bugs,
> the informal process we've had in the past means we've been able
> to choose whether to require that all bugs are flagged or not.
> Having a formal version of the process we have now in SKARA would
> mean having to label every referenced bug in something like [0].
>
> My proposal would be that we shift approvals to SKARA in a similar
> way to which sponsorship is currently handled:
>
> 1. When a PR for a backport project reaches the point at which
> it would currently be integrated (after either /integrate for
> a committer or /sponsor for a non-committer), it instead
> gets an 'approval' label.
>
> 2. For the PR to move forward, an 'integrator' (someone capable
> of merges & tags) [1] would need to perform '/approval [yes|no]' to
> cause the change to either be integrated or rejected.
>
> I believe this would simplify the implementation of this feature
> a lot and also integrate it better into SKARA.
>
> The bot could still label bugs with the jdk<x>-fix-request when
> it adds the 'approval' label to the PR and with jdk<x>-fix-yes
> and jdk<x>-fix-no when the /approval command is used.
>
> The use of jdk<x>-fix-request or jdk<x>-critical-request can
> also be pre-determined by the bot, based on the repository
> the PR is against (jdk<x>u-dev for the former, jdk<x>u for the
> latter).  This is something that would go in the server-side
> bot configuration.
>
> Thoughts?
>
> I'm honestly struggling to see why we'd hold onto the existing
> process, but I also realise that adaptation can be hard, particularly
> for other update projects where SKARA has been in use longer.
>
> [0] https://github.com/openjdk/jdk8u/commit/845d40ae217ff47ec53e2fd655b95933aa35eac6
> [1] https://bugs.openjdk.java.net/browse/SKARA-1384
>
> Thanks,