Merge branch 'master' into iamsobanjaved/fix-pr-title

edx_repo_tools/repo_checks/labels.yaml

@@ -73,6 +73,10 @@
   color: "038E8E" # tealish
   description: "Relates to an Axim Funded Contribution project"
 
+- name: "performance"
+  color: "4a03fc" # blue-purple
+  description: "Relates to improving latency/throughput or reducing resource usage"
+
 
 ### LABELS INDICATING THE SCOPE OR FUNCTION OF THE ISSUE.
 ### AT MOST ONE OF THESE SHOULD BE USED AT A TIME.

edx_repo_tools/repo_checks/repo_checks.py

@@ -14,7 +14,8 @@
 import importlib.resources
 import re
 import textwrap
-from functools import lru_cache
+import typing as t
+from functools import cache
 from itertools import chain
 from pprint import pformat
 
@@ -37,11 +38,6 @@
 
 LABELS_YAML_FILENAME = "./labels.yaml"
 
-# Note: This is functionally equivalent to `from functools import cache`,
-# which becomes available in Python 3.9.
-# https://docs.python.org/3/library/functools.html#functools.cache
-cache = lru_cache(maxsize=None)
-
 
 def all_paged_items(func, *args, **kwargs):
     """
@@ -109,11 +105,25 @@ class Check:
     (is_relevant, check, fix, and dry_run).
     """
 
+    _registered = {}
+
     def __init__(self, api: GhApi, org: str, repo: str):
         self.api = api
         self.org_name = org
         self.repo_name = repo
 
+    @staticmethod
+    def register(subclass: type[t.Self]) -> type[t.Self]:
+        """
+        Decorate a Check subclass so that it will be available in main()
+        """
+        Check._registered[subclass.__name__] = subclass
+        return subclass
+
+    @staticmethod
+    def get_registered_checks() -> dict[str, type[t.Self]]:
+        return Check._registered.copy()
+
     def is_relevant(self) -> bool:
         """
         Checks to see if the given check is relevant to run on the
@@ -157,7 +167,8 @@ def dry_run(self):
         raise NotImplementedError
 
 
-class EnsureRepoSettings(Check):
+@Check.register
+class Settings(Check):
     """
     There are certain settings that we agree we want to be set a specific way on all repos.  This check
     will ensure that those settings are set correctly on all non-security repos.
@@ -245,7 +256,8 @@ def fix(self, dry_run=False):
         return steps
 
 
-class EnsureNoAdminOrMaintainTeams(Check):
+@Check.register
+class NoAdminOrMaintainTeams(Check):
     """
     Teams should not be granted `admin` or `maintain` access to a repository unless the access
     is exceptional and it is noted here.  All other `admin` and `maintain` access is downgraded to
@@ -314,7 +326,8 @@ def fix(self, dry_run=False):
         return steps
 
 
-class EnsureWorkflowTemplates(Check):
+@Check.register
+class Workflows(Check):
     """
     There are certain github action workflows that we to exist on all
     repos exactly as they are defined in the `.github` repo in the org.
@@ -599,7 +612,8 @@ def fix(self, dry_run=False):
         return steps
 
 
-class EnsureLabels(Check):
+@Check.register
+class Labels(Check):
     """
     All repos in the org should have certain labels.
     """
@@ -714,12 +728,12 @@ def _simplify_label(self, label: str):
         return simplified_label
 
 
-class RequireTeamPermission(Check):
+class TeamAccess(Check):
     """
     Require that a team has a certain level of access to a repository.
 
     To use this class as a check, create a subclass that specifies a particular
-    team and permission level, such as RequireTriageTeamAccess below.
+    team and permission level, such as TriageTeam below.
     """
 
     def __init__(self, api: GhApi, org: str, repo: str, team: str, permission: str):
@@ -787,7 +801,8 @@ def fix(self, dry_run=False):
             raise
 
 
-class RequireTriageTeamAccess(RequireTeamPermission):
+@Check.register
+class TriageTeam(TeamAccess):
     """
     Ensure that the openedx-triage team grants Triage access to every public repo in the org.
     """
@@ -802,7 +817,8 @@ def is_relevant(self):
         return is_public(self.api, self.org_name, self.repo_name)
 
 
-class RequiredCLACheck(Check):
+@Check.register
+class EnforceCLA(Check):
     """
     This class validates the following:
 
@@ -822,7 +838,7 @@ def __init__(self, api, org, repo):
         self.cla_team = "cla-checker"
         self.cla_team_permission = "push"
 
-        self.team_check = RequireTeamPermission(
+        self.team_check = TeamAccess(
             api,
             org,
             repo,
@@ -1062,7 +1078,8 @@ def _get_update_params_from_get_branch_protection(self):
         return params
 
 
-class EnsureNoDirectRepoAccessToUsers(Check):
+@Check.register
+class NoDirectUsers(Check):
     """
     Users should not have direct repo access
     """
@@ -1119,17 +1136,62 @@ def fix(self, dry_run=False):
         return steps
 
 
-CHECKS = [
-    RequiredCLACheck,
-    RequireTriageTeamAccess,
-    EnsureLabels,
-    EnsureWorkflowTemplates,
-    EnsureNoAdminOrMaintainTeams,
-    EnsureRepoSettings,
-    EnsureNoDirectRepoAccessToUsers,
-]
-CHECKS_BY_NAME = {check_cls.__name__: check_cls for check_cls in CHECKS}
-CHECKS_BY_NAME_LOWER = {check_cls.__name__.lower(): check_cls for check_cls in CHECKS}
+@Check.register
+class EnsureNoOutsideCollaborators(Check):
+    """
+    Repository shouldn't have outside collaborators
+    """
+
+    def __init__(self, api: GhApi, org: str, repo: str):
+        super().__init__(api, org, repo)
+        self.users_list = []
+
+    def is_relevant(self) -> bool:
+        """
+        All non security fork repos, public or private.
+        """
+        return not is_security_private_fork(self.api, self.org_name, self.repo_name)
+
+    def check(self) -> tuple[bool, str]:
+        """
+        Verify whether or not the check is failing.
+
+        This should not change anything and should not have a side-effect
+        other than populating `self` with any data that is needed later for
+        `fix` or `dry_run`.
+
+        The string in the return tuple should be a human readable reason
+        that the check failed.
+        """
+        self.users_list = list(all_paged_items(
+            self.api.repos.list_collaborators, owner=self.org_name, repo=self.repo_name, affiliation='outside'
+        ))
+        users = [f"{user.login}: {user.role_name}" for user in self.users_list]
+        if users:
+            return (
+                False,
+                f"The repo has some outside collaborators:\n\t\t"
+                + "\n\t\t".join(users),
+            )
+        return (True, "The repo doesn't have any outside collaborators.")
+
+    def dry_run(self):
+        return self.fix(dry_run=True)
+
+    def fix(self, dry_run=False):
+        steps = []
+        for user in self.users_list:
+            if not dry_run:
+                self.api.repos.remove_collaborator(
+                    owner=self.org_name,
+                    repo=self.repo_name,
+                    username=user.login,
+                )
+            steps.append(
+                f"Removed outside collaborator {user.login}"
+            )
+
+        return steps
 
 
 @click.command()
@@ -1159,7 +1221,7 @@ def fix(self, dry_run=False):
     "check_names",
     default=None,
     multiple=True,
-    type=click.Choice(CHECKS_BY_NAME.keys(), case_sensitive=False),
+    type=click.Choice(Check.get_registered_checks().keys(), case_sensitive=False),
     help=f"Limit to specific check(s), case-insensitive.",
 )
 @click.option(
@@ -1198,9 +1260,9 @@ def main(org, dry_run, _github_token, check_names, repos, start_at):
         click.secho("No Actual Changes Being Made", fg="yellow")
 
     if check_names:
-        active_checks = [CHECKS_BY_NAME[check_name] for check_name in check_names]
+        active_checks = [Check.get_registered_checks()[check_name] for check_name in check_names]
     else:
-        active_checks = CHECKS
+        active_checks = list(Check.get_registered_checks().values())
     click.secho(f"The following checks will be run:", fg="magenta", bold=True)
     active_checks_string = "\n".join(
         "\t" + check_cls.__name__ for check_cls in active_checks

tests/test_repo_checks.py

@@ -7,7 +7,7 @@
 
 import pytest
 
-from edx_repo_tools.repo_checks.repo_checks import EnsureLabels
+from edx_repo_tools.repo_checks import repo_checks
 
 
 @pytest.fixture
@@ -35,20 +35,20 @@ def maintenance_label():
 ]
 
 
-@patch.object(EnsureLabels, "labels", labels_yaml)
-class TestEnsureLabels:
+@patch.object(repo_checks.Labels, "labels", labels_yaml)
+class TestLabelsCheck:
     def test_check_for_no_change(self, maintenance_label):
         api = MagicMock()
         api.issues.list_labels_for_repo.side_effect = [[maintenance_label], None]
-        check_cls = EnsureLabels(api, "test_org", "test_repo")
+        check_cls = repo_checks.Labels(api, "test_org", "test_repo")
 
         # Make sure that the check returns True, indicating that no changes need to be made.
         assert check_cls.check()[0]
 
     def test_addition(self, maintenance_label):
         api = MagicMock()
         api.issues.list_labels_for_repo.return_value = []
-        check_cls = EnsureLabels(api, "test_org", "test_repo")
+        check_cls = repo_checks.Labels(api, "test_org", "test_repo")
 
         # The check should be false because the maintenance label should be missing.
         assert check_cls.check()[0] == False
@@ -72,7 +72,7 @@ def test_update_label(self, maintenance_label):
         api = MagicMock()
         api.issues.list_labels_for_repo.side_effect = [[maintenance_label], None]
 
-        check_cls = EnsureLabels(api, "test_org", "test_repo")
+        check_cls = repo_checks.Labels(api, "test_org", "test_repo")
 
         assert check_cls.check()[0] == False
         check_cls.fix()