Reject changes with cleanspecs.
Cleanspecs must not be removed once they have been built. This means
they can't be reverted, or reliably cherry-picked. Just skip any
changes that include them since they make such a mess.
Change-Id: I3df8d81f93651d573485de7a75ecf5c6278c0001
diff --git a/tools/bionicbb/gerrit.py b/tools/bionicbb/gerrit.py
index 40719b4..9c62c6a 100644
--- a/tools/bionicbb/gerrit.py
+++ b/tools/bionicbb/gerrit.py
@@ -29,6 +29,12 @@
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.')
diff --git a/tools/bionicbb/gmail_listener.py b/tools/bionicbb/gmail_listener.py
index 770f0c4..3e501cc 100644
--- a/tools/bionicbb/gmail_listener.py
+++ b/tools/bionicbb/gmail_listener.py
@@ -19,6 +19,7 @@
import httplib2
import jenkinsapi
import json
+import os
import re
import requests
import termcolor
@@ -51,15 +52,35 @@
return headers
-def should_skip_message(info):
- if info['MessageType'] in ('newchange', 'newpatchset', 'comment'):
- commit = gerrit.get_commit(info['Change-Id'], info['PatchSet'])
- committer = commit['committer']['email']
- return not committer.endswith('@google.com')
- else:
- raise ValueError('should_skip_message() is only valid for new '
+def is_untrusted_committer(change_id, patch_set):
+ # TODO(danalbert): Needs to be based on the account that made the comment.
+ commit = gerrit.get_commit(change_id, patch_set)
+ committer = commit['committer']['email']
+ return not committer.endswith('@google.com')
+
+
+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 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 @@
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_message(gerrit_info):
+ if should_skip_build(gerrit_info):
return True
command_map = {
diff --git a/tools/bionicbb/test_gmail_listener.py b/tools/bionicbb/test_gmail_listener.py
index 6545cdc..af9eda0 100644
--- a/tools/bionicbb/test_gmail_listener.py
+++ b/tools/bionicbb/test_gmail_listener.py
@@ -3,61 +3,71 @@
import unittest
-class TestShouldSkipMessage(unittest.TestCase):
- def test_accepts_googlers(self):
+class TestShouldSkipBuild(unittest.TestCase):
+ @mock.patch('gmail_listener.contains_cleanspec')
+ @mock.patch('gerrit.get_commit')
+ def test_accepts_googlers(self, mock_commit, *other_checks):
+ mock_commit.return_value = {
+ 'committer': {'email': 'googler@google.com'}
+ }
+
+ for other_check in other_checks:
+ other_check.return_value = False
+
for message_type in ('newchange', 'newpatchset', 'comment'):
- with mock.patch('gerrit.get_commit') as mock_commit:
- mock_commit.return_value = {
- 'committer': {'email': 'googler@google.com'}
- }
+ self.assertFalse(gmail_listener.should_skip_build({
+ 'MessageType': message_type,
+ 'Change-Id': '',
+ 'PatchSet': '',
+ }))
- self.assertFalse(gmail_listener.should_skip_message({
- 'MessageType': message_type,
- 'Change-Id': '',
- 'PatchSet': '',
- }))
+ @mock.patch('gmail_listener.contains_cleanspec')
+ @mock.patch('gerrit.get_commit')
+ def test_rejects_googlish_domains(self, mock_commit, *other_checks):
+ mock_commit.return_value = {
+ 'committer': {'email': 'fakegoogler@google.com.fake.com'}
+ }
- def test_rejects_non_googlers(self):
+ for other_check in other_checks:
+ other_check.return_value = False
+
for message_type in ('newchange', 'newpatchset', 'comment'):
- with mock.patch('gerrit.get_commit') as mock_commit:
- mock_commit.return_value = {
- 'committer': {'email': 'fakegoogler@google.com.fake.com'}
- }
+ self.assertTrue(gmail_listener.should_skip_build({
+ 'MessageType': message_type,
+ 'Change-Id': '',
+ 'PatchSet': '',
+ }))
- self.assertTrue(gmail_listener.should_skip_message({
- 'MessageType': message_type,
- 'Change-Id': '',
- 'PatchSet': '',
- }))
+ @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'}
+ }
- with mock.patch('gerrit.get_commit') as mock_commit:
- mock_commit.return_value = {
- 'committer': {'email': 'johndoe@example.com'}
- }
+ for other_check in other_checks:
+ other_check.return_value = False
- self.assertTrue(gmail_listener.should_skip_message({
- 'MessageType': message_type,
- 'Change-Id': '',
- 'PatchSet': '',
- }))
-
- def test_calls_gerrit_get_commit(self): # pylint: disable=no-self-use
for message_type in ('newchange', 'newpatchset', 'comment'):
- with mock.patch('gerrit.get_commit') as mock_commit:
- gmail_listener.should_skip_message({
- 'MessageType': message_type,
- 'Change-Id': 'foo',
- 'PatchSet': 'bar',
- })
- mock_commit.assert_called_once_with('foo', 'bar')
+ self.assertTrue(gmail_listener.should_skip_build({
+ 'MessageType': message_type,
+ 'Change-Id': '',
+ 'PatchSet': '',
+ }))
- with mock.patch('gerrit.get_commit') as mock_commit:
- gmail_listener.should_skip_message({
- 'MessageType': message_type,
- 'Change-Id': 'baz',
- 'PatchSet': 'qux',
- })
- mock_commit.assert_called_once_with('baz', 'qux')
+ @mock.patch('gmail_listener.is_untrusted_committer')
+ @mock.patch('gerrit.get_files_for_revision')
+ def test_skips_cleanspecs(self, mock_files, *other_checks):
+ mock_files.return_value = ['foo/CleanSpec.mk']
+ 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': '',
+ }))
if __name__ == '__main__':
