Added TryJobGerritScheduler

scripts/master/try_job_gerrit.py

+# Copyright (c) 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.
+
+import json
+import re
+
+from twisted.internet import defer
+from twisted.python import log
+
+from buildbot.schedulers.base import BaseScheduler
+
+from master.gerrit_poller import GerritPoller
+
+
+class JobDefinition(object):

[Vadim Sh., 2014/04/29 01:22:45]

Consider using collections.namedtuple + factory function instead. It would give
you immutable class + good __repr__ for free.

[nodir, 2014/04/29 04:01:00]

Decided not to change this because I prefer
1) the parse method in the class
2) validation at the construction level
3) factory function to be a type (possible by inheriting from named tuple with
__new__, but complex)

+  """Describes a try job posted on Gerrit."""
+  def __init__(self, builder_names=None):
+    # Force str type and remove empty builder names.
+    self.builder_names = [str(b) for b in builder_names or []
+                          if b]

[Vadim Sh., 2014/04/29 01:22:45]

'if b' can probably fit on previous line
also wrap (builder_name or []) in () to better readability.

[nodir, 2014/04/29 04:01:00]

I am not sure it fits the rules
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...

It says, "Each portion must fit on one line: mapping expression, for clause,
filter expression."

[Vadim Sh., 2014/04/29 18:17:39]

Well, I can tell you we are not following this particular rule in tools/build/*
code base. It's better to be consistent with the existing code, than with the
rule no one follows... 

self.builder_names = [str(b) for b in (builder_names or []) if b]

+
+  def __repr__(self):
+    return repr(self.__dict__)
+
+  @staticmethod
+  def parse(text):
+    """Parses a try job definition."""
+    text = text and text.strip()
+    if not text:
+      # Return an empty definition.
+      return JobDefinition()

[Vadim Sh., 2014/04/29 01:22:45]

Is it valuable? What happens if it is scheduled?

[nodir, 2014/04/29 04:01:00]

Yes. The default list of builders is used

+
+    # Parse as json.
+    job = json.loads(text)
+
+    # Convert to canonical form.
+    if isinstance(job, list):
+      # Treat a list as builder name list.
+      job = {'builderNames': job}

[Vadim Sh., 2014/04/29 01:22:45]

elif not isinstance(job, dict):
  raise ValueError()

(or something like that).

[nodir, 2014/04/29 04:01:00]

Done.

+
+    return JobDefinition(job.get('builderNames'))
+
+
+class _TryJobGerritPoller(GerritPoller):
+  """Polls issues, creates changes and calls scheduler.submitJob.
+
+  This class is a part of TryJobGerritScheduler implementation and not designed
+  to be used otherwise.
+  """
+
+  change_category = 'tryjob'
+
+  MESSAGE_REGEX_TRYJOB = re.compile('Patch set \d+:\s+\!tryjob', re.I)

[Vadim Sh., 2014/04/29 01:22:45]

Each message contains 'Patch set XXX:' as a header? Hm...

[nodir, 2014/04/29 04:01:00]

It does
https://quickoffice-internal-review.googlesource.com/#/c/11081/1

+
+  def __init__(self, scheduler, gerrit_host, gerrit_projects=None,
+               pollInterval=None):
+    assert scheduler
+    GerritPoller.__init__(self, gerrit_host, gerrit_projects, pollInterval)
+    self.scheduler = scheduler
+
+  def _is_interesting_comment(self, comment):  # pylint: disable=R0201

[Vadim Sh., 2014/04/29 01:22:45]

Do you still need this pylint disable here?

[nodir, 2014/04/29 04:01:00]

Done.

+    return self.MESSAGE_REGEX_TRYJOB.match(comment['message'])
+
+  def getChangeQuery(self):
+    query = GerritPoller.getChangeQuery(self)
+    # Request only issues with TryJob=+1 label.
+    query += '+label:TryJob=%2B1'
+    return query
+
+  def parseJob(self, comment):
+    """Parses a JobDefinition from a Gerrit comment."""
+    msg = comment['message']
+    tryjob_match = self.MESSAGE_REGEX_TRYJOB.match(msg)
+    assert tryjob_match
+    job_def_str = msg[tryjob_match.end():]

[Vadim Sh., 2014/04/29 01:22:45]

Use tryjob_match.group(<index>) here instead of .end():. You would need to add
(.*) to regexp.

[nodir, 2014/04/29 04:01:00]

Done.

+    return JobDefinition.parse(job_def_str)
+
+  @defer.inlineCallbacks
+  def addChange(self, (change, comment)):
+    """Parses a job, adds a change and calls self.scheduler.submitJob."""
+    try:
+      job = self.parseJob(comment)
+      buildbotChange = yield self.addBuildbotChange(change, comment)
+      yield self.scheduler.submitJob(buildbotChange, job)
+      defer.returnValue(buildbotChange)
+    except Exception as e:
+      log.err('TryJobGerritPoller failed: %s' % e)

[Vadim Sh., 2014/04/29 01:22:45]

Do you need to reraise an exception? Won't caller be stuck waiting for results
(probably not..). Did you test this code path?

Also Exception is too generic... Do you know what exactly is caught by this
block? If so, I'd prefer more close-to-source except blocks. For example,
json.loads(...) can totally be wrapped in 'except ValueError' to handle json
deserialization errors.

[nodir, 2014/04/29 04:01:00]

added raise and tested. This prints a stack trace to the log and does NOT crash
the server.
I don't know what exception to expect. Since I've added 'raise', it's probably
fine now.

Now I raise ValueError if json.loads failed, see above

+
+
+class TryJobGerritScheduler(BaseScheduler):
+  """Polls try jobs on Gerrit and creates buildsets."""
+  def __init__(self, name, default_builder_names, gerrit_host,
+               gerrit_projects=None, pollInterval=None):
+    """Creates a new TryJobGerritScheduler.
+
+    Args:
+        name: name of the scheduler.
+        default_builder_names: a list of builder names used in case a job didn't
+            specify any.
+        gerrit_host: URL to the Gerrit instance
+        gerrit_projects: Gerrit projects to filter issues.
+        pollInterval: frequency of polling.
+    """
+    BaseScheduler.__init__(self, name,
+                           builderNames=default_builder_names,
+                           properties={})
+    self.poller = _TryJobGerritPoller(self, gerrit_host, gerrit_projects,
+                                      pollInterval)
+
+  def setServiceParent(self, parent):
+    BaseScheduler.setServiceParent(self, parent)
+    self.poller.master = self.master
+    self.poller.setServiceParent(self)
+
+  def gotChange(self, *args, **kwargs):
+    """Do nothing because changes are processed by submitJob."""
+
+  @defer.inlineCallbacks
+  def submitJob(self, change, job):
+    bsid = yield self.addBuildsetForChanges(
+        reason='tryjob',
+        changeids=[change.number],
+        builderNames=job.builder_names,
+        properties=change.properties)
+    log.msg('Successfully submitted a Gerrit try job for %s: %s.' %
+            (change.who, job))
+    defer.returnValue(bsid)