| = Gerrit Change Workflows, David Ostrovsky |
| :backend: slidy |
| :max-width: 70em |
| |
| [[title-page]] |
| |
| == Gerrit Change Workflows |
| === David Ostrovsky |
| ==== Gerrit User Conference |
| ===== Palo Alto, 2018 |
| |
| == Outline |
| |
| * Standard change workflow |
| * Custom change workflow |
| * Draft change workflow |
| * Work In Progress (WIP) workflow proposed by OpenStack project (2012) |
| * Allow to disable Draft change workflow in Gerrit core |
| * WIP plugin (2013) and its limitation |
| * Alternative implementations of WIP workflow in Gerrit core |
| * Gerrit adoption by Chromium project in mid 2017 |
| * WIP workflow details |
| * Replace Draft change workflow with WIP and Private Change workflows |
| * Migration from earlier Gerrit versions |
| |
| == Standard Gerrit change workflow / 1 |
| |
| === Pre-Submit code review workflow |
| |
| ---- |
| $ git push origin HEAD:refs/for/master |
| ---- |
| |
| image:./imgs/change_workflow_pre_submit.png[] |
| |
| == Standard Gerrit change workflow / 2 |
| |
| === Post-Submit code review workflow (supported since 2.14) |
| |
| ---- |
| $ git push origin sha1:refs/for/master%merged |
| ---- |
| |
| image:./imgs/change_workflow_post_submit.png[] |
| |
| == Custom change workflow / 1 |
| |
| * Gerrit doesn't provide (yet) workflow engine, now what? |
| |
| [role="incremental"] |
| |
| image:./imgs/change_workflow_custom.png[] |
| |
| == Custom change workflow / 2 |
| |
| * There is no supported way to do that directly with change states |
| ** We don't consider forking Gerrit as a viable option ;-) |
| |
| * Use issue tracker system, that supports custom workflows |
| ** Link Gerrit change to the corresponding issue |
| |
| == Custom change workflow / 3 |
| |
| * Write a plugin to track the custom workflow |
| * e.g.: verify-status plugin uses database to store verification details |
| |
| == Draft change workflow / 1 |
| |
| - Not yet ready changes |
| - Only visible for reviewers |
| |
| ---- |
| $ git push origin HEAD:refs/drafts/master |
| ---- |
| |
| image:./imgs/draft_change_workflow.png[] |
| |
| == Draft change workflow / 2 |
| |
| - Can draft change be abandoned and restored? |
| |
| [role="incremental"] |
| |
| - Yes, it could, but the DRAFT state would be lost: |
| + |
| image:./imgs/draft_change_abandoned_workflow.png[] |
| |
| == Draft change workflow / 3 |
| |
| - Add new change state ABANDONED_DRAFT to rectify: |
| |
| image:./imgs/draft_change_workflow_draft_abandoned.png[] |
| |
| == Draft patch set workflow |
| |
| ---- |
| $ git push origin HEAD:refs/for/master |
| <amend the commit> |
| $ git push origin HEAD:refs/drafts/master |
| ---- |
| |
| image:./imgs/draft_patch_set_workflow.png[] |
| |
| == Disadvantages of draft change workflow |
| |
| * Visibility constraint is orthogonal to Work In Progress attribute |
| * Once published, there is no way to flip DRAFT status again |
| ** this is needed when reviewers identifed issues, that need to be addressed |
| * User often consfused with draft change workflow: |
| ** draft change and draft patch set |
| ** deletion of draft changes vs. draft patch sets |
| |
| == Work In Progress (WIP) workflow from OpenStack project |
| |
| - WIP state suggested in 2012 by David Shrewsbury from Open Stack project |
| - link:https://gerrit-review.googlesource.com/c/gerrit/+/36091[Add Work In Progress state to Gerrit] |
| |
| image:./imgs/work_in_progress_workflow.png[] |
| |
| == Combined WIP workflow with DRAFT workflow |
| |
| - WIP and Draft change workflow are overlapping workflows: |
| |
| image:./imgs/draft_and_work_in_progress_workflow.png[] |
| |
| == Allow to disable draft workflow in Gerrit core |
| |
| * Add `change.allowDrafts` option to disable draft workflow |
| ** No upload of drafts per push to `refs/drafts/master` |
| ** No publish and deletion actions for a change |
| * As the consequence of this setting DRAFT change state is unused |
| |
| == WIP plugin: abuse DRAFT change state |
| |
| * WIP plugin is created in 2013, that implements WIP workflow |
| * Only usable when `change.allowDrafts` is set to `true` on gerrit site |
| |
| image:./imgs/work_in_progress_workflow_as_wip_plugin.png[] |
| |
| == WIP Plugin: Show regular change |
| |
| * Reviewers can see change on their dashboard: |
| |
| image:./imgs/changes_are_shown.png[] |
| |
| == WIP Plugin: Flip WIP bit on a change |
| |
| * Set `DRAFT = true` from wip-plugin's UI action: |
| |
| image:./imgs/mark_as_wip.png[] |
| |
| == WIP Plugin: WIP changes are filtered out on reviewer's dashboards |
| |
| * WIP change are hidden on reviewer's dashboards: |
| |
| image:./imgs/filtered_wip_changes.png[] |
| |
| == WIP Plugin: Flip ready bit on a change |
| |
| * Set `DRAFT = false` from wip-plugin's UI action: |
| |
| image:./imgs/mark_as_ready.png[] |
| |
| == Disadvantages of WIP plugin |
| |
| * Draft change workflow must be disabled |
| * Cannot upload a change as WIP |
| * Notifications for WIP changes: Firehose is not turned off |
| * Stream events are not implemented |
| |
| == Alternative considered to WIP workflow (in core or as a plugin) |
| |
| * Dedicated label can be used, with vote permission granted to change owner only |
| ** combined with customized dashboard to filter out negative votes on this label |
| * Abuse existing label |
| ** CRVW and interpret the blocking-Vote of change owner as virtual WIP state. |
| * Assignee workflow: assign change directly to owner or reviewer |
| * Change edits can be used to hide notifications, but there are disadvantages |
| ** Certain code review features aren't available on change edits: |
| *** Change edits cannot be shared with other reviewers |
| *** like review comments from CI or static analysis; |
| *** Updating change edit would override the existing ref |
| |
| == Gerrit adoption by Chromium project in mid 2017 |
| |
| * Poorer notification control compared to Rietveld: |
| ** link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4489[Issue 4489 Provide control over when a review starts and reviewers are notified] |
| ** link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4390[Issue 4390 Provide e-mail notification (notify section) for when review is requested] |
| ** link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4673[Issue 4673 Reduce too frequent email notifications to CC list and Reviewers] |
| |
| * Solution: WIP workflow to address these problems |
| |
| == Work In Progress workflow: Overview |
| |
| * link:https://gerrit-review.googlesource.com/c/gerrit/+/97245[Proposal: Work In Progress workflow] |
| * Software development is all about communication |
| ** Gerrit should streamline the communication process: |
| *** make the next action to take obvious for all participants |
| *** make it clear to a reviewer when their attention on a change is needed |
| *** without requiring the reviewer to scan the content of a change in order to make a judgment call |
| * Avoid spamming reviewers with not ready changes: |
| ** Notifications |
| ** Dashboards |
| |
| == WIP workflow: Implementation |
| * Use change attribute: "WIP" instead of change state "WIP" |
| |
| image:./imgs/work_in_progress_workflow_in_core.png[] |
| |
| == WIP workflow: Mark as work in progress - button |
| |
| image:./imgs/mark_as_wip_button.png[] |
| |
| == WIP workflow: Start Review - button |
| |
| image:./imgs/start_review_button.png[] |
| |
| == WIP workflow: Use "Save" button to publish comments without moving the change out of WIP |
| |
| image:./imgs/start_review_save_button.png[] |
| |
| ** The workflow really could use a major UX refresh |
| |
| == WIP workflow: ACL to flip the WIP state to ready |
| |
| * Change owner |
| * Gerrit Administrators |
| * Project Owners |
| |
| == WIP workflow: Push options |
| |
| * Push Option: |
| |
| ---- |
| $ git push -o wip origin HEAD:refs/for/master |
| $ git push origin HEAD:refs/for/master%wip |
| ---- |
| |
| == WIP workflow: Notification impact |
| |
| * Actions that create a new patch set in a WIP change default to notifying *OWNER* |
| * Actions that add a reviewer or CC to a WIP change default to notifying *NONE* |
| * Abandoning a WIP change defaults to notifying *OWNER_REVIEWERS* |
| * Reviewing a WIP change defaults to notifying *OWNER* |
| |
| == WIP workflow: Dashboard |
| |
| * There is a new section for *Work in progress* changes |
| ** The underlying query is for WIP changes owned by the user. |
| * The query driving the existing *Incoming reviews* section |
| ** substracts WIP changes from the results |
| |
| == WIP workflow: Search |
| |
| * WIP Change subtruction from `reviewer:self` predicate: |
| |
| [source,java] |
| ---- |
| if ("reviewer".equalsIgnoreCase(value)) { |
| return Predicate.and( |
| Predicate.not(new BooleanPredicate(ChangeField.WIP)), |
| ReviewerPredicate.reviewer(args, self())); |
| } |
| ---- |
| |
| * Feature requests: link:https://bugs.chromium.org/p/gerrit/issues/detail?id=9902[issue 9902]: allow search for WIP changes for specific reviewers |
| * Consider to change the `reviewer:self` predicate definition: |
| ---- |
| reviewer:self -is:wip |
| ---- |
| |
| == WIP workflow: Always push as WIP per user setting |
| |
| * Add user option to upload changes as WIP per default: |
| |
| image:./imgs/new_change_as_wip_per_default.png[] |
| |
| == WIP workflow: Always push as WIP per project setting |
| |
| * Add config option to upload changes for a project as WIP per default: |
| |
| ---- |
| [change] |
| workInProgressByDefault = true |
| ---- |
| |
| * Controls whether all new changes in the project are set as WIP by default. |
| * This setting can be overridden: |
| ** if the `workInProgress` field in ChangeInput entity is set to `false` explicitly when creating change per REST API |
| ** if the `ready` PushOption is used during the Git push. |
| + |
| * Default is `INHERIT`, which means that this property is inherited from |
| the parent project. |
| |
| == WIP workflow: Always mark as WIP for changes created in browser |
| |
| * Inline change implementation creates empty changes |
| * Empty changes are WIP per definition |
| |
| == Releasing WIP workflow |
| |
| * In addition to Draft change workflow or instead of Draft change workflow? |
| * Both workflows have similarities and differences |
| * Concluson: Discontinue Draft change worklfow |
| |
| == Discontinue Draft change workflow |
| |
| * WIP workflow: not readiness of the changes is orthogonal to the visibility constraints of Draft change workflow |
| * Draft change workflow cannot be replaced with WIP workflow |
| * Now what? |
| |
| == Private change workflow |
| |
| * Implement Private change workflow for visibility constraint' part of Draft change workflow: |
| |
| image:./imgs/work_in_progress_workflow_and_private_change_workflow_in_core.png[] |
| |
| == Migration strategy from earlier Gerrit versions to 2.15 (Schema_159) |
| |
| * Draft changes are migrated to WIP or Private changes |
| * Draft patch sets migrated to regular changes or private changes |
| * Default to WIP changes (this was flipped recently): |
| |
| ---- |
| [...] |
| Migrating data to schema 158 ... |
| > Done (0.000 s) |
| Migrating data to schema 159 ... |
| Migrate draft changes to private changes (default is work-in-progress) [y/N]? |
| Replace draft changes with work_in_progress changes ... |
| done |
| > Done (7.917 s) |
| [...] |
| ---- |
| |
| == Caution with Draft changes migration to Private changes strategy |
| |
| * Changes with Draft patch sets are migrated to private changes |
| ** Risk to mark substantial amount of merged and abandoned changes as |
| *`private` and thus make them non visible |
| * Workaround is to search for changes that were inadvertently marked as `private` and unmark them: |
| |
| ---- |
| owner:self is:private |
| ---- |
| |
| == Left over from removal of Draft change workflow |
| |
| * Because of dependency of some external tools `refs/drafts/master` is preserved |
| ** with the follow semantic: |
| |
| ---- |
| git push origin HEAD:refs/drafts/master # 1 |
| <amend 1> |
| git push origin HEAD:refs/drafts/master # 2 |
| <amend 2> |
| git push origin HEAD:refs/drafts/master # 3 |
| ---- |
| |
| * 1. Creates private change |
| * 2. Creates change edit |
| * 3. Overwrites change edit |
| |
| == Thank you |
| |
| *David Ostrovsky* |
| |
| Mainatainer, Gerrit Code Review |
| |
| ++++ |
| <style type="text/css"> |
| #title-page { |
| border-bottom: 0; |
| text-align: center; |
| position: relative; |
| top: 30%; |
| font-size: 60px; |
| } |
| </style> |
| ++++ |