Drover: Add --milestone option to merge to a specific milestone.

Drover: Add --milestone option to merge to a specific milestone.

Queries omahaproxy.appspot.com for related branch number.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109718

[laforge, 2011-11-09 05:06:12]

First, lemme say thank you for doing this implementation, very cool idea.  That
said, I'd recommend against it.

I threw in some code comments.  However, before you go further, it's probably
worth noting why I hadn't done this before.  If you look at omahaproxy today,
there are multiple entries for 17.0.*.* branches.  Since we cut daily Canary
builds and weekly Dev builds those tend to get out of sequence quickly and there
are multiple definitions of what M17 is.  The reason this is a challenge, is
that say on Monday of next week we are almost ready for Dev but we want a change
merged on to that morning's Canary, the current M17 branch... the script as
written will pick the  branch from the last Dev channel (i.e. the first CF entry
for 17) to merge to, as opposed to the Canary which maybe more appropriate (that
also maybe the wrong branch).  That scenario could be fixed, in theory by
specifying the OS to omahaproxy in the query, however the challenge there
becomes if we have a Dev channel release that we want to patch to w/ a greater
Canary channel.

To change the logic to pick by channel also has challenges because omahaproxy is
only updated after a release (i.e. it's sense of Dev/Beta/Stable will always
match the last push and not necessarily what's queued up).  This breaks workflow
because the branch for "dev" shifts.  It also become problematic as different
OSes (CrOS in particular) have different versions (Major level versions) on
their channels.

More problematic though, is that often times when we ask folks to merge to a
branch the branch hasn't been published yet (e.g.  say we have

http://codereview.chromium.org/8498038/diff/1/drover.py
File drover.py (right):

http://codereview.chromium.org/8498038/diff/1/drover.py#newcode378
drover.py:378: for line in response.readlines()[1:]:
This approach is a little dangerous, but will probably work.  The trouble is
that it will fire on the first match it sees (in this case the CF entries).  In
practice, this shouldn't get out of sync w/ Win/Mac/Linux, but it's not 100%
garunteed.

http://codereview.chromium.org/8498038/diff/1/drover.py#newcode383
drover.py:383: full_milestone = parameters[2]
It would probably be clearer to call this var version

http://codereview.chromium.org/8498038/diff/1/drover.py#newcode391
drover.py:391: return parameters[len(parameters) - 1]
It would be better here to return full_mstone.split('.')[-2].  The trouble w/
the true_branch column is that (using the following example) the real branch for
this line is 932 and not trunk, however this line as written would return the
word trunk.  Similarly the branches like 874_102 are typically one off branches
not meant to be merged to once they've been released once. 

cf,dev,17.0.932.0,17.0.928.0,11/08/11,11/03/11,108826,NA,99314,trunk

[James Hawkins, 2011-11-10 03:37:36]

I also made the adjustment to return the greatest (by value) branch if the
platforms don't match (gated by a prompt).

http://codereview.chromium.org/8498038/diff/1/drover.py
File drover.py (right):

http://codereview.chromium.org/8498038/diff/1/drover.py#newcode378
drover.py:378: for line in response.readlines()[1:]:
On 2011/11/09 05:06:12, laforge wrote:
> This approach is a little dangerous, but will probably work.  The trouble is
> that it will fire on the first match it sees (in this case the CF entries). 
In
> practice, this shouldn't get out of sync w/ Win/Mac/Linux, but it's not 100%
> garunteed.

There are two separate things we can do here:
* Default to 'win', since that's the platform likely to be right for 'Chrome'
(ChromeFrame is the most likely to diverge from other releases.)
* Provide an option to specify the platform.

I agree that this corner case will likely be extremely rare.

http://codereview.chromium.org/8498038/diff/1/drover.py#newcode383
drover.py:383: full_milestone = parameters[2]
On 2011/11/09 05:06:12, laforge wrote:
> It would probably be clearer to call this var version

Done.

http://codereview.chromium.org/8498038/diff/1/drover.py#newcode391
drover.py:391: return parameters[len(parameters) - 1]
On 2011/11/09 05:06:12, laforge wrote:
> It would be better here to return full_mstone.split('.')[-2].  The trouble w/
> the true_branch column is that (using the following example) the real branch
for
> this line is 932 and not trunk, however this line as written would return the
> word trunk.  Similarly the branches like 874_102 are typically one off
branches
> not meant to be merged to once they've been released once. 
> 
> cf,dev,17.0.932.0,17.0.928.0,11/08/11,11/03/11,108826,NA,99314,trunk

Done.

[laforge, 2011-11-11 21:06:47]

LGTM, if that setter works.

http://codereview.chromium.org/8498038/diff/2002/drover.py
File drover.py (right):

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode453
drover.py:453: options.branch = getBranchForMilestone(options.milestone)
I'm not sure that you'll be able to set this value directly, I believe these end
up being read only.

[James Hawkins, 2011-11-11 21:11:41]

http://codereview.chromium.org/8498038/diff/2002/drover.py
File drover.py (right):

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode453
drover.py:453: options.branch = getBranchForMilestone(options.milestone)
On 2011/11/11 21:06:47, laforge wrote:
> I'm not sure that you'll be able to set this value directly, I believe these
end
> up being read only.

Per-offline, manual tests show that this assignment sticks.

[M-A Ruel, 2011-11-11 21:23:23]

lgtm with a few optional comments

http://codereview.chromium.org/8498038/diff/2002/drover.py
File drover.py (right):

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode365
drover.py:365: 
another empty line

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode405
drover.py:405: sorted_branches = sorted(branch_dict.iterkeys())
sorted_branches = sorted(branch_dict)

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode421
drover.py:421: 
another empty line

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode454
drover.py:454: if options.branch is None:
if not options.branch:

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode615
drover.py:615: 
optional: maybe:
reference = bool(options.branch or options.milestone or options.milestone)

then you can use reference instead of the 3 or.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode618
drover.py:618: option_parser.error("--merge requires either --branch or
--local")
or --milestone ?

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode622
drover.py:622: option_parser.error("--local cannot be used with --revert or
--branch")
or --milestone

[James Hawkins, 2011-11-11 21:50:25]

http://codereview.chromium.org/8498038/diff/2002/drover.py
File drover.py (right):

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode365
drover.py:365: 
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> another empty line

Done.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode405
drover.py:405: sorted_branches = sorted(branch_dict.iterkeys())
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> sorted_branches = sorted(branch_dict)

Done.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode421
drover.py:421: 
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> another empty line

Done.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode454
drover.py:454: if options.branch is None:
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> if not options.branch:

Done.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode615
drover.py:615: 
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> optional: maybe:
> reference = bool(options.branch or options.milestone or options.milestone)
> 
> then you can use reference instead of the 3 or.

Done.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode618
drover.py:618: option_parser.error("--merge requires either --branch or
--local")
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> or --milestone ?

Done.

http://codereview.chromium.org/8498038/diff/2002/drover.py#newcode622
drover.py:622: option_parser.error("--local cannot be used with --revert or
--branch")
On 2011/11/11 21:23:24, Marc-Antoine Ruel wrote:
> or --milestone

Done.

[commit-bot: I haz the power, 2011-11-11 22:20:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jhawkins@chromium.org/8498038/12002

[commit-bot: I haz the power, 2011-11-11 22:26:23]

Change committed as 109718