Changing --milestone to use JSON source and adding prompt for branch mismatch.

drover.py

Index: drover.py
===================================================================
--- drover.py	(revision 197073)
+++ drover.py	(working copy)
@@ -4,10 +4,10 @@
 # found in the LICENSE file.
 
 import datetime
+import json
 import optparse
 import os
 import re
-import string
 import sys
 import urllib2
 import urlparse
@@ -375,60 +375,53 @@
 def getBranchForMilestone(milestone):
   """Queries omahaproxy.appspot.com for the branch number given |milestone|.
   """
-  OMAHA_PROXY_URL = "http://omahaproxy.appspot.com/all?csv=1"
-  request = urllib2.Request(OMAHA_PROXY_URL)
+  OMAHA_PROXY_URL = "http://omahaproxy.appspot.com/all?json=1"

[Isaac (away), 2013/05/03 10:00:50]

can we use https?

[Dan Beam, 2013/05/03 17:05:43]

Done.

   try:
-    response = urllib2.urlopen(request)
+    request = urllib2.urlopen(OMAHA_PROXY_URL)

[Isaac (away), 2013/05/03 10:00:50]

I think this variable would be aptly named 'response', as it is a string of the
response.

[Dan Beam, 2013/05/03 17:05:43]

I've renamed to response, but this is actually a file-like object, not a string,
that must be read() by json.load().  maybe you thought I was calling string
loading methods, json.loads(), instead (very close)?

   except urllib2.HTTPError, e:
     print "Failed to query %s: %d" % (OMAHA_PROXY_URL, e.code)
     return None
 
-  # Dictionary of [branch: major]. When searching for the appropriate branch
-  # matching |milestone|, all major versions that match are added to the
-  # dictionary. If all of the branches are the same, this branch value is
-  # returned; otherwise, the user is prompted to accept the largest branch
-  # value.
-  branch_dict = {}
+  # Response is in the form of:
+  # [{ os: "os_name", versions: [{ channel: "canary", true_branch: "1490" }] }]
+  response = json.load(request)

[Isaac (away), 2013/05/03 10:00:50]

nit: change 'response' to variable name that describes the dictionary.

[Dan Beam, 2013/05/03 17:05:43]

Done.

 
-  # Slice the first line since it's column information text.
-  for line in response.readlines()[1:]:
-    # Version data is CSV.
-    parameters = string.split(line, ',')
+  branches = {}

[Isaac (away), 2013/05/03 10:00:50]

import collections

...

branches = collections.defaultdict(list)

...
<remove 400 -402>
branches[branch].append(os_version['os'])

[Dan Beam, 2013/05/03 17:05:43]

Done.

+  for os_version in response:
+    for version in os_version['versions']:
+      if not version['true_branch'] or not version['version']:

[Isaac (away), 2013/05/03 10:00:50]

I don't think this check is necessary.

not version['true_branch'] will get caught in isdigit on line 398.
not version['version'] will get caught in 'not mstone' on line 395.

[Dan Beam, 2013/05/03 17:05:43]

I was getting None type, see below as to why this was popping

+        continue
+      mstone = version['version'].split('.')

[Dan Beam, 2013/05/03 17:05:43]

if version['version'] is None type this dies

+      if not mstone or mstone[0] != str(milestone):
+        continue
+      branch = version['true_branch']
+      if not branch.isdigit():

[Isaac (away), 2013/05/03 10:00:50]

this seems brittle, are they guaranteed to be ints?

[Dan Beam, 2013/05/03 17:05:43]

if version['true_branch'] is None type this dies

[Dan Beam, 2013/05/03 17:05:43]

asked laforge@, didn't get back to me, made a little looser (only
branch[0].isdigit()), this is to omit 'trunk'

+        continue
+      if not branch in branches:
+        branches[branch] = []
+      branches[branch] += [os_version['os']]
 

[Isaac (away), 2013/05/03 10:00:50]

check if branches is empty?

[Dan Beam, 2013/05/03 17:05:43]

Done.

-    # Version is the third parameter and consists of a quad of numbers separated
-    # by periods.
-    version = string.split(parameters[2], '.')
-    major = int(version[0], 10)
-    if major != milestone:
-      continue
+  if len(branches.keys()) == 1:

[Isaac (away), 2013/05/03 10:00:50]

len(branches)

[Dan Beam, 2013/05/03 17:05:43]

Done.

+    return branches.keys()[0]
 
-    # Branch number is the third value in the quad.
-    branch_dict[version[2]] = major
+  choices = ('-(%s): %s' % (b, ', '.join(o)) for b, o in branches.iteritems())
+  print >> sys.stderr, """
+Not all platforms have same branch number for M%d.
 
-  if not branch_dict:
-    # |milestone| not found.
-    print "Milestone provided is invalid"
-    return None
+Here's a list of platforms on each branch:
+%s""" % (milestone, '\n'.join(choices))
 
-  # The following returns a sorted list of the keys of |branch_dict|.
-  sorted_branches = sorted(branch_dict)
-  branch = sorted_branches[-1]
+  errors = 0
+  while errors < 3:
+    user_input = raw_input("Which branch? ('q' to cancel) ")

[Isaac (away), 2013/05/03 10:00:50]

do you need to strip trailing newline?

[Dan Beam, 2013/05/03 17:05:43]

Done. (no, I didn't need to in testing but it doesn't hurt)

+    if user_input in branches.keys():
+      return branch
+    if user_input.lower().startswith('q'):

[Isaac (away), 2013/05/03 10:00:50]

how about also abort on empty input

[Dan Beam, 2013/05/03 17:05:43]

I dislike this behavior as if you've pressed enter while a script is running
this will likely automatically fail here.

+      break
+    errors += 1
 
-  # If all keys match, the branch is the same for all platforms given
-  # |milestone|. This is the safe case, so return the branch.
-  if len(sorted_branches) == 1:
-    return branch
-
-  # Not all of the platforms have the same branch. Prompt the user and return
-  # the greatest (by value) branch on success.
-  if prompt("Not all platforms have the same branch number, "
-            "continue with branch %s?" % branch):
-    return branch
-
-  # User cancelled.
   return None
 
-
 def getSVNAuthInfo(folder=None):
   """Fetches SVN authorization information in the subversion auth folder and
   returns it as a dictionary of dictionaries."""