Closed
Bug 1220232
Opened 7 years ago
Closed 7 years ago
UI to confirm rewritten commit message
Categories
(MozReview Graveyard :: General, defect, P1)
MozReview Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcote, Assigned: glob)
References
Details
Attachments
(5 files, 4 obsolete files)
40 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
gps
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
dminor
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
mdoglio
:
review+
|
Details |
Since there is no universal standard for commit messages right now, we'll be using a best-effort approach to writing the reviewers in the commit message upon autolanding. We'll need to confirm the rewritten message with the user for each commit in the series (ideally all in one dialog). If the user rejects the rewritten message, autoland will not be performed, and the user will be instructed to either land manually or rewrite the commit message, push it back up to MozReview, and try autolanding again. We'll just need to confirm the first line, since that's all that should be rewritten. Eventually we can try doing something more interesting, like allowing the commit message to be edited directly, but for now we can use something simple.
Comment 1•7 years ago
|
||
Mercurial has facilities for opening an editor with content. We could rewrite the commit message and open the editor with that with some in-line comments telling them to fix any issues the auto rewriting may have incurred. This is how other Mercurial commands operate and should "just work."
(In reply to Gregory Szorc [:gps] from comment #1) > Mercurial has facilities for opening an editor with content. We could > rewrite the commit message and open the editor with that with some in-line > comments telling them to fix any issues the auto rewriting may have > incurred. This is how other Mercurial commands operate and should "just > work." the problem with that approach is autoland is triggered from the mozreview ui, not from the command line, so the confirmation needs to live in review board.
mozautomation: add reviewer replacing (bug 1220232); r?dminor Adds replace_reviewers, which will return the commit_description with all existing r= and r? flags replaced with r=reviewers.
Attachment #8688837 -
Flags: review?(dminor)
mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps Promote mozautomation from a raw library to an installable package. Update mozreview's ansible and test scripts to deploy mozautomation.
Attachment #8688838 -
Flags: review?(gps)
mozreview: move review helper methods out of hooks.py (bug 1220232); r?smacleod The review helper methods in hooks.py have utilitiy in other parts of review board. This patch creates a 'review_helper' package.
Attachment #8688839 -
Flags: review?(smacleod)
mozreview: add commit_rewrite api resource (bug 1220232); r?dminor Adds a webapi resource which returns the commit descriptions for the provided parent review with r=reviewer indicators. Existing r? and r= markers will be rewritting, or an r= will be appended to the first line if none were provided. This is used by autoland.
Attachment #8688840 -
Flags: review?(dminor)
mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?dminor Adds a confirmation dialog to the autoland menu item, which displays the rewritten commit messages. If all looks good the rewritten commit message is passed to autoland.
Attachment #8688841 -
Flags: review?(dminor)
(In reply to Byron Jones ‹:glob› from comment #6) > Created attachment 8688840 [details] > MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); > r?dminor this part is missing tests -- i'm still battling on that front.
Comment 9•7 years ago
|
||
Comment on attachment 8688837 [details] MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor https://reviewboard.mozilla.org/r/25435/#review22955 ::: pylib/mozautomation/mozautomation/commitparser.py:111 (Diff revision 1) > + reviewers = "r=" + ",".join(reviewers) nit: we seem to be using ' rather than " in this file, please change this function to be consistent. ::: pylib/mozautomation/mozautomation/commitparser.py:125 (Diff revision 1) > + commit_summary) This leaves any whitespace after a reviewer's name, e.g. replace_reviewers('bug 1 - blah r?dminor r?gps r?abc sr=abc', ['dminor', 'glob', 'gps', 'abc']) gives: 'bug 1 - blah r=dminor,glob,gps,abc sr=abc' ::: pylib/mozautomation/mozautomation/commitparser.py:129 (Diff revision 1) > + d = { 'first': True } You don't seem to use d as dictionary anywhere else, so just: first = True should be fine here. ::: pylib/mozautomation/mozautomation/commitparser.py:139 (Diff revision 1) > + commit_summary = REVIEWERS_RE.sub(replace_first_reviewer, commit_summary) Please trim any trailing whitespace here, just in case.
Attachment #8688837 -
Flags: review?(dminor)
Comment 10•7 years ago
|
||
Comment on attachment 8688840 [details] MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); r?dminor https://reviewboard.mozilla.org/r/25441/#review22965 ::: pylib/mozreview/mozreview/extension.py:53 (Diff revision 1) > +from mozreview.resources.commit_rewrite import (commit_rewrite_resource,) nit: please make this alphabetical and remove the parentheses around commit_rewrite_resource. ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:35 (Diff revision 1) > + if not is_parent(parent_request): This seems like a serious error to me. Please log something here. ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:38 (Diff revision 1) > + return DOES_NOT_EXIST PERMISSION_DENIED seems more suitable here ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:39 (Diff revision 1) > + if COMMITS_KEY not in parent_request.extra_data: This also seems like a serious problem, please log it as well. ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:52 (Diff revision 1) > + if child_request.approved: I think this is an error condition - we should not be asking for a rewrite on an unapproved review request. You can probably exit early with an error here. ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:63 (Diff revision 1) > + 'summary': description, This field should be called description rather than summary.
Attachment #8688840 -
Flags: review?(dminor)
Comment 11•7 years ago
|
||
Comment on attachment 8688841 [details] MozReview Request: mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?dminor https://reviewboard.mozilla.org/r/25443/#review22969 This lgtm, but you might want to flag mdoglio as well - I think he's more familiar with this code.
Attachment #8688841 -
Flags: review?(dminor)
Comment 12•7 years ago
|
||
Comment on attachment 8688838 [details] MozReview Request: mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps https://reviewboard.mozilla.org/r/25437/#review23001 You shouldn't need to check in the egg-info files: Python packaging tools will generate these from setup.py at package build time. Follow https://packaging.python.org/en/latest/distributing/ for the packaging best practices. ::: ansible/roles/docker-rbweb/tasks/main.yml:26 (Diff revision 1) > +- name: Install mozautomation library > + command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/mozautomation > + > - name: Install mozreview extension > command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/mozreview > > - name: Install rbbz extension > command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/rbbz > > - name: Install rbmotd extension > command: /venv/bin/python setup.py develop chdir=/version-control-tools/pylib/rbmotd Should probably be using with_items: here to reduce boilerplate.
Attachment #8688838 -
Flags: review?(gps)
Comment 13•7 years ago
|
||
https://reviewboard.mozilla.org/r/25439/#review23005 ::: pylib/mozreview/mozreview/review_helpers.py:1 (Diff revision 1) > +from __future__ import unicode_literals Need license header.
Assignee | ||
Comment 14•7 years ago
|
||
https://reviewboard.mozilla.org/r/25435/#review22955 > You don't seem to use d as dictionary anywhere else, so just: > > first = True > > should be fine here. this doesn't work - you'll get "local variable 'first' referenced before assignment" because assignments to names always go to the innermost scope.
Updated•7 years ago
|
Attachment #8688839 -
Flags: review?(smacleod) → review+
Comment 15•7 years ago
|
||
Comment on attachment 8688839 [details] MozReview Request: mozreview: move review helper methods out of hooks.py (bug 1220232); r?smacleod https://reviewboard.mozilla.org/r/25439/#review23083
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8688837 [details] MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25435/diff/1-2/
Attachment #8688837 -
Flags: review?(dminor)
Attachment #8688838 -
Attachment is obsolete: true
Attachment #8688839 -
Attachment is obsolete: true
Attachment #8688840 -
Attachment is obsolete: true
Attachment #8688841 -
Attachment is obsolete: true
Assignee | ||
Comment 17•7 years ago
|
||
unfortunately i managed to push a change that wiped out the issues from mozreview. addressing issues here instead: > ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:35 > (Diff revision 1) > > + if not is_parent(parent_request): > > This seems like a serious error to me. Please log something here. i don't agree that logging is required here. the consumer of this API should be able to pass in whatever it wants, and we should return an appropriate error code when required. logging of "hey, this isn't a parent request" belongs in the realm of the consumer, not the api. > ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:63 > (Diff revision 1) > > + 'summary': description, > > This field should be called description rather than summary. for consistency i followed our 'summary' api call from review_request_summary.py, which calls this field 'summary'.
Assignee | ||
Comment 18•7 years ago
|
||
mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps Promote mozautomation from a raw library to an installable package. Update mozreview's ansible and test scripts to deploy mozautomation.
Attachment #8690679 -
Flags: review?(gps)
Assignee | ||
Comment 19•7 years ago
|
||
mozreview: move review helper methods out of hooks.py (bug 1220232); r=smacleod The review helper methods in hooks.py have utilitiy in other parts of review board. This patch creates a 'review_helper' package.
Attachment #8690680 -
Flags: review?(smacleod)
Assignee | ||
Comment 20•7 years ago
|
||
mozreview: add commit_rewrite api resource (bug 1220232); r?dminor Adds a webapi resource which returns the commit descriptions for the provided parent review with r=reviewer indicators. Existing r? and r= markers will be rewritting, or an r= will be appended to the first line if none were provided. This is used by autoland.
Attachment #8690681 -
Flags: review?(dminor)
Assignee | ||
Comment 21•7 years ago
|
||
mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?mdoglio Adds a confirmation dialog to the autoland menu item, which displays the rewritten commit messages. If all looks good the rewritten commit message is passed to autoland.
Attachment #8690682 -
Flags: review?(mdoglio)
Comment 22•7 years ago
|
||
Comment on attachment 8690682 [details] MozReview Request: mozreview: add confirmation dialog to autoland with commit rewriting (bug 1220232); r?mdoglio https://reviewboard.mozilla.org/r/25911/#review23325
Attachment #8690682 -
Flags: review?(mdoglio) → review+
Comment 23•7 years ago
|
||
(In reply to Byron Jones ‹:glob› from comment #17) > unfortunately i managed to push a change that wiped out the issues from > mozreview. > > addressing issues here instead: > > > ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:35 > > (Diff revision 1) > > > + if not is_parent(parent_request): > > > > This seems like a serious error to me. Please log something here. > > i don't agree that logging is required here. the consumer of this API > should be able to pass in whatever it wants, and we should return an > appropriate error code when required. logging of "hey, this isn't a parent > request" belongs in the realm of the consumer, not the api. Ah sorry, I thought you were looking up the parent of the review request passed to the API so if that didn't end up being a parent request it would indicate some kind of data integrity error.
Comment 24•7 years ago
|
||
Comment on attachment 8690681 [details] MozReview Request: mozreview: add commit_rewrite api resource (bug 1220232); r?dminor https://reviewboard.mozilla.org/r/25909/#review23327 ::: pylib/mozreview/mozreview/resources/commit_rewrite.py:36 (Diff revision 1) > + if not is_parent(parent_request): Since this API expects a parent review request, you could look up the parent review request rather than returning an error. You could also then only return the rewritten commit description for the child that is requested. I'm not sure if that is worth doing now, maybe add a TODO?
Attachment #8690681 -
Flags: review?(dminor) → review+
Updated•7 years ago
|
Attachment #8688837 -
Flags: review?(dminor) → review+
Comment 25•7 years ago
|
||
Comment on attachment 8688837 [details] MozReview Request: mozautomation: add reviewer replacing (bug 1220232); r?dminor https://reviewboard.mozilla.org/r/25435/#review23329
Comment 26•7 years ago
|
||
Comment on attachment 8690679 [details] MozReview Request: mozautomation: add setup.py and add to reviewboard deployment (Bug 1220232). r?gps https://reviewboard.mozilla.org/r/25905/#review23437
Attachment #8690679 -
Flags: review?(gps) → review+
Comment 27•7 years ago
|
||
https://reviewboard.mozilla.org/r/25907/#review23439 Is this diff missing the changes to hooks.py? ::: pylib/mozreview/mozreview/review_helpers.py:5 (Diff revision 1) > +from __future__ import unicode_literals Please throw absolute_import in here as well.
Updated•7 years ago
|
Attachment #8690680 -
Flags: review?(smacleod) → review+
Comment 28•7 years ago
|
||
Comment on attachment 8690680 [details] MozReview Request: mozreview: move review helper methods out of hooks.py (bug 1220232); r=smacleod https://reviewboard.mozilla.org/r/25907/#review23721
Assignee | ||
Comment 29•7 years ago
|
||
https://hg.mozilla.org/hgcustom/version-control-tools/rev/ff5286dd10b3 https://hg.mozilla.org/hgcustom/version-control-tools/rev/c94a76673d3c https://hg.mozilla.org/hgcustom/version-control-tools/rev/f61035162306 https://hg.mozilla.org/hgcustom/version-control-tools/rev/5b0cc2971f25 https://hg.mozilla.org/hgcustom/version-control-tools/rev/c2620e01e208
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 30•7 years ago
|
||
Was hgext/reviewboard/tests/test-review-commit-rewrite.t never reviewed? I don't see it in the last revision of that commit in MozReview (https://reviewboard.mozilla.org/r/25909/). I noticed because you were missing a few (glob)s. :)
Assignee | ||
Comment 31•7 years ago
|
||
(In reply to Mark Côté [:mcote] from comment #30) > Was hgext/reviewboard/tests/test-review-commit-rewrite.t never reviewed? I > don't see it in the last revision of that commit in MozReview > (https://reviewboard.mozilla.org/r/25909/). it wasn't. it wasn't raised as an issue, and because i can't carry forward r+s i didn't want to waste anyone else's time on rubberstamping changes and further delay autoland. > I noticed because you were missing a few (glob)s. :) oops. thanks for fixing that (http://hg.mozilla.org/hgcustom/version-control-tools/rev/14b4fd0e4dbc).
Updated•7 years ago
|
Product: Developer Services → MozReview
You need to log in
before you can comment on or make changes to this bug.
Description
•