operations/puppet patches having CR+2 are not retested on a rebase
Closed, ResolvedPublic
Actions

Description

When an event occur on a change having Code-review +2, it is enqueued in gate-and-submit and would eventually be submitted in Gerrit.

Since T105474 / 0f0aecfb695e668786ac9847dfed63f79d8fcca6 , a change having CR+2 would not be rechecked at all.

On the operations/puppet that means such a change would not have any test run again for example when doing a rebase. An example is https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/509888/

Patchset 2 got a Code-Review +2, the rebase had the vote sticking which filters it out from the test-prio pipeline.

The workaround is to remove the Code-Review +2 label and recheck.

Details

Related Gerrit Patches:

integration/config : master	Revert "zuul: skip test/test-prio for CR+2 changes"
operations/puppet : refs/meta/config	Allow tests to run on trivial rebase

Related Objects

Mentioned In: T105474: 'recheck' on a CR+2 patch should trigger gate-and-submit, not test
T223212: Whitelisted users without +2 rights can no longer recheck patches which have a +2
Mentioned Here: T223212: Whitelisted users without +2 rights can no longer recheck patches which have a +2
rCICF0f0aecfb695e: zuul: skip test/test-prio for CR+2 changes
T105474: 'recheck' on a CR+2 patch should trigger gate-and-submit, not test

Event Timeline

• hashar created this task.May 14 2019, 9:39 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 14 2019, 9:39 AM

• hashar added a project: Release-Engineering-Team (Kanban).May 14 2019, 10:06 AM

• hashar moved this task from Backlog to In progress on the Continuous-Integration-Config board.

Change 510112 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/puppet@refs/meta/config] Allow tests to run on trivial rebase

https://gerrit.wikimedia.org/r/510112

gerritbot added a project: Patch-For-Review.May 14 2019, 10:06 AM

jbond added a subscriber: jbond.May 14 2019, 11:00 AM

Relevant Gerrit code:

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java

private static boolean canCopy(
    ProjectState project, PatchSetApproval psa, PatchSet.Id psId, ChangeKind kind) {
  int n = psa.getKey().getParentKey().get();
  checkArgument(n != psId.get());
  LabelType type = project.getLabelTypes().byLabel(psa.getLabelId());
  if (type == null) {
    return false;
  } else if ((type.isCopyMinScore() && type.isMaxNegative(psa))
      || (type.isCopyMaxScore() && type.isMaxPositive(psa))) {
    return true;
  }
  switch (kind) {
    case MERGE_FIRST_PARENT_UPDATE:
      return type.isCopyAllScoresOnMergeFirstParentUpdate();
    case NO_CODE_CHANGE:
      return type.isCopyAllScoresIfNoCodeChange();
    case TRIVIAL_REBASE:
      return type.isCopyAllScoresOnTrivialRebase();
    case NO_CHANGE:
      return type.isCopyAllScoresIfNoChange()
          || type.isCopyAllScoresOnTrivialRebase()
          || type.isCopyAllScoresOnMergeFirstParentUpdate()
          || type.isCopyAllScoresIfNoCodeChange();
    case REWORK:
    default:
      return false;
  }
}

The labels setting copyMaxScore (doc), defaults to false.

When setting it to true that ensure it is always copied. Setting it to false does not shortcircuit any logic and the other CopyAll* are applied thus the CR+2 get copied :-\

Change 510112 abandoned by Hashar:
Allow tests to run on trivial rebase

https://gerrit.wikimedia.org/r/510112

zuul.Scheduler: Processing trigger event <TriggerEvent patchset-created operations/puppet production 509888,3>

The rebase causes a patchset-created event which is matched:

zuul.IndependentPipelineManager:
Event <TriggerEvent patchset-created operations/puppet production 509888,3>
for change <Change 0x7fadd686ea10 509888,3>
matched <EventFilter types: patchset-created branches: (?!^refs/meta/config) ignore_deletes: True emails: <bunch of emails>>
^^^^^
in pipeline <IndependentPipelineManager test-prio>

The change is added to the pipeline test-prio

zuul.Scheduler: Adding operations/puppet, <Change 0x7fadd686ea10 509888,3> to <Pipeline test-prio>

But it rejects the change since it has a CodeReview +2 event:

zuul.IndependentPipelineManager:
Considering adding change <Change 0x7fadd686ea10 509888,3>

Change <Change 0x7fadd686ea10 509888,3>
does not match pipeline requirement <ChangeishFilter reject_approvals: [{'code-review': 2}]>
^^^^^^^^^^^^^^

Same issue as T223212

• hashar mentioned this in T223212: Whitelisted users without +2 rights can no longer recheck patches which have a +2.May 14 2019, 12:25 PM

Change 510148 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Revert "zuul: skip test/test-prio for CR+2 changes"

https://gerrit.wikimedia.org/r/510148

Change 510148 merged by jenkins-bot:
[integration/config@master] Revert "zuul: skip test/test-prio for CR+2 changes"