From c1a6b3a0c7de47239d73302a1047111cd5cc9d84 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 5 Aug 2024 11:15:40 -0400 Subject: [PATCH 1/5] feat: add 'performance' label to all repos (#548) As we focus more on performance issues like latency and memory usage across our various services, it's going to be useful to have a consistent, project-wide label to track these. --- edx_repo_tools/repo_checks/labels.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/edx_repo_tools/repo_checks/labels.yaml b/edx_repo_tools/repo_checks/labels.yaml index 9a1a3fd3..597e0ef3 100644 --- a/edx_repo_tools/repo_checks/labels.yaml +++ b/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. From 7e4a5c8b6ec74f16f98e0391f4c024f8c8e17002 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 5 Aug 2024 13:09:43 -0400 Subject: [PATCH 2/5] chore: lru_cache -> cache --- edx_repo_tools/repo_checks/repo_checks.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/edx_repo_tools/repo_checks/repo_checks.py b/edx_repo_tools/repo_checks/repo_checks.py index 94968acb..d817e957 100644 --- a/edx_repo_tools/repo_checks/repo_checks.py +++ b/edx_repo_tools/repo_checks/repo_checks.py @@ -14,7 +14,7 @@ import importlib.resources import re import textwrap -from functools import lru_cache +from functools import cache from itertools import chain from pprint import pformat @@ -37,11 +37,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): """ From 165e7d669338ec8b8e036a6587a9923e676149c3 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 5 Aug 2024 13:26:12 -0400 Subject: [PATCH 3/5] refactor: register repo checks with a decorator Removes a bit of redundancy / potential typos inherent in the CHECK list. This will also make it easier to break repo_checks.py into multiple modules, if we ever decide to do that. --- edx_repo_tools/repo_checks/repo_checks.py | 41 ++++++++++++++--------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/edx_repo_tools/repo_checks/repo_checks.py b/edx_repo_tools/repo_checks/repo_checks.py index d817e957..4f7afcb9 100644 --- a/edx_repo_tools/repo_checks/repo_checks.py +++ b/edx_repo_tools/repo_checks/repo_checks.py @@ -14,6 +14,7 @@ import importlib.resources import re import textwrap +import typing as t from functools import cache from itertools import chain from pprint import pformat @@ -104,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 @@ -152,6 +167,7 @@ def dry_run(self): raise NotImplementedError +@Check.register class EnsureRepoSettings(Check): """ There are certain settings that we agree we want to be set a specific way on all repos. This check @@ -240,6 +256,7 @@ def fix(self, dry_run=False): return steps +@Check.register class EnsureNoAdminOrMaintainTeams(Check): """ Teams should not be granted `admin` or `maintain` access to a repository unless the access @@ -309,6 +326,7 @@ def fix(self, dry_run=False): return steps +@Check.register class EnsureWorkflowTemplates(Check): """ There are certain github action workflows that we to exist on all @@ -594,6 +612,7 @@ def fix(self, dry_run=False): return steps +@Check.register class EnsureLabels(Check): """ All repos in the org should have certain labels. @@ -782,6 +801,7 @@ def fix(self, dry_run=False): raise +@Check.register class RequireTriageTeamAccess(RequireTeamPermission): """ Ensure that the openedx-triage team grants Triage access to every public repo in the org. @@ -797,6 +817,7 @@ def is_relevant(self): return is_public(self.api, self.org_name, self.repo_name) +@Check.register class RequiredCLACheck(Check): """ This class validates the following: @@ -1057,6 +1078,7 @@ def _get_update_params_from_get_branch_protection(self): return params +@Check.register class EnsureNoDirectRepoAccessToUsers(Check): """ Users should not have direct repo access @@ -1114,19 +1136,6 @@ 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} - - @click.command() @click.option( "--github-token", @@ -1154,7 +1163,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( @@ -1193,9 +1202,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 From 17c885a69136bff2a526cd06ce9c013d7896f894 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Mon, 5 Aug 2024 14:50:09 -0400 Subject: [PATCH 4/5] feat!: rename repo checks to be more concise & consitent No need to prefix them with "Ensure", when they are all ensuring something. This will make it easier to read the list of required of checks in the --help text. It will also make it easier to type out which checks to run. --- edx_repo_tools/repo_checks/repo_checks.py | 20 ++++++++++---------- tests/test_repo_checks.py | 12 ++++++------ 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/edx_repo_tools/repo_checks/repo_checks.py b/edx_repo_tools/repo_checks/repo_checks.py index 4f7afcb9..07966073 100644 --- a/edx_repo_tools/repo_checks/repo_checks.py +++ b/edx_repo_tools/repo_checks/repo_checks.py @@ -168,7 +168,7 @@ def dry_run(self): @Check.register -class EnsureRepoSettings(Check): +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. @@ -257,7 +257,7 @@ def fix(self, dry_run=False): @Check.register -class EnsureNoAdminOrMaintainTeams(Check): +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 @@ -327,7 +327,7 @@ def fix(self, dry_run=False): @Check.register -class EnsureWorkflowTemplates(Check): +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. @@ -613,7 +613,7 @@ def fix(self, dry_run=False): @Check.register -class EnsureLabels(Check): +class Labels(Check): """ All repos in the org should have certain labels. """ @@ -728,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): @@ -802,7 +802,7 @@ def fix(self, dry_run=False): @Check.register -class RequireTriageTeamAccess(RequireTeamPermission): +class TriageTeam(TeamAccess): """ Ensure that the openedx-triage team grants Triage access to every public repo in the org. """ @@ -818,7 +818,7 @@ def is_relevant(self): @Check.register -class RequiredCLACheck(Check): +class EnforceCLA(Check): """ This class validates the following: @@ -838,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, @@ -1079,7 +1079,7 @@ def _get_update_params_from_get_branch_protection(self): @Check.register -class EnsureNoDirectRepoAccessToUsers(Check): +class NoDirectUsers(Check): """ Users should not have direct repo access """ diff --git a/tests/test_repo_checks.py b/tests/test_repo_checks.py index 671d1d27..be1cfbb3 100644 --- a/tests/test_repo_checks.py +++ b/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,12 +35,12 @@ 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] @@ -48,7 +48,7 @@ def test_check_for_no_change(self, maintenance_label): 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() From adbfd24dd73ac215dde7c9fe9160b2479d513581 Mon Sep 17 00:00:00 2001 From: Muhammad Farhan Khan Date: Fri, 26 Jul 2024 17:52:06 +0500 Subject: [PATCH 5/5] feat: Adds check to ensure no repo have any outside collaborator --- edx_repo_tools/repo_checks/repo_checks.py | 58 +++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/edx_repo_tools/repo_checks/repo_checks.py b/edx_repo_tools/repo_checks/repo_checks.py index 07966073..2ae04adc 100644 --- a/edx_repo_tools/repo_checks/repo_checks.py +++ b/edx_repo_tools/repo_checks/repo_checks.py @@ -1136,6 +1136,64 @@ def fix(self, dry_run=False): return steps +@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() @click.option( "--github-token",