Closed Bug 1349997 Opened 7 years ago Closed 7 years ago

refactor autoland/transplant

Categories

(Conduit Graveyard :: Transplant, enhancement)

Product:
Conduit Graveyard
Component:
Transplant
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: glob, Assigned: glob)

Details

Attachments

(12 files)

autoland: refactor: fix logging (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: move transplant's hg_run (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: split transplant into methods (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: consolidate transplant's run_hg_cmds (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: change formulate_hg_error to custom Exception (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoload: refactor: create a Transplant class (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: change transplant from params to instance variables (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: extract core transplant logic (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: split transplant actions into methods (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: use exceptions to signal failure (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: inline hg commands (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details
autoland: refactor: remove transplant wrapper function (bug 1349997);
59 bytes, text/x-review-board-request
gps
: review+
Details 
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 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 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 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 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 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 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 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 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 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 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 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 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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Product: MozReview → Conduit
Product: Conduit → Conduit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: