Page MenuHomePhabricator

[keyresult] Connect Differential code review with continuous integration
Closed, ResolvedPublic


Our code review process must be connected to our continuous integration process. In theory, there are different ways to approach this:

We must choose our preferred option, and implement it.

The Zuul option

Zuul is a python daemon which act as a gateway between events in Gerrit and Jenkins. Zuul establish a ssh connection with Gerrit, executing the command 'gerrit stream-events'.

Whenever an event occurs in Gerrit (a vote, a comment, git ref updated, change abandoned, new patchset, etc.) it emits a json formatted event over the ssh channel which is then consumed by Zuul.

Zuul is modular enough that we can add support for Phabricator to it. We will probably need a system to push events much like Gerrit stream-events, or we can hack something in Zuul to poll events from time to time.

We could instead overhaul the continuous integration system to have Phabricator trigger Jenkins jobs directly. But since I want to eventually phase out Jenkins entirely, I think we should keep Zuul around.

See also: T89714: Integrate Jenkins with Phabricator with Harbormaster


ReferenceSource BranchDest BranchAuthorTitle
repos/data-engineering/airflow-dags!498event-platform-integrationmaintchinAdd event streams datahub transformation
repos/releng/blubber!59review/publish-multi-platformexperimental/native-llbdduvallci: Publish a multi-platform image once more
toolforge-repos/phab-ban!5work/bd808/T317584mainbd808Hide ban form if not authorized
repos/abstract-wiki/wikifunctions/function-orchestrator!51apine-no-static-validationmainapineRemove static validation and ensure that all validator functions are pass-throughs.
repos/abstract-wiki/wikifunctions/function-evaluator!29sync-function-schematamainjforresterUpdate function-schemata sub-module to HEAD (5f40813)
repos/abstract-wiki/wikifunctions/wikilambda-cli!12sync-function-schematamainjforresterUpdate function-schemata sub-module to HEAD (5f40813)
repos/abstract-wiki/wikifunctions/function-orchestrator!35apine-sync-function-schematamainapineBREAKING CHANGE: Update function-schemata sub-module to HEAD (63aa93e)
repos/abstract-wiki/wikifunctions/function-orchestrator!31apine-Z885-builtinmainapineImplement builtin for errortype-to-type.
repos/abstract-wiki/wikifunctions/function-schemata!18apine-Z885-builtinmainapineSwitch Z985 to a built-in implementation.
repos/abstract-wiki/wikifunctions/function-orchestrator!30apine-eagerly-evaluate-resultmainapineDraft: Eagerly evaluate function return values.
repos/releng/gitlab-settings!24review/dancy/group-management-2maindancyGroup management script improvements
repos/releng/gitlab-settings!23review/dancy/group-managementmaindancyAdd group management scripts
repos/ci-tools/patchdemo!560github/fork/isaranto/feature-add-ores-extensionmastermatmarexfeat: add ORES extension to list of available extensions
repos/abstract-wiki/wikifunctions/function-orchestrator!6gerrit-823187mainjforresterUse heuristic type-checking instead of validation when resolving.
repos/phabricator/deployment!7work/2023-03-30-releasewmf/stablebrennenupdate submodules for other assignees, MR widget, SVN URI generation
repos/structured-data/image-suggestions!19T311289-combinedmaincparleSection image suggestions pipeline
repos/mwbot-rs/mwbot!24error-refactormainlegoktmRefactor and remove mwapi_errors
repos/data-engineering/airflow-dags!226disable-skein-log-collectionmainmilimetricDisable skein log collection by default
repos/releng/release!6work/catrope/add-vuetestmastercatropeStart branching VueTest
repos/data-engineering/airflow-dags!220work/ebernhardson/search-platform-cirrus-namespace-mapmainebernhardsonAdd initial evaluation dag for search-platform
Show related patches Customize query in GitLab

Related Objects

Event Timeline

flimport raised the priority of this task from to Medium.Sep 12 2014, 1:22 AM
flimport set Reference to fl56.

qgil wrote on 2014-04-22 18:00:41 (UTC)

Could you check whether the Phabricator API already provides what you need, or whether we would need to request something upstream, please?

epriestley wrote on 2014-04-25 22:27:52 (UTC)

We have feed.http-hooks (config, allows you to add webhooks) and feed.query (conduit API, for polling) but no socket-ish stream.

You can subclass PhabricatorSSHWorkflow to add a new SSH workflow as a local patch if you want to stick with stream-over-ssh, although I'd guess one of the above mechanisms is a better approach.

PhabricatorBotFeedNotificationHandler, which polls feed.query to push events through the chatbot, might be a good starting point if you want to poll.

qgil wrote on 2014-04-30 15:30:07 (UTC)

Moving to "Not critical for the RfC" because it is assumed that Phabricator will need to be plugged to our continuous integration process. If the RfC is approved, then we will need to define a technical plan before going anywhere.

Maybe we can skip Zuul altogether?

Harbormaster is marked as "prototype" today, but maybe by the time we need it, it suits our needs? In any case, after searching a bit it is clear that we are not the only ones and neither the first ones needing to connect Phabricator with Jenkins.

<hashar> CI is not that hard, though we need Phabricator to emit events
<hashar> and write some code in Zuul to understand those events
<qgil> hashar, or maybe we can skip Zuul altogether? See the couple of links I just posted at
<hashar> potentially
<hashar> if Phabricator has the utilities to reimplement all the logic we have in Zuul (very very unlikely)
<hashar> I am 99% sure Harbormaster does not match Zuul feature set :(
<qgil> hashar, are the features to match documented anywhere?
<hashar> the most important one is the ability to gate changes together which has a few pages documentation on
<hashar> if we have change A , B which are approved with a +2 , we trigger two set of jobs. The first set test A
<hashar> the second set test A + B together
<hashar> before A get merged. Zuul speculates that A will pass so it assumes it is fine to test B with A already being merged
<hashar> if A pass it is merged, and if A + B pass it is then merged. Ie you don't have to wait for A to be merged to run tests for B
<hashar> (and changes can be in different repositories)

Qgil renamed this task from Build a binding between Phabricator and Zuul to Connect Differential code review with continuous integration.Oct 9 2014, 2:38 PM
Qgil updated the task description. (Show Details)
Qgil removed a project: Wikimedia Phabricator RfC.
Qgil set Security to None.

Obviously, Antoine and Timo will be the people leading this decision. I haven't assigned it to them yet as (at least speaking for Antoine) we don't have time to commit to it right now.

Obviously! I don't think that Continuous Integration integration ;) is a key element of T560: Proof of concept of code review in Phabricator. It would be useful to have @hashar and @Krinkle in the loop to make sure that we are not making any decisions now that will hit us when someone starts working effectively on the CI part.

It would be also good to have a plan, which can be a rough plan now, and a bit more polished plan by the end of year. But still a plan. The actual work on CI is not expected to start before the checkpoint after the proof of concept and the blessing of the full code review migration plan at that point. Needless to say, such plan must take into consideration the availability of the people that needs to be involved for the tasks that we have agreed.

As Antoine mentioned, the plan to move away from Jenkins is one reason to keep an abstraction layer (e.g. middleware) between Phabricator and Zuul.

Another important reason is the need for security and authentication. Until we have sandboxed VM testing, it is crucial that only submitted code from trusted users is executed. Phabricator probably doesn't (and shouldn't have to) have a built-in mechanism for doing that. It would depend on the middleware instead (e.g. Zuul).

Qgil lowered the priority of this task from Medium to Low.Nov 27 2014, 7:57 AM

Currently #Code-Review is in the queue of Phabricator subprojects after Project-Management and Gitblit-Deprecate. If someone wants to work on this task, take it and assign the priority accordingly.

Flagging it with Continuous-Integration-Infrastructure, I think we will talk about it during our January 2015 team meeting / tech summit.

greg raised the priority of this task from Low to Medium.Nov 4 2015, 7:19 PM
greg moved this task from To Triage to Backlog on the Differential board.
greg renamed this task from Connect Differential code review with continuous integration to [keyresult] Connect Differential code review with continuous integration.Jan 20 2016, 12:15 AM

Just quoting this here so it's not lost. Regarding comments from Jenkins on build status:

One can send markup that would be interpreted by Phabricator. Ie icons from

OK{icon check color=green}
OK (non voting){icon info-circle color=green}
Failed (non voting){icon info-circle color=yellow}
Failed{icon ban color=red}
Whatever else{icon info-circle color=blue}
Luke081515 added a subscriber: Luke081515.

+1 to that, that would look nice ;)

@mmodell have you managed to get phabricator working with Zuul?

We are not using Zuul for now. Phabricator triggers jobs directly in Jenkins over its REST API.

@hashar but how do we view jobs like we could in Zuul please since I found it easy the way Zuul was layout. Since it showed all jobs running for that project. May we could get upstream to add the Zuul type showing projects with how long they have been running for and how long left and underneath it could show the tests running for that project like Zuul does including progress and linked to the job. Please.

Also how can we add jobs to phabricator for each repo. Like in Zuul we have layout.yaml which made it easy for people to add tests to there repo. We should probably do the same for phabricator please.

mmodell claimed this task.