Page MenuHomePhabricator

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

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 created this task.Jul 10 2015, 11:11 AM
hashar raised the priority of this task from to Normal.
hashar updated the task description. (Show Details)
hashar added a subscriber: hashar.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 10 2015, 11:11 AM

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

Yep, this would be great! :D

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 set Security to None.Oct 26 2015, 12:03 PM
Paladox removed a subscriber: Legoktm.
Zppix added a subscriber: Zppix.Apr 5 2017, 9:39 PM

@hashar was this ever implemented?

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?

hashar claimed this task.May 2 2019, 2:34 PM
hashar moved this task from Backlog to In progress on the Continuous-Integration-Config board.
hashar moved this task from Backlog to In-progress on the Release-Engineering-Team (Kanban) board.
hashar closed this task as Resolved.May 6 2019, 9:25 AM

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

hashar added a comment.May 6 2019, 2:55 PM

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}]>
hashar added projects: Zuul, Upstream.EditedMay 6 2019, 9:36 PM

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

hashar added a comment.May 7 2019, 8:38 AM

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

Mentioned in SAL (#wikimedia-releng) [2019-05-13T10:37:26Z] <hashar> Rolling CI config change https://gerrit.wikimedia.org/r/508512 which caused some patches to not be processed last week # https://wikitech.wikimedia.org/wiki/Incident_documentation/20190506-zuul / T105474

hashar closed this task as Resolved.May 13 2019, 10:46 AM

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

hashar reopened this task as Open.May 14 2019, 12:34 PM

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

hashar removed hashar as the assignee of this task.May 14 2019, 6:34 PM

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).