bot_update: Rewrite to use properties, and add override options for patches.

recipe_modules/bot_update/api.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 
 """Recipe module to ensure a checkout is consistant on a bot."""
 
 from recipe_engine import recipe_api
 
 
@@ skipping 30 matching lines @@
       ret += ']'
     elif isinstance(spec, basestring):
       ret = repr(str(spec))
     else:
       ret = repr(spec)
   return ret
 
 
 class BotUpdateApi(recipe_api.RecipeApi):
 
-  def __init__(self, *args, **kwargs):
-      self._properties = {}
-      super(BotUpdateApi, self).__init__(*args, **kwargs)
+  def __init__(self, mastername, buildername, slavename, issue, patchset,

[szager1, 2016/03/10 21:28:16]

You added mandatory parameters, but did not change instantiations?  This is dark
magic.

[martiniss, 2016/03/10 21:40:46]

So this dark magic is so that people don't do raw api.m.properties['foobar']
dark magic. So we're roughly replacing one dark magic to another. 

And, no use actually instantiates this class. It's created by the recipe engine
code.

[iannucci, 2016/03/10 21:43:11]

these are filled by the explicit properties mechanism. Instead of having modules
implicitly change their behavior at runtime by probing api.properties, all of
the potentially behavior changing properties are added up front. This
constructor is only called by the engine, not user code. See the listing of
properties in __init__.py. These reflect the current reality, they're not
changing the behavior of this module.

The number of these properties needs to be whittled down over time, but we can't
fix everything in one CL.

+               patch_url, repository, gerrit_ref, rietveld, revision,
+               parent_got_revision, deps_revision_overrides, fail_patch,
+               *args, **kwargs):
+    self._mastername = mastername
+    self._buildername = buildername
+    self._slavename = slavename
+    self._issue = issue
+    self._patchset = patchset
+    self._patch_url = patch_url
+    self._repository = repository
+    self._gerrit_ref = gerrit_ref
+    self._rietveld = rietveld
+    self._revision = revision
+    self._parent_got_revision = parent_got_revision
+    self._deps_revision_overrides = deps_revision_overrides
+    self._fail_patch = fail_patch
+
+    self._last_returned_properties = {}
+    super(BotUpdateApi, self).__init__(*args, **kwargs)
 
   def __call__(self, name, cmd, **kwargs):
     """Wrapper for easy calling of bot_update."""
     assert isinstance(cmd, (list, tuple))
     bot_update_path = self.resource('bot_update.py')
     kwargs.setdefault('infra_step', True)
     kwargs.setdefault('env', {})
     kwargs['env'].setdefault('PATH', '%(PATH)s')
     kwargs['env']['PATH'] = self.m.path.pathsep.join([
         kwargs['env']['PATH'], str(self._module.PACKAGE_REPO_ROOT)])
     return self.m.python(name, bot_update_path, cmd, **kwargs)
 
   @property
-  def properties(self):
-      return self._properties
+  def last_returned_properties(self):
+      return self._last_returned_properties
 
   def ensure_checkout(self, gclient_config=None, suffix=None,
                       patch=True, update_presentation=True,
                       force=False, patch_root=None, no_shallow=False,
                       with_branch_heads=False, refs=None,
                       patch_project_roots=None, patch_oauth2=False,
                       output_manifest=True, clobber=False,
-                      root_solution_revision=None, **kwargs):
+                      root_solution_revision=None, rietveld=None, issue=None,

[szager1, 2016/03/10 21:28:16]

17 named parameters is really not very many.  Can you 8 or 9 more?

[martiniss, 2016/03/10 21:40:46]

:( I know.

[iannucci, 2016/03/10 21:43:11]

This is not an actionable or helpful comment. It's well known that this API has
too many parameters. The only reason that this is changing in this CL is because
YOU added these additional parameters.

+                      patchset=None, **kwargs):
+    """
+    Args:
+      gclient_config: The gclient configuration to use when running bot_update.
+        If omitted, the current gclient configuration is used.
+      rietveld: The rietveld server to use. If omitted, will infer from
+        the 'rietveld' property.
+      issue: The rietveld issue number to use. If omitted, will infer from
+        the 'issue' property.
+      patchset: The rietveld issue patchset to use. If omitted, will infer from
+        the 'patchset' property.
+      git_cache_dir: The directory to use as the git cache. If omitted, will use

[szager1, 2016/03/10 21:28:16]

This isn't actually an arg.

[martiniss, 2016/03/10 21:40:46]

Whoops, removed this but forgot the docs. Thanks.

+        the 'cache_dir' path in the path api module.
+    """
     refs = refs or []
     # We can re-use the gclient spec from the gclient module, since all the
     # data bot_update needs is already configured into the gclient spec.
     cfg = gclient_config or self.m.gclient.c
     spec_string = jsonish_to_python(cfg.as_jsonish(), True)
 
     # Used by bot_update to determine if we want to run or not.
-    master = self.m.properties['mastername']
-    builder = self.m.properties['buildername']
-    slave = self.m.properties['slavename']
+    master = self._mastername
+    builder = self._buildername
+    slave = self._slavename
 
     # Construct our bot_update command.  This basically be inclusive of
     # everything required for bot_update to know:
     root = patch_root
     if root is None:
       root = cfg.solutions[0].name
       additional = self.m.rietveld.calculate_issue_root(patch_project_roots)
       if additional:
         root = self.m.path.join(root, additional)
 
     if patch:
-      issue = self.m.properties.get('issue')
-      patchset = self.m.properties.get('patchset')
-      patch_url = self.m.properties.get('patch_url')
-      gerrit_repo = self.m.properties.get('repository')
-      gerrit_ref = self.m.properties.get('event.patchSet.ref')
+      issue = issue or self._issue
+      patchset = patchset or self._patchset
+      patch_url = self._patch_url
+      gerrit_repo = self._repository
+      gerrit_ref = self._gerrit_ref
     else:
       # The trybot recipe sometimes wants to de-apply the patch. In which case
       # we pretend the issue/patchset/patch_url never existed.
       issue = patchset = patch_url = email_file = key_file = None
       gerrit_repo = gerrit_ref = None
 
     # Issue and patchset must come together.
     if issue:
       assert patchset
     if patchset:
@@ skipping 23 matching lines @@
     flags = [
         # 1. Do we want to run? (master/builder/slave).
         ['--master', master],
         ['--builder', builder],
         ['--slave', slave],
 
         # 2. What do we want to check out (spec/root/rev/rev_map).
         ['--spec', spec_string],
         ['--root', root],
         ['--revision_mapping_file', self.m.json.input(rev_map)],
-        ['--git-cache-dir', self.m.path['git_cache']],
+        ['--git-cache-dir', cfg.cache_dir],
 
         # 3. How to find the patch, if any (issue/patchset/patch_url).
         ['--issue', issue],
         ['--patchset', patchset],
         ['--patch_url', patch_url],
-        ['--rietveld_server', self.m.properties.get('rietveld')],
+        ['--rietveld_server', rietveld or self._rietveld],
         ['--gerrit_repo', gerrit_repo],
         ['--gerrit_ref', gerrit_ref],
         ['--apply_issue_email_file', email_file],
         ['--apply_issue_key_file', key_file],
 
         # 4. Hookups to JSON output back into recipes.
         ['--output_json', self.m.json.output()],]
 
 
     # Collect all fixed revisions to simulate them in the json output.
     # Fixed revision are the explicit input revisions of bot_update.py, i.e.
     # every command line parameter "--revision name@value".
     fixed_revisions = {}
 
     revisions = {}
     for solution in cfg.solutions:
       if solution.revision:
         revisions[solution.name] = solution.revision
       elif solution == cfg.solutions[0]:
         revisions[solution.name] = (
-            self.m.properties.get('parent_got_revision') or
-            self.m.properties.get('revision') or
+            self._parent_got_revision or
+            self._revision or
             'HEAD')
     if self.m.gclient.c and self.m.gclient.c.revisions:
       revisions.update(self.m.gclient.c.revisions)
     if cfg.solutions and root_solution_revision:
       revisions[cfg.solutions[0].name] = root_solution_revision
     # Allow for overrides required to bisect into rolls.
-    revisions.update(self.m.properties.get('deps_revision_overrides', {}))
+    revisions.update(self._deps_revision_overrides)
     for name, revision in sorted(revisions.items()):
       fixed_revision = self.m.gclient.resolve_revision(revision)
       if fixed_revision:
         fixed_revisions[name] = fixed_revision
         flags.append(['--revision', '%s@%s' % (name, fixed_revision)])
 
     # Add extra fetch refspecs.
     for ref in refs:
       flags.append(['--refs', ref])
 
     # Filter out flags that are None.
     cmd = [item for flag_set in flags
            for item in flag_set if flag_set[1] is not None]
 
     if clobber:
       cmd.append('--clobber')
     if force:
       cmd.append('--force')
     if no_shallow:
       cmd.append('--no_shallow')
     if output_manifest:
       cmd.append('--output_manifest')
     if with_branch_heads or cfg.with_branch_heads:
       cmd.append('--with_branch_heads')
 
     # Inject Json output for testing.
-    git_mode = self.m.properties.get('mastername') not in SVN_MASTERS
+    git_mode = self._mastername not in SVN_MASTERS
     first_sln = cfg.solutions[0].name
     step_test_data = lambda: self.test_api.output_json(
         master, builder, slave, root, first_sln, rev_map, git_mode, force,
-        self.m.properties.get('fail_patch', False),
+        self._fail_patch,
         output_manifest=output_manifest, fixed_revisions=fixed_revisions)
 
     # Add suffixes to the step name, if specified.
     name = 'bot_update'
     if not patch:
       name += ' (without patch)'
     if suffix:
       name += ' - %s' % suffix
 
     # Ah hah! Now that everything is in place, lets run bot_update!
     try:
       # 87 and 88 are the 'patch failure' codes for patch download and patch
       # apply, respectively. We don't actually use the error codes, and instead
       # rely on emitted json to determine cause of failure.
       self(name, cmd, step_test_data=step_test_data,
            ok_ret=(0, 87, 88), **kwargs)
     finally:
       step_result = self.m.step.active_result
-      self._properties = step_result.json.output.get('properties', {})
+      self._last_returned_properties = step_result.json.output.get(
+          'properties', {})
 
       if update_presentation:
         # Set properties such as got_revision.
-        for prop_name, prop_value in self.properties.iteritems():
+        for prop_name, prop_value in self.last_returned_properties.iteritems():
           step_result.presentation.properties[prop_name] = prop_value
       # Add helpful step description in the step UI.
       if 'step_text' in step_result.json.output:
         step_text = step_result.json.output['step_text']
         step_result.presentation.step_text = step_text
       # Add log line output.
       if 'log_lines' in step_result.json.output:
         for log_name, log_lines in step_result.json.output['log_lines']:
           step_result.presentation.logs[log_name] = log_lines.splitlines()
 
@@ skipping 24 matching lines @@
 
       # bot_update actually just sets root to be the folder name of the
       # first solution.
       if step_result.json.output['did_run']:
         co_root = step_result.json.output['root']
         cwd = kwargs.get('cwd', self.m.path['slave_build'])
         if 'checkout' not in self.m.path:
           self.m.path['checkout'] = cwd.join(*co_root.split(self.m.path.sep))
 
     return step_result