am 5cf46f81: Merge "Reject changes with cleanspecs."

* commit '5cf46f81ead958f43178ee8f613432b5f66045e5':
  Reject changes with cleanspecs.

tools/bionicbb/gerrit.py

@@ -29,6 +29,12 @@ def get_commit(change_id, revision):
         call('/changes/{}/revisions/{}/commit'.format(change_id, revision)))
 
 
+def get_files_for_revision(change_id, revision):
+    return json.loads(
+        call('/changes/{}/revisions/{}/files'.format(
+            change_id, revision))).keys()
+
+
 def call(endpoint, method='GET'):
     if method != 'GET':
         raise NotImplementedError('Currently only HTTP GET is supported.')

tools/bionicbb/gmail_listener.py

@@ -19,6 +19,7 @@ import httplib
 import httplib2
 import jenkinsapi
 import json
+import os
 import re
 import requests
 import termcolor
@@ -51,15 +52,35 @@ def get_headers(msg):
     return headers
 
 
-def should_skip_message(info):
+def is_untrusted_committer(change_id, patch_set):
-    if info['MessageType'] in ('newchange', 'newpatchset', 'comment'):
+    # TODO(danalbert): Needs to be based on the account that made the comment.
-        commit = gerrit.get_commit(info['Change-Id'], info['PatchSet'])
+    commit = gerrit.get_commit(change_id, patch_set)
     committer = commit['committer']['email']
     return not committer.endswith('@google.com')
-    else:
+
-        raise ValueError('should_skip_message() is only valid for new '
+
+def contains_cleanspec(change_id, patch_set):
+    files = gerrit.get_files_for_revision(change_id, patch_set)
+    return 'CleanSpec.mk' in [os.path.basename(f) for f in files]
+
+
+def should_skip_build(info):
+    if info['MessageType'] not in ('newchange', 'newpatchset', 'comment'):
+        raise ValueError('should_skip_build() is only valid for new '
                          'changes, patch sets, and commits.')
 
+    change_id = info['Change-Id']
+    patch_set = info['PatchSet']
+
+    checks = [
+        is_untrusted_committer,
+        contains_cleanspec,
+    ]
+    for check in checks:
+        if check(change_id, patch_set):
+            return True
+    return False
+
 
 def build_service():
     from apiclient.discovery import build
@@ -214,7 +235,7 @@ def build_project(gerrit_info, dry_run, lunch_target=None):
 
 
 def handle_change(gerrit_info, _, dry_run):
-    if should_skip_message(gerrit_info):
+    if should_skip_build(gerrit_info):
         return True
     return build_project(gerrit_info, dry_run)
 handle_newchange = handle_change
@@ -246,8 +267,7 @@ def handle_comment(gerrit_info, body, dry_run):
     if 'Verified+1' in body:
         drop_rejection(gerrit_info, dry_run)
 
-    # TODO(danalbert): Needs to be based on the account that made the comment.
+    if should_skip_build(gerrit_info):
-    if should_skip_message(gerrit_info):
         return True
 
     command_map = {

tools/bionicbb/test_gmail_listener.py

@@ -3,61 +3,71 @@ import mock
 import unittest
 
 
-class TestShouldSkipMessage(unittest.TestCase):
+class TestShouldSkipBuild(unittest.TestCase):
-    def test_accepts_googlers(self):
+    @mock.patch('gmail_listener.contains_cleanspec')
-        for message_type in ('newchange', 'newpatchset', 'comment'):
+    @mock.patch('gerrit.get_commit')
-            with mock.patch('gerrit.get_commit') as mock_commit:
+    def test_accepts_googlers(self, mock_commit, *other_checks):
         mock_commit.return_value = {
             'committer': {'email': 'googler@google.com'}
         }
 
-                self.assertFalse(gmail_listener.should_skip_message({
+        for other_check in other_checks:
+            other_check.return_value = False
+
+        for message_type in ('newchange', 'newpatchset', 'comment'):
+            self.assertFalse(gmail_listener.should_skip_build({
                 'MessageType': message_type,
                 'Change-Id': '',
                 'PatchSet': '',
             }))
 
-    def test_rejects_non_googlers(self):
+    @mock.patch('gmail_listener.contains_cleanspec')
-        for message_type in ('newchange', 'newpatchset', 'comment'):
+    @mock.patch('gerrit.get_commit')
-            with mock.patch('gerrit.get_commit') as mock_commit:
+    def test_rejects_googlish_domains(self, mock_commit, *other_checks):
         mock_commit.return_value = {
             'committer': {'email': 'fakegoogler@google.com.fake.com'}
         }
 
-                self.assertTrue(gmail_listener.should_skip_message({
+        for other_check in other_checks:
+            other_check.return_value = False
+
+        for message_type in ('newchange', 'newpatchset', 'comment'):
+            self.assertTrue(gmail_listener.should_skip_build({
                 'MessageType': message_type,
                 'Change-Id': '',
                 'PatchSet': '',
             }))
 
-            with mock.patch('gerrit.get_commit') as mock_commit:
+    @mock.patch('gmail_listener.contains_cleanspec')
+    @mock.patch('gerrit.get_commit')
+    def test_rejects_non_googlers(self, mock_commit, *other_checks):
         mock_commit.return_value = {
             'committer': {'email': 'johndoe@example.com'}
         }
 
-                self.assertTrue(gmail_listener.should_skip_message({
+        for other_check in other_checks:
+            other_check.return_value = False
+
+        for message_type in ('newchange', 'newpatchset', 'comment'):
+            self.assertTrue(gmail_listener.should_skip_build({
                 'MessageType': message_type,
                 'Change-Id': '',
                 'PatchSet': '',
             }))
 
-    def test_calls_gerrit_get_commit(self):  # pylint: disable=no-self-use
+    @mock.patch('gmail_listener.is_untrusted_committer')
-        for message_type in ('newchange', 'newpatchset', 'comment'):
+    @mock.patch('gerrit.get_files_for_revision')
-            with mock.patch('gerrit.get_commit') as mock_commit:
+    def test_skips_cleanspecs(self, mock_files, *other_checks):
-                gmail_listener.should_skip_message({
+        mock_files.return_value = ['foo/CleanSpec.mk']
-                    'MessageType': message_type,
+        for other_check in other_checks:
-                    'Change-Id': 'foo',
+            other_check.return_value = False
-                    'PatchSet': 'bar',
-                })
-            mock_commit.assert_called_once_with('foo', 'bar')
 
-            with mock.patch('gerrit.get_commit') as mock_commit:
+        for message_type in ('newchange', 'newpatchset', 'comment'):
-                gmail_listener.should_skip_message({
+            self.assertTrue(gmail_listener.should_skip_build({
                 'MessageType': message_type,
-                    'Change-Id': 'baz',
+                'Change-Id': '',
-                    'PatchSet': 'qux',
+                'PatchSet': '',
-                })
+            }))
-            mock_commit.assert_called_once_with('baz', 'qux')
 
 
 if __name__ == '__main__':