git cl try-results: add --wait-till-finished option.

git_cl.py

Index: git_cl.py
diff --git a/git_cl.py b/git_cl.py
index 18dc963407d46321b7f77937af2f9ed1de3fcfeb..53cf39fb6dbbee5988bbf06217b5231a9fc04fab 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -154,6 +154,12 @@ def time_sleep(seconds):
   return time.sleep(seconds)
 
 
+def time_time():
+  # Use this so that it can be mocked in tests without interfering with python
+  # system machinery.
+  import time  # Local import to discourage others from importing time globally.
+  return time.time()
+
 def ask_for_data(prompt):
   try:
     return raw_input(prompt)
@@ -458,6 +464,11 @@ def fetch_try_jobs(auth_config, changelist, buildbucket_host,
   return builds
 
 
+def _count_unfinished_tryjobs(jobs):
+  """Returns number of unfinished jobs from fetch_try_jobs result."""
+  return sum(b['status'] != 'COMPLETED' for b in jobs.itervalues())

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

perhaps 

len(b for b in jobs.itervalues() if b['status'] != 'COMPLETED')

is better than hiding implicit bool-to-int conversion here.

+
+
 def print_try_jobs(options, builds):
   """Prints nicely result of fetch_try_jobs."""
   if not builds:
@@ -4902,6 +4913,11 @@ def CMDtry_results(parser, args):
       help='Host of buildbucket. The default host is %default.')
   group.add_option(
       '--json', help='Path of JSON output file to write try job results to.')
+  group.add_option(
+      '-w', '--wait-till-finished', action='store_true', default=False,
+      help='Keep checking buildbucket until either all jobs finish. '

[Michael Achenbach, 2016/10/20 09:40:35]

nit: remove "either"?

+           'If after 1 hour jobs are still running, aborts and returns error '
+           'code 3.')
   parser.add_option_group(group)
   auth.add_auth_options(parser)
   options, args = parser.parse_args(args)
@@ -4929,15 +4945,32 @@ def CMDtry_results(parser, args):
             'By default, git cl try-results uses the latest patchset from '
             'codereview, continuing to use patchset %s.\n' %
             (patchset, cl.GetPatchset(), patchset))
-  try:
-    jobs = fetch_try_jobs(auth_config, cl, options.buildbucket_host, patchset)
-  except BuildbucketResponseException as ex:
-    print('Buildbucket error: %s' % ex)
-    return 1
-  if options.json:
-    write_try_results_json(options.json, jobs)
-  else:
-    print_try_jobs(options, jobs)
+
+  start_time = time_time()
+  while True:
+    try:
+      jobs = fetch_try_jobs(auth_config, cl, options.buildbucket_host, patchset)
+    except BuildbucketResponseException as ex:
+      print('Buildbucket error: %s' % ex)
+      return 1
+    if options.json:
+      write_try_results_json(options.json, jobs)

[Michael Achenbach, 2016/10/20 09:40:35]

Does this need to write the json in the loop? How about just once in the end?

Also: This prints everything in a loop, maybe only the remaining ones should be
printed? Or maybe use carriage return to keep modifying the same lines?

+    else:
+      print_try_jobs(options, jobs)
+    if not options.wait_till_finished:
+      return 0

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

Please "break", so that if someone chooses to add something later in this func,
then it will be executed

+
+    # Only if --wait-till-finished is specified.
+    unfinished = _count_unfinished_tryjobs(jobs)
+    if not unfinished:
+      return 0

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

ditto

+    passed = time_time() - start_time
+    print('After %.0f seconds waiting, %d jobs still running\n\n' %

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

Juse use %d instead of %.0f. It will round it to the next it... it's not the
same because %.0f will take floor operation, but AFAIK this doesn't matter here
and %d is simpler to understand.

+          (passed, unfinished))
+    if passed > 3600:

[Michael Achenbach, 2016/10/20 09:40:35]

Do you need a limit at all? Is this tool supposed to be usable by automated
infrastructure? If it's just intended for users maybe stay without limit and be
robust to using Ctrl-C?

[Sergiy Byelozyorov, 2016/10/20 11:44:43]

IMHO, this is useful to prevent users forgetting that they are running their
command in some screen shell and having it run for days causing unneeded load on
buildbucket.

+      print('Ran for too long, aborting now.')
+      return 3
+    time_sleep(10)  # Limit unnecessary load on buildbucket.

[Sergiy Byelozyorov, 2016/10/19 15:41:13]

If the option becomes widely popular, we may have a lot of developers running
this in parallel. A 1000 concurrently running commands with this option will
cause 100 requests per second. But OTH, IMHO paying for 0.1 requests per second
per developer is good value for the money, so this is probably reasonable. We
can probably revisit this later if it does indeed become that popular.

   return 0