[bot_update] Remove support for rietveld patches.

R=agable@chromium.org, tandrii@chromium.org

Recipe-Nontrivial-Roll: build
Recipe-Nontrivial-Roll: infra
Change-Id: I518335e7b8fdce316b5566caccd3c4c7c2ae741a
Reviewed-on: https://chromium-review.googlesource.com/960161
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

recipes/README.recipes.md

@@ -48,27 +48,23 @@ Wrapper for easy calling of bot_update.
 
 — **def [apply\_gerrit\_ref](/recipes/recipe_modules/bot_update/api.py#45)(self, root, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, gerrit_repo=None, gerrit_ref=None, step_name='apply_gerrit', \*\*kwargs):**
 
-— **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#421)(self, bot_update_step):**
+— **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#393)(self, bot_update_step):**
 
 Deapplies a patch, taking care of DEPS and solution revisions properly.
     
 
-— **def [ensure\_checkout](/recipes/recipe_modules/bot_update/api.py#67)(self, gclient_config=None, suffix=None, patch=True, update_presentation=True, patch_root=None, no_shallow=False, with_branch_heads=False, with_tags=False, refs=None, patch_oauth2=False, oauth2_json=False, use_site_config_creds=None, clobber=False, root_solution_revision=None, rietveld=None, issue=None, patchset=None, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, disable_syntax_validation=False, manifest_name=None, \*\*kwargs):**
+— **def [ensure\_checkout](/recipes/recipe_modules/bot_update/api.py#67)(self, gclient_config=None, suffix=None, patch=True, update_presentation=True, patch_root=None, no_shallow=False, with_branch_heads=False, with_tags=False, refs=None, patch_oauth2=None, oauth2_json=None, use_site_config_creds=None, clobber=False, root_solution_revision=None, rietveld=None, issue=None, patchset=None, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, disable_syntax_validation=False, manifest_name=None, \*\*kwargs):**
 
 Args:
   gclient_config: The gclient configuration to use when running bot_update.
     If omitted, the current gclient configuration is used.
-  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.
   disable_syntax_validation: (legacy) Disables syntax validation for DEPS.
     Needed as migration paths for recipes dealing with older revisions,
     such as bisect.
   manifest_name: The name of the manifest to upload to LogDog.  This must
     be unique for the whole build.
 
-— **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#398)(self, project_name, gclient_config=None):**
+— **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#370)(self, project_name, gclient_config=None):**
 
 Returns all property names used for storing the checked-out revision of
 a given project.

recipes/recipe_modules/bot_update/__init__.py

@@ -30,8 +30,6 @@ PROPERTIES = {
   'event.patchSet.ref': Property(default=None, param_name='gerrit_ref'),
 
   # Rietveld-only (?) fields.
-  'issue': Property(default=None),
-  'patchset': Property(default=None),
   'repository': Property(default=None),
 
   # Common fields for both systems.

recipes/recipe_modules/bot_update/api.py

@@ -10,12 +10,12 @@ from recipe_engine import recipe_api
 
 class BotUpdateApi(recipe_api.RecipeApi):
 
-  def __init__(self, issue, patch_issue, patchset, patch_set,
+  def __init__(self, patch_issue, patch_set,
                repository, patch_repository_url, gerrit_ref, patch_ref,
                patch_gerrit_url, revision, parent_got_revision,
                deps_revision_overrides, fail_patch, *args, **kwargs):
-    self._issue = issue or patch_issue
-    self._patchset = patchset or patch_set
+    self._issue = patch_issue
+    self._patchset = patch_set
     self._repository = repository or patch_repository_url
     self._gerrit_ref = gerrit_ref or patch_ref
     self._gerrit = patch_gerrit_url
@@ -68,7 +68,7 @@ class BotUpdateApi(recipe_api.RecipeApi):
                       patch=True, update_presentation=True,
                       patch_root=None, no_shallow=False,
                       with_branch_heads=False, with_tags=False, refs=None,
-                      patch_oauth2=False, oauth2_json=False,
+                      patch_oauth2=None, oauth2_json=None,
                       use_site_config_creds=None, clobber=False,
                       root_solution_revision=None, rietveld=None, issue=None,
                       patchset=None, gerrit_no_reset=False,
@@ -79,10 +79,6 @@ class BotUpdateApi(recipe_api.RecipeApi):
     Args:
       gclient_config: The gclient configuration to use when running bot_update.
         If omitted, the current gclient configuration is used.
-      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.
       disable_syntax_validation: (legacy) Disables syntax validation for DEPS.
         Needed as migration paths for recipes dealing with older revisions,
         such as bisect.
@@ -91,6 +87,10 @@ class BotUpdateApi(recipe_api.RecipeApi):
     """
     assert use_site_config_creds is None, "use_site_config_creds is deprecated"
     assert rietveld is None, "rietveld is deprecated"
+    assert issue is None, "issue is deprecated"
+    assert patchset is None, "patchset is deprecated"
+    assert patch_oauth2 is None, "patch_oauth2 is deprecated"
+    assert oauth2_json is None, "oauth2_json is deprecated"
 
     refs = refs or []
     # We can re-use the gclient spec from the gclient module, since all the
@@ -99,8 +99,6 @@ class BotUpdateApi(recipe_api.RecipeApi):
     assert cfg is not None, (
         'missing gclient_config or forgot api.gclient.set_config(...) before?')
 
-    # Only one of these should exist.
-    assert not (oauth2_json and patch_oauth2)
 
     # Construct our bot_update command.  This basically be inclusive of
     # everything required for bot_update to know:
@@ -110,42 +108,19 @@ class BotUpdateApi(recipe_api.RecipeApi):
           self.m.properties.get('patch_project'), cfg, self._repository)
 
     if patch:
-      issue = issue or self._issue
       patchset = patchset or self._patchset
       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 never existed.
-      issue = patchset = None
       gerrit_repo = gerrit_ref = None
 
-    # Issue and patchset must come together.
-    if issue:
-      assert patchset
-    if patchset:
-      assert issue
-
     # The gerrit_ref and gerrit_repo must be together or not at all.  If one is
     # missing, clear both of them.
     if not gerrit_ref or not gerrit_repo:
       gerrit_repo = gerrit_ref = None
     assert (gerrit_ref != None) == (gerrit_repo != None)
-    if gerrit_ref:
-      # Gerrit patches have historically not specified issue and patchset.
-      # resourece/bot_update has as a result implicit assumption that set issue
-      # implies Rietveld patch.
-      # TODO(tandrii): fix this madness.
-      issue = patchset = None
-
-    # Point to the oauth2 auth files if specified.
-    # These paths are where the bots put their credential files.
-    oauth2_json_file = None
-    if oauth2_json:
-      if self.m.platform.is_win:
-        oauth2_json_file = 'C:\\creds\\refresh_tokens\\internal-try'
-      else:
-        oauth2_json_file = '/creds/refresh_tokens/internal-try'
 
     # Allow patch_project's revision if necessary.
     # This is important for projects which are checked out as DEPS of the
@@ -164,12 +139,9 @@ class BotUpdateApi(recipe_api.RecipeApi):
         ['--git-cache-dir', cfg.cache_dir],
         ['--cleanup-dir', self.m.path['cleanup'].join('bot_update')],
 
-        # How to find the patch, if any (issue/patchset).
-        ['--issue', issue],
-        ['--patchset', patchset],
+        # How to find the patch, if any
         ['--gerrit_repo', gerrit_repo],
         ['--gerrit_ref', gerrit_ref],
-        ['--apply_issue_oauth2_file', oauth2_json_file],
 
         # Hookups to JSON output back into recipes.
         ['--output_json', self.m.json.output()],

recipes/recipe_modules/bot_update/examples/full.expected/deprecated_got_revision_mapping.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.expected/tryjob.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.expected/tryjob_empty_revision.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch_download.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.expected/tryjob_v8.json

@@ -14,10 +14,6 @@
       "[GIT_CACHE]",
       "--cleanup-dir",
       "[CLEANUP]/bot_update",
-      "--issue",
-      "12345",
-      "--patchset",
-      "654321",
       "--output_json",
       "/path/to/tmp/json",
       "--revision",

recipes/recipe_modules/bot_update/examples/full.py

@@ -44,8 +44,6 @@ def RunSteps(api):
   with_branch_heads = api.properties.get('with_branch_heads', False)
   with_tags = api.properties.get('with_tags', False)
   refs = api.properties.get('refs', [])
-  oauth2 = api.properties.get('oauth2', False)
-  oauth2_json = api.properties.get('oauth2_json', False)
   root_solution_revision = api.properties.get('root_solution_revision')
   suffix = api.properties.get('suffix')
   gerrit_no_reset = True if api.properties.get('gerrit_no_reset') else False
@@ -75,8 +73,7 @@ def RunSteps(api):
         patch=patch,
         with_branch_heads=with_branch_heads,
         with_tags=with_tags,
-        refs=refs, patch_oauth2=oauth2,
-        oauth2_json=oauth2_json,
+        refs=refs,
         clobber=clobber,
         root_solution_revision=root_solution_revision,
         suffix=suffix,
@@ -134,22 +131,6 @@ def GenTests(api):
   yield api.test('trychange') + api.properties(
       refs=['+refs/change/1/2/333'],
   )
-  yield api.test('trychange_oauth2_buildbot') + api.properties(
-      path_config='buildbot',
-      oauth2=True,
-  )
-  yield api.test('trychange_oauth2_json') + api.properties(
-      mastername='tryserver.chromium.linux',
-      buildername='linux_rel',
-      slavename='totallyaslave-c4',
-      oauth2_json=True,
-  )
-  yield api.test('trychange_oauth2_json_win') + api.properties(
-      mastername='tryserver.chromium.win',
-      buildername='win_rel',
-      slavename='totallyaslave-c4',
-      oauth2_json=True,
-  ) + api.platform('win', 64)
   yield api.test('tryjob_fail') + api.properties(
       issue=12345,
       patchset=654321,

recipes/recipe_modules/bot_update/resources/bot_update.py

@@ -719,43 +719,6 @@ def _download(url):
         raise
 
 
-def apply_rietveld_issue(issue, patchset, root, _rev_map, _revision,
-                         oauth2_file, whitelist=None, blacklist=None):
-  apply_issue_bin = ('apply_issue.bat' if sys.platform.startswith('win')
-                     else 'apply_issue')
-  cmd = [apply_issue_bin,
-         # The patch will be applied on top of this directory.
-         '--root_dir', root,
-         # Tell apply_issue how to fetch the patch.
-         '--issue', issue,
-         '--server', 'codereview.chromium.org',
-         # Always run apply_issue.py, otherwise it would see update.flag
-         # and then bail out.
-         '--force',
-         # Don't run gclient sync when it sees a DEPS change.
-         '--ignore_deps',
-  ]
-  # Use an oauth key or json file if specified.
-  if oauth2_file:
-    cmd.extend(['--auth-refresh-token-json', oauth2_file])
-  else:
-    cmd.append('--no-auth')
-
-  if patchset:
-    cmd.extend(['--patchset', patchset])
-  if whitelist:
-    for item in whitelist:
-      cmd.extend(['--whitelist', item])
-  elif blacklist:
-    for item in blacklist:
-      cmd.extend(['--blacklist', item])
-
-  # Only try once, since subsequent failures hide the real failure.
-  try:
-    call(*cmd)
-  except SubprocessFailed as e:
-    raise PatchFailed(e.message, e.code, e.output)
-
 def apply_gerrit_ref(gerrit_repo, gerrit_ref, root, gerrit_reset,
                      gerrit_rebase_patch_ref):
   gerrit_repo = gerrit_repo or 'origin'
@@ -765,7 +728,7 @@ def apply_gerrit_ref(gerrit_repo, gerrit_ref, root, gerrit_reset,
   print '===Applying gerrit ref==='
   print 'Repo is %r @ %r, ref is %r, root is %r' % (
       gerrit_repo, base_rev, gerrit_ref, root)
-  # TODO(tandrii): move the fix below to common rietveld/gerrit codepath.
+  # TODO(tandrii): move the fix below to common gerrit codepath.
   # Speculative fix: prior bot_update run with Rietveld patch may leave git
   # index with unmerged paths. bot_update calls 'checkout --force xyz' thus
   # ignoring such paths, but potentially never cleaning them up. The following
@@ -867,20 +830,17 @@ def emit_json(out_file, did_run, gclient_output=None, **kwargs):
 
 
 def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
-                    target_cpu, patch_root, issue, patchset, gerrit_repo,
-                    gerrit_ref, gerrit_rebase_patch_ref, revision_mapping,
-                    apply_issue_oauth2_file, shallow, refs, git_cache_dir,
+                    target_cpu, patch_root, gerrit_repo, gerrit_ref,
+                    gerrit_rebase_patch_ref, shallow, refs, git_cache_dir,
                     cleanup_dir, gerrit_reset, disable_syntax_validation):
   # Get a checkout of each solution, without DEPS or hooks.
   # Calling git directly because there is no way to run Gclient without
   # invoking DEPS.
   print 'Fetching Git checkout'
 
-  git_ref = git_checkouts(solutions, revisions, shallow, refs, git_cache_dir,
-                          cleanup_dir)
+  git_checkouts(solutions, revisions, shallow, refs, git_cache_dir, cleanup_dir)
 
   print '===Processing patch solutions==='
-  already_patched = []
   patch_root = patch_root or ''
   applied_gerrit_patch = False
   print 'Patch root is %r' % patch_root
@@ -891,18 +851,14 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
       relative_root = solution['name'][len(patch_root) + 1:]
       target = '/'.join([relative_root, 'DEPS']).lstrip('/')
       print '  relative root is %r, target is %r' % (relative_root, target)
-      if issue:
-        apply_rietveld_issue(issue, patchset, patch_root, revision_mapping,
-                             git_ref, apply_issue_oauth2_file,
-                             whitelist=[target])
-        already_patched.append(target)
-      elif gerrit_ref:
+      if gerrit_ref:
         apply_gerrit_ref(gerrit_repo, gerrit_ref, patch_root, gerrit_reset,
                          gerrit_rebase_patch_ref)
         applied_gerrit_patch = True
 
   # Ensure our build/ directory is set up with the correct .gclient file.
-  gclient_configure(solutions, target_os, target_os_only, target_cpu, git_cache_dir)
+  gclient_configure(solutions, target_os, target_os_only, target_cpu,
+                    git_cache_dir)
 
   # Windows sometimes has trouble deleting files. This can make git commands
   # that rely on locks fail.
@@ -934,10 +890,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
     git('checkout', 'HEAD', '--', '.DEPS.git', cwd=first_sln)
 
   # Apply the rest of the patch here (sans DEPS)
-  if issue:
-    apply_rietveld_issue(issue, patchset, patch_root, revision_mapping, git_ref,
-                         apply_issue_oauth2_file, blacklist=already_patched)
-  elif gerrit_ref and not applied_gerrit_patch:
+  if gerrit_ref and not applied_gerrit_patch:
     # If gerrit_ref was for solution's main repository, it has already been
     # applied above. This chunk is executed only for patches to DEPS-ed in
     # git repositories.
@@ -947,7 +900,8 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
   # Reset the deps_file point in the solutions so that hooks get run properly.
   for sln in solutions:
     sln['deps_file'] = sln.get('deps_file', 'DEPS').replace('.DEPS.git', 'DEPS')
-  gclient_configure(solutions, target_os, target_os_only, target_cpu, git_cache_dir)
+  gclient_configure(solutions, target_os, target_os_only, target_cpu,
+                    git_cache_dir)
 
   return gclient_output
 
@@ -996,12 +950,6 @@ def parse_revisions(revisions, root):
 def parse_args():
   parse = optparse.OptionParser()
 
-  parse.add_option('--issue', help='Issue number to patch from.')
-  parse.add_option('--patchset',
-                   help='Patchset from issue to patch from, if applicable.')
-  parse.add_option('--apply_issue_oauth2_file',
-                   help='--auth-refresh-token-json option passthrough for '
-                        'apply_patch.py.')
   parse.add_option('--root', dest='patch_root',
                    help='DEPRECATED: Use --patch_root.')
   parse.add_option('--patch_root', help='Directory to patch on top of.')
@@ -1152,13 +1100,9 @@ def checkout(options, git_slns, specs, revisions, step_text, shallow):
 
           # Then, pass in information about how to patch.
           patch_root=options.patch_root,
-          issue=options.issue,
-          patchset=options.patchset,
           gerrit_repo=options.gerrit_repo,
           gerrit_ref=options.gerrit_ref,
           gerrit_rebase_patch_ref=not options.gerrit_no_rebase_patch_ref,
-          revision_mapping=options.revision_mapping,
-          apply_issue_oauth2_file=options.apply_issue_oauth2_file,
 
           # Finally, extra configurations such as shallowness of the clone.
           shallow=shallow,

tests/bot_update_coverage_test.py

@@ -149,13 +149,9 @@ class BotUpdateUnittests(unittest.TestCase):
       'target_os_only': None,
       'target_cpu': None,
       'patch_root': None,
-      'issue': None,
-      'patchset': None,
       'gerrit_repo': None,
       'gerrit_ref': None,
       'gerrit_rebase_patch_ref': None,
-      'revision_mapping': {},
-      'apply_issue_oauth2_file': None,
       'shallow': False,
       'refs': [],
       'git_cache_dir': '',

tests/gclient_smoketest.py

@@ -782,7 +782,7 @@ class GClientSmokeGIT(GClientSmokeBase):
     with open(output_deps) as f:
       deps_contents = f.read()
 
-    self.maxDiff = None
+    self.maxDiff = None  # pylint: disable=attribute-defined-outside-init
     self.assertEqual([
         'gclient_gn_args_file = "src/repo2/gclient.args"',
         'gclient_gn_args = [\'false_var\', \'false_str_var\', \'true_var\', '
@@ -958,7 +958,7 @@ class GClientSmokeGIT(GClientSmokeBase):
     with open(output_deps) as f:
       deps_contents = f.read()
 
-    self.maxDiff = None
+    self.maxDiff = None  # pylint: disable=attribute-defined-outside-init
     self.assertEqual([
         'gclient_gn_args_file = "src/repo2/gclient.args"',
         'gclient_gn_args = [\'false_var\', \'false_str_var\', \'true_var\', '
@@ -1247,7 +1247,7 @@ class GClientSmokeGIT(GClientSmokeBase):
     with open(output_deps) as f:
       deps_contents = f.read()
 
-    self.maxDiff = None
+    self.maxDiff = None  # pylint: disable=attribute-defined-outside-init
     self.assertEqual([
         'deps = {',
         '  # src',

tests/git_cl_test.py

@@ -388,7 +388,7 @@ class TestGitClBasic(unittest.TestCase):
         'Cr-Branched-From: somehash-refs/heads/master@{#12}')
 
   def test_git_number_ever_moooooooore_lineage(self):
-    self.maxDiff = 10000
+    self.maxDiff = 10000  # pylint: disable=attribute-defined-outside-init
     actual = self._test_git_number(
         'CQ commit on fresh new branch + numbering.\n'
         '\n'
@@ -618,7 +618,7 @@ class GitCookiesCheckerTest(TestCase):
       expected = f.read()
     def by_line(text):
       return [l.rstrip() for l in text.rstrip().splitlines()]
-    self.maxDiff = 10000
+    self.maxDiff = 10000  # pylint: disable=attribute-defined-outside-init
     self.assertEqual(by_line(sys.stdout.getvalue().strip()), by_line(expected))