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

YashJipkate · 2019-04-06T09:40:23Z

Explanation

Fixes #6449. A check is added to ensure the most important rules/patterns are at the bottom of the CODEOWNERS file.

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

codecov-io · 2019-04-06T10:37:49Z

Codecov Report

Merging #6560 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #6560   +/-   ##
========================================
  Coverage    95.32%   95.32%           
========================================
  Files          371      371           
  Lines        50558    50558           
========================================
  Hits         48193    48193           
  Misses        2365     2365

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 419a0f2...6679d6c. Read the comment docs.

YashJipkate · 2019-04-13T17:47:37Z

Hi, @apb7 Could you PTAL!
Thanks!

YashJipkate · 2019-04-19T03:37:00Z

Hi @apb7 Ping!

apb7

Hi @YashJipkate, sorry for the delay. Please manually check these changes by deliberately making such an error, locally. Thanks!

scripts/pre_commit_linter.py

apb7 · 2019-04-19T05:02:03Z

scripts/pre_commit_linter.py

@@ -2178,6 +2190,23 @@ def _check_codeowner_file(self):
                            print ('WARNING! %s/%s is not covered under '
                                   'CODEOWNERS' % (root, file_name))

+            # Checks that the most important rules/patterns are at the bottom
+            # of the CODEOWNERS file.
+            important_path_match_bool_list = ([


A couple of points here:

Please add comments explaining why we are populating this list.

Where are we checking whether the paths in CODEOWNER_IMPORTANT_PATHS are towards the end of the CODEOWNERS file?

Added the comments. PTAL!

I think you haven't answered this:

Where are we checking whether the paths in CODEOWNER_IMPORTANT_PATHS are towards the end of the CODEOWNERS file?

You are only checking the lines in CODEOWNER_IMPORTANT_PATHS as far as I understand.

Yes, we are checking for the ones in CODEOWNER_IMPORTANT_PATHS which contains the important rules.
Does this seem wrong?

Thanks for the explanation. Looks good!

scripts/pre_commit_linter.py

apb7 · 2019-04-19T19:34:46Z

scripts/pre_commit_linter.py

@@ -2178,6 +2190,23 @@ def _check_codeowner_file(self):
                            print ('WARNING! %s/%s is not covered under '
                                   'CODEOWNERS' % (root, file_name))

+            # Checks that the most important rules/patterns are at the bottom
+            # of the CODEOWNERS file.
+            important_path_match_bool_list = ([


I think you haven't answered this:

Where are we checking whether the paths in CODEOWNER_IMPORTANT_PATHS are towards the end of the CODEOWNERS file?

You are only checking the lines in CODEOWNER_IMPORTANT_PATHS as far as I understand.

apb7 · 2019-04-19T19:37:16Z

@YashJipkate, did you do this as well?

Please manually check these changes by deliberately making such an error, locally. Thanks!

Please answer all queries before requesting another review :)

YashJipkate · 2019-04-20T06:28:37Z

Sorry for that @apb7.
Yes I have tried this manually on my local by moving the important rules upward such that they are not at the end of the file, and the linter reported the same too.
Thanks!

apb7

LGTM, thanks, @YashJipkate!

apb7 · 2019-04-21T05:23:21Z

scripts/pre_commit_linter.py

@@ -2178,6 +2190,23 @@ def _check_codeowner_file(self):
                            print ('WARNING! %s/%s is not covered under '
                                   'CODEOWNERS' % (root, file_name))

+            # Checks that the most important rules/patterns are at the bottom
+            # of the CODEOWNERS file.
+            important_path_match_bool_list = ([


Thanks for the explanation. Looks good!

YashJipkate · 2019-04-23T19:10:01Z

Hi @apb7, Is this good to merge?
Thanks!

DubeySandeep

@YashJipkate, I went through this PR and left a few comments can you please check! :)

scripts/pre_commit_linter.py

DubeySandeep · 2019-04-27T11:45:25Z

scripts/pre_commit_linter.py

@@ -2178,6 +2192,30 @@ def _check_codeowner_file(self):
                            print ('WARNING! %s/%s is not covered under '
                                   'CODEOWNERS' % (root, file_name))

+            # Checks that the most important rules/patterns are at the bottom


I think to check code owner function is already 150~ lines big can we create a sperate function for checking important rule or pattern and call this over here? will it be possible?

This will also move this many comments into the new function docstring!

Yeah sounds good. Will do that!
Also, @apb7, do you agree on this too? (since you are always in favor of clubbing similar functionalities to a pre-existing function if any)

I agree with @DubeySandeep here. We can modularize this code. (As an FYI -- I am in favor of clubbing similar checks so that we do not end up repeating stuff. If there's no repetition, then it's totally fine :))

@YashJipkate, is this comment resolved?

Yes @DubeySandeep This is resolved now. Sorry for the delay.
PTAL! Thanks

DubeySandeep · 2019-04-27T11:48:35Z

scripts/pre_commit_linter.py

+            # matching the last lines of the CODEOWNERS file with the list of
+            # the important CODEOWNERS paths.
+            important_path_match_bool_list = ([
+                imp_path in path_lines[-1 * len(


Is someone adds a new pattern(important) in the top of the list, will this check ask users to add this list over here or will this check fail?

No, the check will fail. The 'n'-list and the last 'n' lines need to be in sync.

I tried adding a new file in the CODEOWNERS critical section. (without doing any changes in pre_commit_linter.py)

What should be the expected behavior?
I got this:

This is happening because of the new important rules that are not in sync with the CODEOWNER_IMPORTANT_PATHS list. The (outdated) list checks for the last n-lines of the CODEOWNERS file in which the erring pattern is not found and hence the error.

I'm happy that it's failing but the error message is something unrelated. Can we do something for that?

I have modified the error message. PTAL!
Thanks.

DubeySandeep · 2019-04-27T11:52:22Z

scripts/pre_commit_linter.py

+            # This condition checks that all the values in the boolean list are
+            # True. This ensures that the last 'n' lines of the CODEOWNERS file
+            # matches with the 'n'-element list of important CODEOWNER paths.
+            if not all(important_path_match_bool_list):


This will also look for all the value in the list i.e, what we have done in line 2209, if yes then can we just have the for loop? (is there any benefit of doing this check?)

I added this check so that the loop runs only when it is needed. At present the list is small so it may not seem that big a problem but in future when it enlarges it might be redundant to unnecessary run the loop.
What do you think?

In any case, we are going to run through all the values, isn't it?

Like if there is any false in the list we will cover the entire list through the for loop and if all the values are true we are going to loop through all the values via all(...), right?

Yeah, you are right about this. Will remove this. Thanks for pointing out!

DubeySandeep

(I'm marking this to "Request changes")

seanlip

LGTM (but only as code owner of CODEOWNERS :) )

oppiabot · 2019-06-05T22:26:49Z

Hi @YashJipkate, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

apb7 · 2019-06-08T16:17:56Z

Hi @YashJipkate, have you addressed the last round of review comments? If yes, can you please reply to them inline so that the reviewers have an idea. This PR has been standing for quite some time now. Can you please look into this? Thanks!

YashJipkate · 2019-06-09T11:12:07Z

Hi @apb7, Sorry for this PR getting stale. I have addressed all the comments except this #6560 (comment) since that requires complete restructure of the method I have used.

seanlip · 2019-06-09T11:21:02Z

Hi @YashJipkate, I think you can still address that comment. The change is not that big. Are you planning to do so?

YashJipkate · 2019-06-12T12:39:44Z

Hi @seanlip Sorry for the delay, I have changed the working rule to make way for sets. PTAL!
Thanks!

seanlip

This looks good to me, just some very minor comments left. Thanks @YashJipkate!

seanlip · 2019-06-12T19:20:19Z

scripts/pre_commit_linter.py

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


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?

I have generalized "patterns".

seanlip · 2019-06-12T19:21:34Z

scripts/pre_commit_linter.py

+                paths/rules for CODEOWNERS file.
+
+        Returns:
+            bool. Status of failure.


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

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

seanlip · 2019-06-12T19:22:54Z

scripts/pre_commit_linter.py

+                  ' 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 '


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

seanlip · 2019-06-12T19:23:09Z

scripts/pre_commit_linter.py

+            print('scripts/pre_commit_linter.py --> Duplicate pattern found in '
+                  'CODEOWNER_IMPORTANT_PATHS list.')
+            failed = True
+        # Check missing rules by set difference operation.


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

seanlip · 2019-06-12T19:23:39Z

scripts/pre_commit_linter.py

+            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 '


since --> section since

seanlip

LGTM. Thanks @YashJipkate !

seanlip · 2019-06-13T05:22:12Z

@DubeySandeep PTAL. Thanks!

seanlip · 2019-06-13T07:56:22Z

@YashJipkate qq -- don't the lint checks run before you push? Just wondering how that slipped through and how we can prevent it in the future.

/cc @oppia/dev-workflow-team

YashJipkate · 2019-06-13T07:59:29Z

Actually, in this copy of my repo, there was some problem regarding the python version (since this my new system) and hence the linter does not work (probably). The other copy works fine as it had the correct python version installed.

seanlip · 2019-06-13T08:00:43Z

Ah, ok. If you feel like we need to fix the pre-commit linter to guard against this issue, it might be worth doing that. Might save a lot of time in future.

YashJipkate · 2019-06-13T08:04:03Z

Thanks @seanlip. But fortunately, I no longer use that copy and use the one which has the linter running. This is an old PR and hence was in the old copy.

seanlip · 2019-06-13T08:15:39Z

OK cool! Sounds good then :D

YashJipkate · 2019-06-13T12:15:37Z

@DubeySandeep The checks pass now. Could this be merged?
Thanks!

YashJipkate added 2 commits April 6, 2019 15:01

Added imp rules check

7423c00

Removed unnecessary comment

bea64b0

YashJipkate requested a review from apb7 as a code owner April 6, 2019 09:40

YashJipkate added the CHANGELOG: Development workflow label Apr 6, 2019

YashJipkate assigned apb7 Apr 8, 2019

apb7 suggested changes Apr 19, 2019

View reviewed changes

Added comments, alphabetize list and improved error message

c2c94e8

YashJipkate requested a review from apb7 April 19, 2019 09:31

apb7 suggested changes Apr 19, 2019

View reviewed changes

apb7 assigned YashJipkate and unassigned apb7 Apr 19, 2019

YashJipkate assigned apb7 and unassigned YashJipkate Apr 20, 2019

YashJipkate requested a review from apb7 April 20, 2019 06:29

apb7 approved these changes Apr 21, 2019

View reviewed changes

DubeySandeep reviewed Apr 27, 2019

View reviewed changes

DubeySandeep assigned YashJipkate and unassigned apb7 Apr 27, 2019

YashJipkate added 2 commits May 1, 2019 12:58

Merge branch 'develop' into codeowner-imp-rules

35abb55

Review addressed

9d2e3c3

YashJipkate requested a review from seanlip as a code owner May 1, 2019 07:46

DubeySandeep requested changes May 2, 2019

View reviewed changes

seanlip approved these changes May 2, 2019

View reviewed changes

Improved error messages

2df107a

oppiabot bot added the stale label Jun 5, 2019

Address review

596f823

oppiabot bot removed the stale label Jun 6, 2019

YashJipkate added 3 commits June 12, 2019 16:36

Merge branch 'develop' into codeowner-imp-rules

0b5b9b9

Changed working rule to use set operations

1c3f7c4

Fix lint

7e2ad7c

seanlip requested changes Jun 12, 2019

View reviewed changes

seanlip assigned YashJipkate Jun 12, 2019

Address review

283997d

YashJipkate requested a review from seanlip June 13, 2019 03:49

YashJipkate assigned seanlip and unassigned YashJipkate Jun 13, 2019

seanlip approved these changes Jun 13, 2019

View reviewed changes

seanlip removed their assignment Jun 13, 2019

Fix lint

6679d6c

DubeySandeep approved these changes Jun 13, 2019

View reviewed changes

DubeySandeep merged commit 61a6350 into oppia:develop Jun 13, 2019

YashJipkate deleted the codeowner-imp-rules branch June 13, 2019 16:49

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

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

Conversation

YashJipkate commented Apr 6, 2019

Explanation

Checklist

codecov-io commented Apr 6, 2019 • edited by codecov bot Loading

Codecov Report

YashJipkate commented Apr 13, 2019

YashJipkate commented Apr 19, 2019

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apb7 commented Apr 19, 2019

YashJipkate commented Apr 20, 2019

apb7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YashJipkate commented Apr 23, 2019

DubeySandeep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DubeySandeep left a comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

oppiabot bot commented Jun 5, 2019

apb7 commented Jun 8, 2019

YashJipkate commented Jun 9, 2019

seanlip commented Jun 9, 2019

YashJipkate commented Jun 12, 2019

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Jun 13, 2019

seanlip commented Jun 13, 2019

YashJipkate commented Jun 13, 2019

seanlip commented Jun 13, 2019

YashJipkate commented Jun 13, 2019

seanlip commented Jun 13, 2019

YashJipkate commented Jun 13, 2019

codecov-io commented Apr 6, 2019 •

edited by codecov bot

Loading