Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(107)

Issue 1155513002: [Rietveld] Add support for patchset dependencies (Closed)

Created:
5 years, 7 months ago by rmistry
Modified:
5 years, 6 months ago
Reviewers:
jrobbins
CC:
chromium-reviews, rmistry+cc_chromium.org
Base URL:
https://chromium.googlesource.com/infra/infra@master
Target Ref:
refs/heads/master
Project:
infra
Visibility:
Public.

Description

Add support for patchset dependencies. The following is a summary of my changes: * A new endpoint "get_depends_on_patchset" has been added. This endpoint will be called by apply_issue to form a chain of dependencies to apply to the local checkout. The apply_issue change is here: https://codereview.chromium.org/1149653002/. * I extracted majority of the new logic into a new module (dependency_utils) and added comprehensive unit tests. * This CL adds two new fields to the PatchSet model. These fields define the dependency and the dependents of the patchset. * The dependency and dependents of a patchset are displayed to the user but only in the new UI. I figure we are close enough to making the new UI the default that I can stop doing double UI work. * I tried to cover all interactions among dependent patchsets, here is a example of dependencies and how different Rietveld actions will impact those dependencies- -> CL3#PS2 CL1#PS2 -> CL2#PS1 -> CL3.1#PS1 -> CL3.1#PS2 Explanation of the above: CL3#PS2 & CL3.1#PS1 & CL3.1#PS2 all depend on CL2#PS1 which depends on CL1#PS2. ==Rietveld action: issue is closed and reopened== - If CL2 is closed then CL2#PS1 shows up with a strikethrough in CL3#PS2 & CL3.1#PS1 & CL3.1#PS2. - If CL2 is reopened then CL2#PS1 shows up normally (without a strikethrough) in CL3#PS2 & CL3.1#PS1 & CL3.1#PS2. ==Rietveld action: issue is deleted== - If CL2 is deleted then CL3#PS2 & CL3.1#PS1 & CL3.1#PS2 are updated to remove their dependency on CL2#PS1. Additionally, if any patchset of CL2 is a dependent then its dependency is removed. i.e. CL1#PS2 will be updated to remove CL2#PS1 as a dependent. ==Rietveld action: patchset is deleted== - If CL2#PS1 is deleted then CL3#PS2 & CL3.1#PS1 & CL3.1#PS2 are updated to remove their dependency on CL2#PS1. Additionally, if CL2#PS1 is a dependent then its dependency is removed. i.e. CL1#PS2 will be updated to remove CL2#PS1 as a dependent. Tested end-to-end using a test Git repository (https://skia.googlesource.com/skiabot-test/) and the following CLs created in my test Rietveld instance: * https://skia-codereview-staging.appspot.com/931002 ('Branch1 CL') * https://skia-codereview-staging.appspot.com/5001001 ('Branch2 CL') * https://skia-codereview-staging.appspot.com/9881001 ('Branch3 CL') * https://skia-codereview-staging.appspot.com/3951001 ('Branch3.1 CL') Opt into the new UI and observe the new 'Depends on Patchset' and 'Dependent Patchsets' sections in the above CLs. Design doc is here: https://docs.google.com/document/d/1KZGFKZpOPvco81sYVRCzwlnjGctup71RAzY0MSb0ntc/edit#heading=h.6r6lt4tsvssw BUG=502255 Committed: https://chromium.googlesource.com/infra/infra/+/be2d01f56f79233fb2fe216916469ac4c06c24a4

Patch Set 1 : Initial upload #

Patch Set 2 : dependent_issue -> dependent_patchset #

Patch Set 3 : Display depends on patchset (only in new UI) #

Patch Set 4 : Dependent -> DependsOn #

Patch Set 5 : Record and display dependent CLs #

Patch Set 6 : Cleanup #

Patch Set 7 : More cleanup #

Patch Set 8 : Update dependencies when issue is closed or reopened #

Patch Set 9 : Cleanup #

Patch Set 10 : Extract into module and add comprehensive unittests #

Patch Set 11 : Use the new patchset_dependency_utils module #

Patch Set 12 : Lint unittests #

Patch Set 13 : Update all patchsets in a CL when deleted or closed/re-opened #

Patch Set 14 : Add ability to update dependents #

Patch Set 15 : patchset_dependency_utils -> dependency_utils #

Patch Set 16 : Add unittest for remove_as_dependent #

Patch Set 17 : Remove buildbucket disabling code #

Patch Set 18 : Remove debugging stmts #

Patch Set 19 : Do not remove dependencies except for deletions and display closed issues with strikethrough #

Patch Set 20 : Do not return dependency if the issue is closed #

Patch Set 21 : Rebase #

Patch Set 22 : Make get_depends_on_patchset return JSON instead of text #

Total comments: 2

Patch Set 23 : Change link text #

Patch Set 24 : Rebase #

Patch Set 25 : Fix lint issues in test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+485 lines, -2 lines) Patch
A appengine/chromium_rietveld/codereview/dependency_utils.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +153 lines, -0 lines 0 comments Download
M appengine/chromium_rietveld/codereview/models.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -0 lines 0 comments Download
M appengine/chromium_rietveld/codereview/urls.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +2 lines, -0 lines 0 comments Download
M appengine/chromium_rietveld/codereview/views.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 8 chunks +46 lines, -2 lines 0 comments Download
M appengine/chromium_rietveld/new_static/components/cr-issue-patchset.css View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +20 lines, -0 lines 0 comments Download
M appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +29 lines, -0 lines 0 comments Download
M appengine/chromium_rietveld/new_static/model/patch_set.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +26 lines, -0 lines 0 comments Download
A appengine/chromium_rietveld/tests/test_dependency_utils.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +207 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (10 generated)
rmistry
5 years, 6 months ago (2015-06-08 14:05:55 UTC) #3
jrobbins
lgtm. Outstanding job on this. Thanks! https://codereview.chromium.org/1155513002/diff/430001/appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html File appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html (right): https://codereview.chromium.org/1155513002/diff/430001/appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html#newcode74 appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html:74: {{ patchset.dependsOnPatchset | ...
5 years, 6 months ago (2015-06-09 17:23:08 UTC) #4
rmistry
https://codereview.chromium.org/1155513002/diff/430001/appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html File appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html (right): https://codereview.chromium.org/1155513002/diff/430001/appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html#newcode74 appengine/chromium_rietveld/new_static/components/cr-issue-patchset.html:74: {{ patchset.dependsOnPatchset | getPatchsetUrl }} On 2015/06/09 17:23:08, Jason ...
5 years, 6 months ago (2015-06-10 12:43:30 UTC) #5
jrobbins
lgtm
5 years, 6 months ago (2015-06-10 19:28:30 UTC) #6
rmistry
On 2015/06/10 19:28:30, Jason Robbins wrote: > lgtm Jason, this CL only displays dependencies in ...
5 years, 6 months ago (2015-06-10 19:58:33 UTC) #7
Jason Robbins -- corp
On 2015/06/10 at 19:58:33, rmistry wrote: > On 2015/06/10 19:28:30, Jason Robbins wrote: > > ...
5 years, 6 months ago (2015-06-10 23:23:40 UTC) #8
rmistry
On 2015/06/10 23:23:40, Jason Robbins -- corp wrote: > On 2015/06/10 at 19:58:33, rmistry wrote: ...
5 years, 6 months ago (2015-06-11 20:23:25 UTC) #9
rmistry
On 2015/06/11 20:23:25, rmistry wrote: > On 2015/06/10 23:23:40, Jason Robbins -- corp wrote: > ...
5 years, 6 months ago (2015-06-19 16:40:24 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155513002/450001
5 years, 6 months ago (2015-06-19 16:41:00 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: infra_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_presubmit/builds/80)
5 years, 6 months ago (2015-06-19 16:45:01 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155513002/500001
5 years, 6 months ago (2015-06-19 17:03:12 UTC) #19
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 6 months ago (2015-06-19 17:06:29 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155513002/500001
5 years, 6 months ago (2015-06-19 17:07:07 UTC) #23
commit-bot: I haz the power
5 years, 6 months ago (2015-06-19 17:09:10 UTC) #24
Message was sent while issue was closed.
Committed patchset #25 (id:500001) as
https://chromium.googlesource.com/infra/infra/+/be2d01f56f79233fb2fe216916469...

Powered by Google App Engine
This is Rietveld 408576698