Page MenuHomePhabricator

'recheck' on a CR+2 patch should trigger gate-and-submit, not test
Open, MediumPublic

Description

Would be rather useful to be able to reattempt gate-and-submit with 'recheck'. Gotta add to the gate-and-submit pipeline a new condition for comment-added matching 'recheck' and the change having CR+2, and another condition to prevent that in the test (i.e. CR is not +2).

Event Timeline

hashar raised the priority of this task from to Medium.
hashar updated the task description. (Show Details)
hashar added a subscriber: hashar.

@Addshore that is the feature you talked about on IRC :-]

I looked a bit at it, when a recheck comment is added on a change having CR+2:

  • gate-and-pipeline should react to it, but only if there is CR+2 approval
  • test should not enqueue it, i.e. ignore changes that have a Code-Review: +2

Change 227223 had a related patch set uploaded (by Hashar):
'recheck' on CR 2 now triggers gate-and-submit

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

@Legoktm suggests instead of recheck to use resubmit.

Paladox removed a subscriber: Legoktm.

Change 368154 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] 'recheck' on CR 2 now triggers gate-and-submit

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

Change 227223 abandoned by Hashar:
'recheck gate' on CR 2 now triggers gate-and-submit

Reason:
There are too many patchsets spam on this change. I have sent the original patchset as a new change: https://gerrit.wikimedia.org/r/368154

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

Seems that feature is now working. Potentially because when commenting recheck the event emitted by Gerrit now carries the Code-Review:+2 vote and hence the event is matched by the gate-and-submit pipeline.

However, it also enters the test pipeline. So most probably we would want test to filter out +2ed changes?

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

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

Change 508323 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/508323

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

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

Eventually my change broke Zuul workflow entirely. Various changes could not enter the test pipelines. The patch added:

reject:
  # Patches handled by gate pipelines - T105474
  approval:
    - code-review: 2

An example with https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/508011/10 which has been send at 13:26 UTC. In Zuul:

2019-05-06 13:26:35,417 INFO zuul.Scheduler: Adding operations/puppet, <Change 0x7f81228b5f10 508011,10> to <Pipeline test-prio>
2019-05-06 13:26:35,417 DEBUG zuul.IndependentPipelineManager:
    Considering adding change <Change 0x7f81228b5f10 508011,10>
    Change <Change 0x7f81228b5f10 508011,10>
        does not match pipeline requirement <ChangeishFilter reject_approvals: [{'code-review': 2}]>

I need a test that is the other way around, namely ensure a patch without approval is accepted :D

And I would guess patch Zuul with Upstream patch https://review.opendev.org/#/c/589762/

Markus Hosch @ bmw.de

 Fix reject clauses in the absence of approvals

This change fixes a behavior that might be surprising to a
user: If a reject clause is configured for a pipeline, the reject clause
is always true even if no approval is set. If, however, an unrelated
approval is added to the change, the reject clause is evaluated as
expected.

The root cause was a check that assumed that the absence of approvals
will lead to a negative filter result. However, this is only true for
requirements.

Change 508509 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/zuul@patch-queue/debian/jessie-wikimedia] WMF: backport Fix reject clauses in the abscence of approvals

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

Change 508511 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/zuul@debian/jessie-wikimedia] patch: Fix reject clauses in the absence of approvals

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

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

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

Change 508509 merged by Hashar:
[integration/zuul@patch-queue/debian/jessie-wikimedia] WMF: backport Fix reject clauses in the abscence of approvals

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

I have backported the upstream patch https://review.opendev.org/#/c/589762/ which fix rejection always being true when a change lacks approvals. That is now covered by a test in integration/config ( https://gerrit.wikimedia.org/r/#/c/integration/config/+/508512/3/tests/test_zuul_scheduler.py ) which is roughly:

change_without_approvals = zuul.model.Change(
    self.sched.getProject('mediawiki/core'))
change_without_approvals.branch = 'master'

self.assertTrue(
    pipeline.manager.addChange(change_without_approvals),
       "Independent pipeline '%s' must accept a change "
        "without any approval" % (p.name)
)

Change 508511 merged by jenkins-bot:
[integration/zuul@debian/jessie-wikimedia] patch: Fix reject clauses in the absence of approvals

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

Change 508520 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/zuul@debian/jessie-wikimedia] 2.5.1-wmf8: bugs fix following incident

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

Change 508520 merged by jenkins-bot:
[integration/zuul@debian/jessie-wikimedia] 2.5.1-wmf8: bugs fix following incident

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

Mentioned in SAL (#wikimedia-releng) [2019-05-07T10:57:47Z] <hashar> Upgraded Zuul to 2.5.1-wmf8 # T105474 T140297

Mentioned in SAL (#wikimedia-operations) [2019-05-07T11:16:22Z] <hashar> Downgraded Zuul back to 2.5.1-wmf7 # T105474 T140297

Mentioned in SAL (#wikimedia-operations) [2019-05-13T09:33:27Z] <hashar> Upgrading Zuul 2.5.1-wmf7 -> 2.5.1-wmf9 T105474

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

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

So now it should be fixed. The second iteration of the config change ( https://gerrit.wikimedia.org/r/508512 ) does have a test that failed before Zuul 2.5.1wmf8 but now pass.

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"

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

That caused a few side effects such as T223212 or T223209 :-(

The patch worked but it had a few culprit as described in T223212 and T223209 which would need to be addressed:

  • operations/puppet does not have a gate-and-submit so patches never get rechecked
  • On a patch having code-review +2, if someone else comment recheck the comment message does not carry a CR+2 vote. gate-and-submit only process comments that have a code-review +2.

So it is a bit more complicated than expected. I have reverted the configuration change and do not plan to work on this any further (for now).