If we did this, the C+2 button would disappear from the UI; people would have to open the code review panel and explicitly check C+2 and Send.

Removing the Submit ability from regular users might be a saner choice?

In T226123#6130733, @Jdforrester-WMF wrote:

If we did this, the C+2 button would disappear from the UI; […]

Ha, interesting! I agree removing Submit is definitely worth doing as well. It does however mean Gerrit admins are needed to bypass CI, which might be fine, but I didn't want to go that far all at once. I figured if we keep that, but generally have the test pipeline leave V+1 that means Submit naturally isn't in the UI, unless you manually give it a V+2. So it would leave the intentional ability while removing the unintentional ability.

Speaking of the Submit button appearing etc. I think this is tied to the same kind of ruleset as the CR+2 blue button appearing. We can probably configure it so that CR+2 as a button is shown for V>=1 instead of V>=2.

In T226123#6130749, @Krinkle wrote:

Speaking of the Submit button appearing etc. I think this is tied to the same kind of ruleset as the CR+2 blue button appearing. We can probably configure it so that CR+2 as a button is shown for V>=1 instead of V>=2.

Yeah. One option would be to patch our version of gerrit, but given we're so far behind and are trying to catch up I think that'd be a bad move.

I'm not 100% sure, but looking through Gerrit's documentation it seems like it's logic for buttons and needed scores is configurable via refs/meta/config, much like the general access control rules. This is documented in Gerrit's "Prolog cookbooks".

@Paladox This might be of interest to you. Right now in Gerrit, the blue "Code Review +2" button is only shown if the change has V+2 (usually from Jenkins). We would like for the blue "Code Review +2" button to also be shown if there is only a V+1. Like now, it should still now show it if there is no V or a negative V score.

Do you think that is possible via meta/refs/config rule on All-Projects, based on the examples in the Gerrit Prolog docs?

The blue code-review +2 button is a feature known as quick approval in Gerrit. It shows up when there is one label left without the maximum scoring. Or in other words, it only show when verified+2 is present.

From polygerrit code in 2.15:

    /**
     * Get highest score for last missing permitted label for current change.
     * Returns null if no labels permitted or more than one label missing.
     *
     * @return {{label: string, score: string}|null}
     */
    _getTopMissingApproval() {
...
      for (const label in this.change.labels) {
...
        const status = this._getLabelStatus(this.change.labels[label]);
        if (status === LabelStatus.NEED) {
          if (result) {
            // More than one label is missing, so it's unclear which to quick
            // approve, return null;
            return null;
          }
          result = label;

result hold the label missing a top score. While iterating the labels, if it is already set and another label is missing, the function returns null and the quick approval blue button is not shown.

I used a dummy change https://gerrit.wikimedia.org/r/#/c/test/gerrit-ping/+/226272/ with just a Verified +1. Querying for the label statuses:

gerrit query change:226272 --current-patch-set --submit-records
...
  open: true
  status: NEW

  currentPatchSet:
    approvals:
      type: Verified
      description: Verified
      value: 1

  submitRecords:
    status: NOT_READY
    labels:
      label: Verified
      status: NEED
    labels:
      label: Code-Review
      status: NEED

The change only has Verified +1 and it is thus missing Verified and Code-Review. I guess the submit button only shows up when all labels are at their top score.

So either we:

• loose the cr+2 blue button (which might be annoying)
• dismiss this feature request

We can also prevent the manual submit but:

• some repositories / branches are broken and are typically force merged
• some repos do not gate-and-submit (listed on this task, notably operations/puppet.git or operations/dns.git)
• iirc we need the force merge for vendor.git

• some repositories / branches are broken and are typically force merged.
• some repos do not gate-and-submit (listed on this task, notably operations/puppet.git or operations/dns.git)

This permission is set on a per-repo basis. So ops repos and third-party extensions could stay as-is.

iirc we need the force merge for vendor.git

No, not for anymore. That was resolved :)

To retain the quick blue CR+2, we need Verified to be at max score.

I am tempted to drop the submit permissions, but I am sure people will hate us for that. But that might be doable by raising awareness about it and addresses the use case for a manual submit?

A related use case documented at T257459. A change got a CR+2 but depended on an unmerged change. Since it got V+2 already a user has been able to submit it.

This task would have prevented it if we had a V+1 instead of V+2 or if we disabled submit entirely.

That task got forgotten.

We can't make it vote Verified +1 since that we would loose the quick CR+2 blue button.

I am very tempted to just drop the Submit permission and have CI being the sole doing it (with some exceptions: notably for operations/dns or operations/puppet ) then deal with the aftermaths as they show up. But I am too worry of having to suffer the potential backlashes that might cause.

A reply I have made to an internal discussion we had:

The task is https://phabricator.wikimedia.org/T226123 Make test pipline vote Verified+1 instead of +2 to avoid unintentional submit. If we make CI to vote Verified +1 the maximum verified score is not reached and Gerrit will no more display the {Code-Review +2} blue button to quickly approve. On some repositories for which submit is done manually (operations/puppet, operations/dns),we can adjust the maximum score of Verified to be +1 which would still yield the code-review +2.

An alternative is to remove the Submit permission entirely and only allow CI to submit changes. That will effectively prevents developer from force merging a change which might be disliked (sometime things break, repositories typically do not have CI configured when they are created, I think we might have some use cases for submitting manually).

I would love to have the rules clearly defined in a document for the general use cases and for the few exceptions which would be the reference / a policy. Then have Gerrit CI to just implement the proposal. I have no idea where the topic can be bring on the table though.