From dadac10fccb1558fb00bbafc3fc4f6b3a20f9591 Mon Sep 17 00:00:00 2001 From: Dan Albert Date: Mon, 6 Apr 2015 12:43:55 -0700 Subject: 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 --- tools/bionicbb/gerrit.py | 6 ++ tools/bionicbb/gmail_listener.py | 40 +++++++++---- tools/bionicbb/test_gmail_listener.py | 102 +++++++++++++++++++--------------- 3 files changed, 92 insertions(+), 56 deletions(-) (limited to 'tools') 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 @@ 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.') 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 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): - 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 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_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 mock import unittest -class TestShouldSkipMessage(unittest.TestCase): - def test_accepts_googlers(self): - 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'} - } +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'} + } - self.assertFalse(gmail_listener.should_skip_message({ - 'MessageType': message_type, - 'Change-Id': '', - 'PatchSet': '', - })) + for other_check in other_checks: + other_check.return_value = False - def test_rejects_non_googlers(self): 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.assertFalse(gmail_listener.should_skip_build({ + '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'} + } + + for other_check in other_checks: + other_check.return_value = False - self.assertTrue(gmail_listener.should_skip_message({ - 'MessageType': message_type, - 'Change-Id': '', - 'PatchSet': '', - })) + 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_commit.return_value = { - 'committer': {'email': 'johndoe@example.com'} - } + @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({ - 'MessageType': message_type, - 'Change-Id': '', - 'PatchSet': '', - })) + for other_check in other_checks: + other_check.return_value = False - 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': '', + })) + + @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 - 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') + for message_type in ('newchange', 'newpatchset', 'comment'): + self.assertTrue(gmail_listener.should_skip_build({ + 'MessageType': message_type, + 'Change-Id': '', + 'PatchSet': '', + })) if __name__ == '__main__': -- cgit v1.1