Bug 51176 – sheriff-bot should be able to do multi-revision rollouts

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gabor Rapcsanyi]

The sheriff-bot should be able to do multi-revision rollouts.
The webkit-patch is ready for this after https://bugs.webkit.org/show_bug.cgi?id=33336

Eg.:
  sheriffbot rollout r123 r124 r125 Some reason

[Gabor Rapcsanyi]

Created attachment 76757 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 76757 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76757&action=review

Needs another round. :)

> WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py:64
> +        read_revision = True
> +        rollout_reason = []
> +        # the first argument must be a revision number
> +        svn_revision_list = [args[0].lstrip("r")]
> +        if not svn_revision_list[0].isdigit():
> +            read_revision = False
> +

Please split this (arg parsing, etc.) into private (_foo()) helper functions instead of just making execute longer.

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:58
> +            svn_revisions = " ".join([str(int(revision)) for revision in svn_revision_list])

Um, why do str(int(?

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:76
> +            svn_revisions,

Please pass them individually as numbers converted to strings, instead of as a joined string.

[Gabor Rapcsanyi]

Created attachment 77872 [details]
proposed_patch_v2

[Gabor Rapcsanyi]

(In reply to comment #2)
> (From update of attachment 76757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76757&action=review
> 
> Needs another round. :)
> 
> > WebKitTools/Scripts/webkitpy/tool/bot/irc_command.py:64
> > +        read_revision = True
> > +        rollout_reason = []
> > +        # the first argument must be a revision number
> > +        svn_revision_list = [args[0].lstrip("r")]
> > +        if not svn_revision_list[0].isdigit():
> > +            read_revision = False
> > +
> 
> Please split this (arg parsing, etc.) into private (_foo()) helper functions instead of just making execute longer.
> 

corrected

> > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:58
> > +            svn_revisions = " ".join([str(int(revision)) for revision in svn_revision_list])
> 
> Um, why do str(int(?
> 

Here I just check the revisions whether they are numbers and after I convert back to string for the concatenation.

> > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:76
> > +            svn_revisions,
> 
> Please pass them individually as numbers converted to strings, instead of as a joined string.

We have to pass the revisions between apostrophes or quotes to webkit-patch, and I think that's the best place to convert them to that format.

[Adam Barth]

Comment on attachment 77872 [details]
proposed_patch_v2

Looks great!

[Csaba Osztrogonác]

(In reply to comment #5)
> (From update of attachment 77872 [details])
> Looks great!

Landed in http://trac.webkit.org/changeset/75062

[Csaba Osztrogonác]

(In reply to comment #6)
> Landed in http://trac.webkit.org/changeset/75062

And a little fix landed in http://trac.webkit.org/changeset/75063