refactor autoland/transplant

RESOLVED FIXED

Status

MozReview
Autoland
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: glob, Assigned: glob)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(12 attachments)

59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
59 bytes, text/x-review-board-request
gps
: review+
Details | Review
(Assignee)

Description

a year ago
i need to add functionality to autoland's transplant code for servo-vcs-sync.
this code could do with a tidy up to make it easier to modify.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8852910 [details]
autoland: refactor: fix logging (bug 1349997);

https://reviewboard.mozilla.org/r/125050/#review127730

::: autoland/autoland/autoland.py:36
(Diff revision 1)
>  MOZREVIEW_RETRY_DELAY = datetime.timedelta(minutes=5)
>  
>  # time to wait before retrying a transplant
>  TRANSPLANT_RETRY_DELAY = datetime.timedelta(minutes=5)
>  
> +logger = logging.getLogger('autoland')

Nit: typically one uses `__name__` for the logger name so messages for different modules can be distinguished. This can be annoying if writing the logger name in the output. But it's trivial to not do that. And, since this is a refactor, I suppose it is best to preserve existing behavior.
Attachment #8852910 - Flags: review?(gps) → review+

Comment 14

a year ago
mozreview-review
Comment on attachment 8852911 [details]
autoland: refactor: move transplant's hg_run (bug 1349997);

https://reviewboard.mozilla.org/r/125052/#review127732
Attachment #8852911 - Flags: review?(gps) → review+

Comment 15

a year ago
mozreview-review
Comment on attachment 8852912 [details]
autoland: refactor: split transplant into methods (bug 1349997);

https://reviewboard.mozilla.org/r/125054/#review127734

Nice refactor.

But, this was difficult to review because of the size. This could have been split up into several more commits to make review easier.
Attachment #8852912 - Flags: review?(gps) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8852913 [details]
autoland: refactor: consolidate transplant's run_hg_cmds (bug 1349997);

https://reviewboard.mozilla.org/r/125056/#review127736

This is much nicer!
Attachment #8852913 - Flags: review?(gps) → review+

Comment 17

a year ago
mozreview-review
Comment on attachment 8852914 [details]
autoland: refactor: change formulate_hg_error to custom Exception (bug 1349997);

https://reviewboard.mozilla.org/r/125058/#review127738
Attachment #8852914 - Flags: review?(gps) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8852915 [details]
autoload: refactor: create a Transplant class (bug 1349997);

https://reviewboard.mozilla.org/r/125060/#review127740

::: autoland/autoland/transplant.py:54
(Diff revision 1)
> +    def __init__(self):
> +        pass

This isn't strictly required since this is what the default implementation does. But I'm guessing you'll expand this later. So meh.
Attachment #8852915 - Flags: review?(gps) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8852916 [details]
autoland: refactor: change transplant from params to instance variables (bug 1349997);

https://reviewboard.mozilla.org/r/125062/#review127780
Attachment #8852916 - Flags: review?(gps) → review+

Comment 20

a year ago
mozreview-review
Comment on attachment 8852917 [details]
autoland: refactor: extract core transplant logic (bug 1349997);

https://reviewboard.mozilla.org/r/125064/#review127782
Attachment #8852917 - Flags: review?(gps) → review+

Comment 21

a year ago
mozreview-review
Comment on attachment 8852918 [details]
autoland: refactor: split transplant actions into methods (bug 1349997);

https://reviewboard.mozilla.org/r/125066/#review127802
Attachment #8852918 - Flags: review?(gps) → review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8852919 [details]
autoland: refactor: use exceptions to signal failure (bug 1349997);

https://reviewboard.mozilla.org/r/125068/#review127804
Attachment #8852919 - Flags: review?(gps) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8852920 [details]
autoland: refactor: inline hg commands (bug 1349997);

https://reviewboard.mozilla.org/r/125070/#review127808
Attachment #8852920 - Flags: review?(gps) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8852921 [details]
autoland: refactor: remove transplant wrapper function (bug 1349997);

https://reviewboard.mozilla.org/r/125072/#review127810

::: autoland/autoland/transplant.py:24
(Diff revision 1)
>          message = 'hg error in cmd: hg %s: %s' % (' '.join(hg_args),
>                                                    out.getvalue())
>          super(HgCommandError, self).__init__(message)
>  
>  
> -def transplant(tree, destination, rev, trysyntax=None,
> +class Transplant:

Please change to `class Transplant(object)`.
Attachment #8852921 - Flags: review?(gps) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

a year ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/474d46a4cfbf
autoland: refactor: fix logging ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/031eac6a57cb
autoland: refactor: move transplant's hg_run ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1bd04d39bf3c
autoland: refactor: split transplant into methods ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f7c98c03723b
autoland: refactor: consolidate transplant's run_hg_cmds ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d749480d2679
autoland: refactor: change formulate_hg_error to custom Exception ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/fbe527b3b67b
autoload: refactor: create a Transplant class ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/56908364d1ad
autoland: refactor: change transplant from params to instance variables ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7aaa7b98d7e0
autoland: refactor: extract core transplant logic ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f824a31c276f
autoland: refactor: split transplant actions into methods ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/ecb35624d757
autoland: refactor: use exceptions to signal failure ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d6a758cfdc87
autoland: refactor: inline hg commands ; r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/60d4ae1f2808
autoland: refactor: remove transplant wrapper function ; r=gps
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.