Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #6449: Added check for CODEOWNERS to ensure that the most important rules are at the end. #6560

Merged
merged 16 commits into from
Jun 13, 2019
Merged

Fix #6449: Added check for CODEOWNERS to ensure that the most important rules are at the end. #6560

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7423c00
Added imp rules check
YashJipkate Apr 6, 2019
bea64b0
Removed unnecessary comment
YashJipkate Apr 6, 2019
c2c94e8
Added comments, alphabetize list and improved error message
YashJipkate Apr 19, 2019
35abb55
Merge branch 'develop' into codeowner-imp-rules
YashJipkate May 1, 2019
9d2e3c3
Review addressed
YashJipkate May 1, 2019
2df107a
Improved error messages
YashJipkate May 3, 2019
78d4b16
Merge and break codeowners check function
YashJipkate May 19, 2019
644c6a2
Fix lint
YashJipkate May 20, 2019
026e091
Merge develop
YashJipkate May 29, 2019
65a4c8f
Address review
YashJipkate May 29, 2019
596f823
Address review
YashJipkate Jun 6, 2019
0b5b9b9
Merge branch 'develop' into codeowner-imp-rules
YashJipkate Jun 12, 2019
1c3f7c4
Changed working rule to use set operations
YashJipkate Jun 12, 2019
7e2ad7c
Fix lint
YashJipkate Jun 12, 2019
283997d
Address review
YashJipkate Jun 13, 2019
6679d6c
Fix lint
YashJipkate Jun 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,8 @@
#
# https://help.github.com/en/articles/about-code-owners
#
# On modifying this list make sure to keep the CODEOWNER_IMPORTANT_PATHS list
# in scripts/pre_commit_linter.py in sync with the modifications.
/core/controllers/acl_decorators*.py @seanlip
/core/controllers/base*.py @seanlip
/core/domain/dependency_registry*.py @seanlip
Expand Down
92 changes: 86 additions & 6 deletions scripts/pre_commit_linter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,25 @@
'webpack.dev.config.ts',
'webpack.prod.config.ts')

CODEOWNER_FILEPATH = '.github/CODEOWNERS'

# This list needs to be in sync with the important patterns in the CODEOWNERS
# file.
CODEOWNER_IMPORTANT_PATHS = [
DubeySandeep marked this conversation as resolved.
Show resolved Hide resolved
'/core/controllers/acl_decorators*.py',
'/core/controllers/base*.py',
'/core/domain/dependency_registry*.py',
'/core/domain/html*.py',
'/core/domain/rights_manager*.py',
'/core/domain/role_services*.py',
'/core/domain/user*.py',
'/core/storage/',
'/export/',
'/manifest.json',
'/package*.json',
'/scripts/install_third_party.sh',
'/.github/']

if not os.getcwd().endswith('oppia'):
print ''
print 'ERROR Please run this script from the oppia root directory.'
Expand Down Expand Up @@ -2188,42 +2207,99 @@ def _check_bad_patterns(self):

return summary_messages

def check_for_important_rules_at_bottom_of_codeowners(
self, important_patterns):
"""Checks that the most important rules/patterns are at the bottom
of the CODEOWNERS file.

Arguments:
important_patterns: list(str). List of the important
paths/rules for CODEOWNERS file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you say "paths/rules", but in the docstring you say "rules/patterns", and to make things even more confusing, this arg is called "important_patterns".

Please could you figure out what is actually being referenced, and amend the docstrings/argname/descriptions accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have generalized "patterns".


Returns:
bool. Status of failure.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool. Whether the CODEOWNERS "important rule" check passes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"- check fails" would be more appropriate I guess since the bool actually has the status of failure.

"""

failed = False
# Check that there are no duplicate elements in the lists.
important_patterns_set = set(important_patterns)
codeowner_important_paths_set = set(CODEOWNER_IMPORTANT_PATHS)
if len(important_patterns_set) != len(important_patterns):
print('%s --> Duplicate patterns found in critical rules'
' section.' % CODEOWNER_FILEPATH)
failed = True
if len(codeowner_important_paths_set) != len(CODEOWNER_IMPORTANT_PATHS):
print('scripts/pre_commit_linter.py --> Duplicate pattern found in '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: pattern --> patterns for consistency with previous message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

'CODEOWNER_IMPORTANT_PATHS list.')
failed = True
# Check missing rules by set difference operation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a blank line above this. Divide your code into meaningful "blocks".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

critical_rule_section_minus_list_set = (
important_patterns_set.difference(codeowner_important_paths_set))
list_minus_critical_rule_section_set = (
codeowner_important_paths_set.difference(important_patterns_set))
for rule in critical_rule_section_minus_list_set:
print('%s --> Rule %s is not present in the '
'CODEOWNER_IMPORTANT_PATHS list in '
'scripts/pre_commit_linter.py. Please add this rule in the '
'mentioned list or remove this rule from the \'Critical files'
'\' section.' % (CODEOWNER_FILEPATH, rule))
failed = True
for rule in list_minus_critical_rule_section_set:
print('%s --> Rule \'%s\' is not present in the \'Critical files\' '
'section. Please place it under the \'Critical files\' since '
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since --> section since

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

'it is an important rule. Alternatively please remove it '
'from the \'CODEOWNER_IMPORTANT_PATHS\' list in '
'scripts/pre_commit_linter.py if it is no longer an '
'important rule.' % (CODEOWNER_FILEPATH, rule))
failed = True
return failed

def _check_codeowner_file(self):
"""Checks the CODEOWNERS file for any uncovered dirs/files and also
checks that every pattern in the CODEOWNERS file matches at least one
file/dir. Note that this checks the CODEOWNERS file according to the
glob patterns supported by Python2.7 environment. For more information
please refer https://docs.python.org/2/library/glob.html.
This function also ensures that the most important rules are at the
bottom of the CODEOWNERS file.
"""
if self.verbose_mode_enabled:
print 'Starting CODEOWNERS file check'
print '----------------------------------------'

with _redirect_stdout(_TARGET_STDOUT):
codeowner_filepath = '.github/CODEOWNERS'
failed = False
summary_messages = []
# Checks whether every pattern in the CODEOWNERS file matches at
# least one dir/file.
critical_file_section_found = False
important_rules_in_critical_section = []
file_patterns = []
dir_patterns = []
for line_num, line in enumerate(FileCache.readlines(
codeowner_filepath)):
CODEOWNER_FILEPATH)):
stripped_line = line.strip()
if '# Critical files' in line:
critical_file_section_found = True
if stripped_line and stripped_line[0] != '#':
if '@' not in line:
print ('%s --> Pattern on line %s doesn\'t have '
'codeowner' % (codeowner_filepath, line_num + 1))
'codeowner' % (CODEOWNER_FILEPATH, line_num + 1))
failed = True
else:
# Extract the file pattern from the line.
line_in_concern = line.split('@')[0].strip()
# This is being populated for the important rules
# check.
if critical_file_section_found:
important_rules_in_critical_section.append(
line_in_concern)
# Checks if the path is the full path relative to the
# root oppia directory.
if not line_in_concern.startswith('/'):
print ('%s --> Pattern on line %s is invalid. Use '
'full path relative to the root directory'
% (codeowner_filepath, line_num + 1))
% (CODEOWNER_FILEPATH, line_num + 1))
failed = True

# The double asterisks pattern is supported by the
Expand All @@ -2232,7 +2308,7 @@ def _check_codeowner_file(self):
if '**' in line_in_concern:
print ('%s --> Pattern on line %s is invalid. '
'\'**\' wildcard not allowed' % (
codeowner_filepath, line_num + 1))
CODEOWNER_FILEPATH, line_num + 1))
failed = True
# Adjustments to the dir paths in CODEOWNERS syntax
# for glob-style patterns to match correctly.
Expand All @@ -2250,7 +2326,7 @@ def _check_codeowner_file(self):
if not glob.glob(line_in_concern):
print ('%s --> Pattern on line %s doesn\'t match '
'any file or directory' % (
codeowner_filepath, line_num + 1))
CODEOWNER_FILEPATH, line_num + 1))
failed = True
# The following list is being populated with the
# paths in the CODEOWNERS file with the removal of the
Expand All @@ -2275,6 +2351,10 @@ def _check_codeowner_file(self):
print '%s is not covered under CODEOWNERS' % file_path
failed = True

failed = failed or (
self.check_for_important_rules_at_bottom_of_codeowners(
important_rules_in_critical_section))

if failed:
summary_message = '%s CODEOWNERS file check failed' % (
_MESSAGE_TYPE_FAILED)
Expand Down