bot_update: understand new Gerrit patch properties.

This change is backwards compatible, so no expectation changes expected in downstream rolls. Depends on https://codereview.chromium.org/2442173003 recipe_engine change. BUG=645616 R=martiniss@chromium.org,machenbach@chromium.org Review-Url: https://codereview.chromium.org/2439373002
9 years ago · 51a7b096ca
parent c1a668d883
commit 51a7b096ca
9 changed files with 186 additions and 15 deletions
--- a/recipe_modules/bot_update/init.py
+++ b/recipe_modules/bot_update/init.py
@ -15,14 +15,27 @@ from recipe_engine.recipe_api import Property
 from recipe_engine.types import freeze

 PROPERTIES = {
+  # Gerrit patches will have all properties about them prefixed with patch_.
+  'patch_issue': Property(default=None),  # TODO(tandrii): add kind=int.
+  'patch_set': Property(default=None),  # TODO(tandrii): add kind=int.
+  'patch_project': Property(default=None),  # Also used by Rietveld.
+  'patch_gerrit_url': Property(default=None),
+  'patch_repository_url': Property(default=None),
+  'patch_ref': Property(default=None),
+
+  # TODO(tAndrii): remove legacy Gerrit fields.
+  # Legacy Gerrit fields.
+  'event.patchSet.ref': Property(default=None, param_name='gerrit_ref'),
+
+  # Rietveld-only fields.
+  'rietveld': Property(default=None),  # Stores Url of Rietveld server.
  'issue': Property(default=None),
  'patchset': Property(default=None),
-  'patch_project': Property(default=None),
  'repository': Property(default=None),
-  'event.patchSet.ref': Property(default=None, param_name="gerrit_ref"),
-  'rietveld': Property(default=None),
-  'revision': Property(default=None),
-  'parent_got_revision': Property(default=None),
+
+  # Common fields for both systems.
  'deps_revision_overrides': Property(default=freeze({})),
  'fail_patch': Property(default=None, kind=str),
+  'parent_got_revision': Property(default=None),
+  'revision': Property(default=None),
 }
--- a/recipe_modules/bot_update/api.py
+++ b/recipe_modules/bot_update/api.py
@ -10,13 +10,15 @@ from recipe_engine import recipe_api

 class BotUpdateApi(recipe_api.RecipeApi):

-  def __init__(self, issue, patchset, repository, gerrit_ref, rietveld,
-               revision, parent_got_revision, deps_revision_overrides,
-               fail_patch, *args, **kwargs):
-    self._issue = issue
-    self._patchset = patchset
-    self._repository = repository
-    self._gerrit_ref = gerrit_ref
+  def __init__(self, issue, patch_issue, patchset, patch_set, patch_project,
+               repository, patch_repository_url, gerrit_ref, patch_ref,
+               patch_gerrit_url, rietveld, revision, parent_got_revision,
+               deps_revision_overrides, fail_patch, *args, **kwargs):
+    self._issue = issue or patch_issue
+    self._patchset = patchset or patch_set
+    self._repository = repository or patch_repository_url
+    self._gerrit_ref = gerrit_ref or patch_ref
+    self._gerrit = patch_gerrit_url
    self._rietveld = rietveld
    self._revision = revision
    self._parent_got_revision = parent_got_revision
@ -124,6 +126,12 @@ class BotUpdateApi(recipe_api.RecipeApi):
    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.
--- a/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle.json
+++ b/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle.json
@ -15,7 +15,7 @@
      "--gerrit_repo",
      "https://chromium.googlesource.com/angle/angle",
      "--gerrit_ref",
-      "refs/changes/11/338811/3",
+      "refs/changes/89/456789/12",
      "--output_json",
      "/path/to/tmp/json",
      "--revision",
--- a/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle_deprecated.json
+++ b/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle_deprecated.json
@ -0,0 +1,57 @@
+[
+  {
+    "cmd": [
+      "python",
+      "-u",
+      "RECIPE_MODULE[depot_tools::bot_update]/resources/bot_update.py",
+      "--spec",
+      "cache_dir = '[GIT_CACHE]'\nsolutions = [{'deps_file': '.DEPS.git', 'managed': True, 'name': 'src', 'url': 'https://chromium.googlesource.com/chromium/src.git'}]",
+      "--patch_root",
+      "src/third_party/angle",
+      "--revision_mapping_file",
+      "{\"src\": \"got_cr_revision\"}",
+      "--git-cache-dir",
+      "[GIT_CACHE]",
+      "--gerrit_repo",
+      "https://chromium.googlesource.com/angle/angle",
+      "--gerrit_ref",
+      "refs/changes/11/338811/3",
+      "--output_json",
+      "/path/to/tmp/json",
+      "--revision",
+      "src@HEAD",
+      "--revision",
+      "src/third_party/angle@HEAD"
+    ],
+    "env": {
+      "PATH": "%(PATH)s:RECIPE_PACKAGE_REPO[depot_tools]"
+    },
+    "name": "bot_update",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@Some step text@@@",
+      "@@@STEP_LOG_LINE@json.output@{@@@",
+      "@@@STEP_LOG_LINE@json.output@  \"did_run\": true, @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"fixed_revisions\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@    \"src\": \"HEAD\", @@@",
+      "@@@STEP_LOG_LINE@json.output@    \"src/third_party/angle\": \"HEAD\"@@@",
+      "@@@STEP_LOG_LINE@json.output@  }, @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"patch_failure\": false, @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"patch_root\": \"src/third_party/angle\", @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"properties\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@    \"got_cr_revision\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\", @@@",
+      "@@@STEP_LOG_LINE@json.output@    \"got_cr_revision_cp\": \"refs/heads/master@{#170242}\"@@@",
+      "@@@STEP_LOG_LINE@json.output@  }, @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"root\": \"src\", @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"step_text\": \"Some step text\"@@@",
+      "@@@STEP_LOG_LINE@json.output@}@@@",
+      "@@@STEP_LOG_END@json.output@@@",
+      "@@@SET_BUILD_PROPERTY@got_cr_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_cr_revision_cp@\"refs/heads/master@{#170242}\"@@@"
+    ]
+  },
+  {
+    "name": "$result",
+    "recipe_result": null,
+    "status_code": 0
+  }
+]
--- a/recipe_modules/bot_update/example.py
+++ b/recipe_modules/bot_update/example.py
@ -141,6 +141,11 @@ def GenTests(api):
  yield api.test('tryjob_v8_head_by_default') + api.properties.tryserver(
      patch_project='v8',
  )
-  yield api.test('tryjob_gerrit_angle') + api.properties.tryserver_gerrit(
-      full_project_name='angle/angle',
+  yield api.test('tryjob_gerrit_angle') + api.properties.tryserver(
+      gerrit_project='angle/angle',
+      patch_issue=338811,
+      patch_set=3,
+  )
+  yield (api.test('tryjob_gerrit_angle_deprecated') +
+         api.properties.tryserver_gerrit(full_project_name='angle/angle')
  )
--- a/recipe_modules/tryserver/api.py
+++ b/recipe_modules/tryserver/api.py
@ -41,6 +41,9 @@ class TryserverApi(recipe_api.RecipeApi):
  @property
  def is_gerrit_issue(self):
    """Returns true iff the properties exist to match a Gerrit issue."""
+    if self.m.properties.get('patch_storage') == 'gerrit':
+      return True
+    # TODO(tandrii): remove this, once nobody is using buildbot Gerrit Poller.
    return ('event.patchSet.ref' in self.m.properties and
            'event.change.url' in self.m.properties and
            'event.change.id' in self.m.properties)
--- a/recipe_modules/tryserver/example.expected/with_gerrit_patch.json
+++ b/recipe_modules/tryserver/example.expected/with_gerrit_patch.json
@ -0,0 +1,39 @@
+[
+  {
+    "cmd": [
+      "git",
+      "diff",
+      "--cached",
+      "--name-only"
+    ],
+    "cwd": "[SLAVE_BUILD]",
+    "name": "git diff to analyze patch",
+    "stdout": "/path/to/tmp/",
+    "~followup_annotations": [
+      "@@@STEP_LOG_LINE@files@foo.cc@@@",
+      "@@@STEP_LOG_END@files@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@"
+    ]
+  },
+  {
+    "cmd": [
+      "python",
+      "-u",
+      "import sys; sys.exit(1)"
+    ],
+    "name": "fail",
+    "~followup_annotations": [
+      "step returned non-zero exit code: 1",
+      "@@@STEP_TEXT@foo@@@",
+      "@@@STEP_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_hash@\"c2311ad770732eade3d2f48247abd147e40a70e7\"@@@"
+    ]
+  },
+  {
+    "name": "$result",
+    "reason": "Step('fail') failed with return_code 1",
+    "recipe_result": null,
+    "status_code": 1
+  }
+]
--- a/recipe_modules/tryserver/example.expected/with_gerrit_patch_deprecated.json
+++ b/recipe_modules/tryserver/example.expected/with_gerrit_patch_deprecated.json
@ -0,0 +1,39 @@
+[
+  {
+    "cmd": [
+      "git",
+      "diff",
+      "--cached",
+      "--name-only"
+    ],
+    "cwd": "[SLAVE_BUILD]",
+    "name": "git diff to analyze patch",
+    "stdout": "/path/to/tmp/",
+    "~followup_annotations": [
+      "@@@STEP_LOG_LINE@files@foo.cc@@@",
+      "@@@STEP_LOG_END@files@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@"
+    ]
+  },
+  {
+    "cmd": [
+      "python",
+      "-u",
+      "import sys; sys.exit(1)"
+    ],
+    "name": "fail",
+    "~followup_annotations": [
+      "step returned non-zero exit code: 1",
+      "@@@STEP_TEXT@foo@@@",
+      "@@@STEP_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_hash@\"c2311ad770732eade3d2f48247abd147e40a70e7\"@@@"
+    ]
+  },
+  {
+    "name": "$result",
+    "reason": "Step('fail') failed with return_code 1",
+    "recipe_result": null,
+    "status_code": 1
+  }
+]
--- a/recipe_modules/tryserver/example.py
+++ b/recipe_modules/tryserver/example.py
@ -65,6 +65,13 @@ def GenTests(api):
         api.properties.tryserver(test_patch_root='sub/project') +
         description_step)

+  yield (api.test('with_gerrit_patch_deprecated') +
+         api.properties.tryserver_gerrit(
+           full_project_name='infra/infra'))
+
+  yield (api.test('with_gerrit_patch') +
+         api.properties.tryserver(gerrit_project='infra/infra'))
+
  yield (api.test('with_wrong_patch_new') + api.platform('win', 32) +
         api.properties(test_patch_root='sub\\project'))