From 72606f8c2402da51a92c91e324125e2026fcf457 Mon Sep 17 00:00:00 2001 From: Jochen Eisinger Date: Tue, 4 Apr 2017 10:44:18 +0200 Subject: [PATCH] Add support for a global status files for OWNERS This allows for having some global comments such as timezones or long-term unavailability. The comments go into build/OWNERS.status (that way, they should be available in all repos that map in build/ for the gn config files). The local can be overwritten in codereview.settings. The format is email: status Comments (starting with #) are allowed in that file, but they're ignored. BUG=694222 R=dpranke@chromium.org Change-Id: I49f58be87497d1ccaaa74f0a2f3d373403be44e7 Reviewed-on: https://chromium-review.googlesource.com/459542 Commit-Queue: Jochen Eisinger Reviewed-by: Dirk Pranke --- git_cl.py | 11 +++++--- owners.py | 53 ++++++++++++++++++++++++++++++++++--- owners_finder.py | 19 ++++++++++--- presubmit_support.py | 21 ++++++++++++++- tests/git_cl_test.py | 19 +++++++++++++ tests/owners_finder_test.py | 19 ++++++++++++- tests/owners_unittest.py | 10 +++++-- tests/presubmit_unittest.py | 12 ++++++--- 8 files changed, 147 insertions(+), 17 deletions(-) diff --git a/git_cl.py b/git_cl.py index d65ef68cc..f8c536a7e 100755 --- a/git_cl.py +++ b/git_cl.py @@ -3244,7 +3244,8 @@ class ChangeDescription(object): reviewers.append(name) if add_owners_tbr: owners_db = owners.Database(change.RepositoryRoot(), - fopen=file, os_path=os.path) + change.GetOwnersStatusFile(), + fopen=file, os_path=os.path) all_reviewers = set(tbr_names + reviewers) missing_files = owners_db.files_not_covered_by(change.LocalPaths(), all_reviewers) @@ -3463,6 +3464,9 @@ def LoadCodereviewSettingsFromFile(fileobj): SetProperty('run-post-upload-hook', 'RUN_POST_UPLOAD_HOOK', unset_error_ok=True) + if 'OWNERS_STATUS_FILE' in keyvals: + SetProperty('owners-status-file', 'OWNERS_STATUS_FILE', unset_error_ok=True) + if 'GERRIT_HOST' in keyvals: RunGit(['config', 'gerrit.host', keyvals['GERRIT_HOST']]) @@ -5512,8 +5516,9 @@ def CMDowners(parser, args): return owners_finder.OwnersFinder( [f.LocalPath() for f in cl.GetChange(base_branch, None).AffectedFiles()], - change.RepositoryRoot(), author, - fopen=file, os_path=os.path, + change.RepositoryRoot(), + change.GetOwnersStatusFile(), + author, fopen=file, os_path=os.path, disable_color=options.no_color).run() diff --git a/owners.py b/owners.py index 068e35b4c..2a44a9da9 100644 --- a/owners.py +++ b/owners.py @@ -68,6 +68,11 @@ EVERYONE = '*' BASIC_EMAIL_REGEXP = r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$' +# Key for global comments per email address. Should be unlikely to be a +# pathname. +GLOBAL_STATUS = '*' + + def _assert_is_collection(obj): assert not isinstance(obj, basestring) # Module 'collections' has no 'Iterable' member @@ -95,14 +100,16 @@ class Database(object): of changed files, and see if a list of changed files is covered by a list of reviewers.""" - def __init__(self, root, fopen, os_path): + def __init__(self, root, status_file, fopen, os_path): """Args: root: the path to the root of the Repository + status_file: the path relative to root to global status entries or None open: function callback to open a text file for reading os_path: module/object callback with fields for 'abspath', 'dirname', 'exists', 'join', and 'relpath' """ self.root = root + self.status_file = status_file self.fopen = fopen self.os_path = os_path @@ -196,6 +203,7 @@ class Database(object): return dirpath def load_data_needed_for(self, files): + self._read_global_comments() for f in files: dirpath = self.os_path.dirname(f) while not self._owners_for(dirpath): @@ -267,6 +275,44 @@ class Database(object): self._add_entry(dirpath, line, owners_path, lineno, ' '.join(comment)) + def _read_global_comments(self): + if not self.status_file: + return + + owners_status_path = self.os_path.join(self.root, self.status_file) + if not self.os_path.exists(owners_status_path): + raise IOError('Could not find global status file "%s"' % + owners_status_path) + + if owners_status_path in self.read_files: + return + + self.read_files.add(owners_status_path) + + lineno = 0 + for line in self.fopen(owners_status_path): + lineno += 1 + line = line.strip() + if line.startswith('#'): + continue + if line == '': + continue + + m = re.match('(.+?):(.+)', line) + if m: + owner = m.group(1).strip() + comment = m.group(2).strip() + if not self.email_regexp.match(owner): + raise SyntaxErrorInOwnersFile(owners_status_path, lineno, + 'invalid email address: "%s"' % owner) + + self.comments.setdefault(owner, {}) + self.comments[owner][GLOBAL_STATUS] = comment + continue + + raise SyntaxErrorInOwnersFile(owners_status_path, lineno, + 'cannot parse status entry: "%s"' % line.strip()) + def _add_entry(self, owned_paths, directive, owners_path, lineno, comment): if directive == 'set noparent': self._stop_looking.add(owned_paths) @@ -281,8 +327,9 @@ class Database(object): self._owners_to_paths.setdefault(owner, set()).add(owned_paths) self._paths_to_owners.setdefault(owned_paths, set()).add(owner) elif self.email_regexp.match(directive) or directive == EVERYONE: - self.comments.setdefault(directive, {}) - self.comments[directive][owned_paths] = comment + if comment: + self.comments.setdefault(directive, {}) + self.comments[directive][owned_paths] = comment self._owners_to_paths.setdefault(directive, set()).add(owned_paths) self._paths_to_owners.setdefault(owned_paths, set()).add(directive) else: diff --git a/owners_finder.py b/owners_finder.py index baca18eed..9fd011963 100644 --- a/owners_finder.py +++ b/owners_finder.py @@ -22,7 +22,7 @@ class OwnersFinder(object): indentation = 0 - def __init__(self, files, local_root, author, + def __init__(self, files, local_root, owners_status_file, author, fopen, os_path, email_postfix='@chromium.org', disable_color=False): @@ -34,7 +34,8 @@ class OwnersFinder(object): self.COLOR_GREY = '' self.COLOR_RESET = '' - self.db = owners_module.Database(local_root, fopen, os_path) + self.db = owners_module.Database(local_root, owners_status_file, fopen, + os_path) self.db.load_data_needed_for(files) self.os_path = os_path @@ -51,7 +52,8 @@ class OwnersFinder(object): if len(filtered_files) != len(files): files = filtered_files # Reload the database. - self.db = owners_module.Database(local_root, fopen, os_path) + self.db = owners_module.Database(local_root, owners_status_file, fopen, + os_path) self.db.load_data_needed_for(files) self.all_possible_owners = self.db.all_possible_owners(files, None) @@ -207,8 +209,17 @@ class OwnersFinder(object): else: self.writeln(self.bold_name(owner) + ' is commented as:') self.indent() + if owners_module.GLOBAL_STATUS in self.comments[owner]: + self.writeln( + self.greyed(self.comments[owner][owners_module.GLOBAL_STATUS]) + + ' (global status)') + if len(self.comments[owner]) == 1: + self.unindent() + return for path in self.comments[owner]: - if len(self.comments[owner][path]) > 0: + if path == owners_module.GLOBAL_STATUS: + continue + elif len(self.comments[owner][path]) > 0: self.writeln(self.greyed(self.comments[owner][path]) + ' (at ' + self.bold(path or '') + ')') else: diff --git a/presubmit_support.py b/presubmit_support.py index 08c4774a3..087b3e4e2 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -418,10 +418,12 @@ class InputApi(object): # We carry the canned checks so presubmit scripts can easily use them. self.canned_checks = presubmit_canned_checks + # TODO(dpranke): figure out a list of all approved owners for a repo # in order to be able to handle wildcard OWNERS files? self.owners_db = owners.Database(change.RepositoryRoot(), - fopen=file, os_path=self.os_path) + change.GetOwnersStatusFile(), + fopen=file, os_path=self.os_path) self.verbose = verbose self.Command = CommandData @@ -916,6 +918,10 @@ class Change(object): x for x in self.AffectedFiles(include_deletes=False) if x.IsTestableFile()) + def GetOwnersStatusFile(self): + """Returns the name of the global OWNERS status file.""" + return None + class GitChange(Change): _AFFECTED_FILES = GitAffectedFile @@ -927,6 +933,19 @@ class GitChange(Change): return subprocess.check_output( ['git', 'ls-files', '--', '.'], cwd=root).splitlines() + def GetOwnersStatusFile(self): + """Returns the name of the global OWNERS status file.""" + + try: + status_file = subprocess.check_output( + ['git', 'config', 'rietveld.owners-status-file'], + cwd=self.RepositoryRoot()) + return status_file + except subprocess.CalledProcessError: + pass + + return None + def ListRelevantPresubmitFiles(files, root): """Finds all presubmit files that apply to a given set of source files. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index f5cbb7213..cfae719a5 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -684,6 +684,25 @@ class TestGitCl(TestCase): ] self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file)) + def test_LoadCodereviewSettingsFromFile_owners_status(self): + codereview_file = StringIO.StringIO('OWNERS_STATUS_FILE: status') + self.calls = [ + ((['git', 'config', '--unset-all', 'rietveld.server'],), ''), + ((['git', 'config', '--unset-all', 'rietveld.cc'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.private'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.tree-status-url'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.viewvc-url'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.bug-prefix'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.cpplint-regex'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.cpplint-ignore-regex'],), + CERR1), + ((['git', 'config', '--unset-all', 'rietveld.project'],), CERR1), + ((['git', 'config', '--unset-all', 'rietveld.run-post-upload-hook'],), + CERR1), + ((['git', 'config', 'rietveld.owners-status-file', 'status'],), ''), + ] + self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file)) + @classmethod def _is_gerrit_calls(cls, gerrit=False): return [((['git', 'config', 'rietveld.autoupdate'],), ''), diff --git a/tests/owners_finder_test.py b/tests/owners_finder_test.py index 1d9e08366..2933b1b19 100755 --- a/tests/owners_finder_test.py +++ b/tests/owners_finder_test.py @@ -21,6 +21,7 @@ import owners ben = 'ben@example.com' brett = 'brett@example.com' darin = 'darin@example.com' +jochen = 'jochen@example.com' john = 'john@example.com' ken = 'ken@example.com' peter = 'peter@example.com' @@ -41,6 +42,7 @@ def test_repo(): return filesystem_mock.MockFileSystem(files={ '/DEPS': '', '/OWNERS': owners_file(ken, peter, tom), + '/build/OWNERS.status': '%s: bar' % jochen, '/base/vlog.h': '', '/chrome/OWNERS': owners_file(ben, brett), '/chrome/browser/OWNERS': owners_file(brett), @@ -57,6 +59,10 @@ def test_repo(): '/content/baz/froboz.h': '', '/content/baz/ugly.cc': '', '/content/baz/ugly.h': '', + '/content/common/OWNERS': owners_file(jochen), + '/content/common/common.cc': '', + '/content/foo/OWNERS': owners_file(jochen, comment='foo'), + '/content/foo/foo.cc': '', '/content/views/OWNERS': owners_file(ben, john, owners.EVERYONE, noparent=True), '/content/views/pie.h': '', @@ -67,7 +73,7 @@ class OutputInterceptedOwnersFinder(owners_finder.OwnersFinder): def __init__(self, files, local_root, fopen, os_path, disable_color=False): super(OutputInterceptedOwnersFinder, self).__init__( - files, local_root, None, + files, local_root, os_path.join('build', 'OWNERS.status'), None, fopen, os_path, disable_color=disable_color) self.output = [] self.indentation_stack = [] @@ -232,6 +238,17 @@ class OwnersFinderTests(_BaseTestCase): self.assertEqual(finder.output, [darin + ' is commented as:', ['foo (at content)']]) + def test_print_global_comments(self): + finder = self.ownersFinder(['content/common/common.cc']) + finder.print_comments(jochen) + self.assertEqual(finder.output, + [jochen + ' is commented as:', ['bar (global status)']]) + + finder = self.ownersFinder(['content/foo/foo.cc']) + finder.print_comments(jochen) + self.assertEqual(finder.output, + [jochen + ' is commented as:', ['bar (global status)', + 'foo (at content/foo)']]) if __name__ == '__main__': unittest.main() diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index 715d946a3..adbe7192f 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -74,11 +74,11 @@ class _BaseTestCase(unittest.TestCase): self.root = '/' self.fopen = self.repo.open_for_reading - def db(self, root=None, fopen=None, os_path=None): + def db(self, root=None, fopen=None, os_path=None, status_file=None): root = root or self.root fopen = fopen or self.fopen os_path = os_path or self.repo - return owners.Database(root, fopen, os_path) + return owners.Database(root, status_file, fopen, os_path) class OwnersDatabaseTest(_BaseTestCase): @@ -334,6 +334,12 @@ class OwnersDatabaseTest(_BaseTestCase): def test_syntax_error__invalid_relative_file(self): self.assert_syntax_error('file:foo/bar/baz\n') + def test_non_existant_status_file(self): + db = self.db(status_file='does_not_exist') + self.files['/foo/OWNERS'] = brett + self.files['/foo/DEPS'] = '' + self.assertRaises(IOError, db.reviewers_for, ['foo/DEPS'], None) + class ReviewersForTest(_BaseTestCase): def assert_reviewers_for(self, files, potential_suggested_reviewers, diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 9bc10476f..0b3ceaccd 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -120,6 +120,8 @@ index fe3de7b..54ae6e1 100755 self._root = obj.fake_root_dir def RepositoryRoot(self): return self._root + def GetOwnersStatusFile(self): + return None self.mox.StubOutWithMock(presubmit, 'random') self.mox.StubOutWithMock(presubmit, 'warn') @@ -511,6 +513,7 @@ class PresubmitUnittest(PresubmitTestsBase): 0, 0, None) + change.GetOwnersStatusFile = lambda: None executer = presubmit.PresubmitExecuter(change, False, None, False) self.failIf(executer.ExecPresubmitScript('', fake_presubmit)) # No error if no on-upload entry point @@ -1065,6 +1068,7 @@ class InputApiUnittest(PresubmitTestsBase): 0, 0, None) + change.GetOwnersStatusFile = lambda: None input_api = presubmit.InputApi( change, presubmit.os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'), @@ -1193,6 +1197,7 @@ class InputApiUnittest(PresubmitTestsBase): change = presubmit.GitChange( 'mychange', '', self.fake_root_dir, files, 0, 0, None) + change.GetOwnersStatusFile = lambda: None input_api = presubmit.InputApi( change, presubmit.os.path.join(self.fake_root_dir, 'PRESUBMIT.py'), @@ -1213,6 +1218,7 @@ class InputApiUnittest(PresubmitTestsBase): change = presubmit.GitChange( 'mychange', '', self.fake_root_dir, files, 0, 0, None) + change.GetOwnersStatusFile = lambda: None input_api = presubmit.InputApi( change, './PRESUBMIT.py', False, None, False) # Sample usage of overiding the default white and black lists. @@ -1535,9 +1541,9 @@ class ChangeUnittest(PresubmitTestsBase): members = [ 'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTestableFiles', 'AffectedTextFiles', - 'AllFiles', 'DescriptionText', 'FullDescriptionText', 'LocalPaths', - 'Name', 'RepositoryRoot', 'RightHandSideLines', - 'SetDescriptionText', 'TAG_LINE_RE', + 'AllFiles', 'DescriptionText', 'FullDescriptionText', + 'GetOwnersStatusFile', 'LocalPaths', 'Name', 'RepositoryRoot', + 'RightHandSideLines', 'SetDescriptionText', 'TAG_LINE_RE', 'author_email', 'issue', 'patchset', 'scm', 'tags', ] # If this test fails, you should add the relevant test.