From e4c0c064028edf6c5815f011c8b2ae1865116fd8 Mon Sep 17 00:00:00 2001 From: jmeridth Date: Wed, 13 Mar 2024 04:43:06 -0500 Subject: [PATCH 1/7] fix: handle if ORGANIZATION not provided Fixes #27 Use each repo's org from REPOSITORY list if ORGANIZTION not provided This makes sense because the repositories in REPOSITORY could have different organizations - [x] utilize repo's owner/org when ORGANIZTION not provided - [x] show the prefixed org is set when parsing REPOSITORY env var - [ ] additional testing Signed-off-by: jmeridth --- cleanowners.py | 11 +++++++---- test_cleanowners.py | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cleanowners.py b/cleanowners.py index f7fc22f..e364107 100644 --- a/cleanowners.py +++ b/cleanowners.py @@ -85,10 +85,11 @@ def main(): # pragma: no cover usernames = get_usernames_from_codeowners(codeowners_file_contents) for username in usernames: + org = organization if organization else repo.owner.login # Check to see if the username is a member of the organization - if not github_connection.organization(organization).is_member(username): + if not github_connection.organization(org).is_member(username): print( - f"\t{username} is not a member of {organization}. Suggest removing them from {repo.full_name}" + f"\t{username} is not a member of {org}. Suggest removing them from {repo.full_name}" ) users_count += 1 if not dry_run: @@ -143,9 +144,11 @@ def get_repos_iterator(organization, repository_list, github_connection): repos = github_connection.organization(organization).repositories() else: # Get the repositories from the repository_list - for repo in repository_list: + for full_repo_path in repository_list: + org = full_repo_path.split("/")[0] + repo = full_repo_path.split("/")[1] repos.append( - github_connection.repository(repo.split("/")[0], repo.split("/")[1]) + github_connection.repository(org, repo) ) return repos diff --git a/test_cleanowners.py b/test_cleanowners.py index 792ce1a..2a67a86 100644 --- a/test_cleanowners.py +++ b/test_cleanowners.py @@ -111,7 +111,7 @@ def test_get_repos_iterator_with_organization(self, mock_github): def test_get_repos_iterator_with_repository_list(self, mock_github): """Test the get_repos_iterator function with a repository list""" organization = None - repository_list = ["org/repo1", "org/repo2"] + repository_list = ["org/repo1", "org2/repo2"] github_connection = mock_github.return_value mock_repository = MagicMock() @@ -123,7 +123,7 @@ def test_get_repos_iterator_with_repository_list(self, mock_github): # Assert that the repository method was called with the correct arguments for each repository in the list expected_calls = [ unittest.mock.call("org", "repo1"), - unittest.mock.call("org", "repo2"), + unittest.mock.call("org2", "repo2"), ] github_connection.repository.assert_has_calls(expected_calls) From bc0ca94e4fe57f98cfd79839ae15d72daafd78de Mon Sep 17 00:00:00 2001 From: jmeridth Date: Wed, 13 Mar 2024 17:18:41 -0500 Subject: [PATCH 2/7] fix: linting `black` linting Signed-off-by: jmeridth --- cleanowners.py | 4 +--- test_auth.py | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cleanowners.py b/cleanowners.py index e364107..c5f6cf6 100644 --- a/cleanowners.py +++ b/cleanowners.py @@ -147,9 +147,7 @@ def get_repos_iterator(organization, repository_list, github_connection): for full_repo_path in repository_list: org = full_repo_path.split("/")[0] repo = full_repo_path.split("/")[1] - repos.append( - github_connection.repository(org, repo) - ) + repos.append(github_connection.repository(org, repo)) return repos diff --git a/test_auth.py b/test_auth.py index 9c8786c..d46ffd5 100644 --- a/test_auth.py +++ b/test_auth.py @@ -1,4 +1,5 @@ """Test cases for the auth module.""" + import unittest from unittest.mock import patch From 0c0bec45c4062b2672e49695588018d5cfce8bbe Mon Sep 17 00:00:00 2001 From: jmeridth Date: Thu, 14 Mar 2024 16:23:04 -0500 Subject: [PATCH 3/7] fix: check if the org prefix is an org when the user provides the org in the org/repo comman delimited list of REPOSITORY env var, we still need to validate that the org is an org and not a user Signed-off-by: jmeridth --- cleanowners.py | 25 +++++++++++++++++++++++-- test_cleanowners.py | 17 +++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/cleanowners.py b/cleanowners.py index c5f6cf6..4bc8cce 100644 --- a/cleanowners.py +++ b/cleanowners.py @@ -6,6 +6,15 @@ import env import github3 +def get_org(github_connection, organization): + """Get the organization object""" + if organization: + try: + return github_connection.organization(organization) + except github3.exceptions.NotFoundError: + print(f"Organization {organization} not found") + return None + return None def main(): # pragma: no cover """Run the main program""" @@ -31,6 +40,11 @@ def main(): # pragma: no cover codeowners_count = 0 users_count = 0 + gh_org = get_org(github_connection, organization) + if not gh_org: + print(f"Organization {organization} not found") + return + # Get the repositories from the organization or list of repositories repos = get_repos_iterator(organization, repository_list, github_connection) @@ -86,8 +100,13 @@ def main(): # pragma: no cover for username in usernames: org = organization if organization else repo.owner.login + gh_org = get_org(github_connection, org) + if not gh_org: + print(f"Owner {org} of repo {repo} is not an organization.") + break + # Check to see if the username is a member of the organization - if not github_connection.organization(org).is_member(username): + if not gh_org.is_member(username): print( f"\t{username} is not a member of {org}. Suggest removing them from {repo.full_name}" ) @@ -97,7 +116,9 @@ def main(): # pragma: no cover file_changed = True bytes_username = f"@{username}".encode("ASCII") codeowners_file_contents_new = ( - codeowners_file_contents.decoded.replace(bytes_username, b"") + codeowners_file_contents.decoded.replace( + bytes_username, b"" + ) ) # Update the CODEOWNERS file if usernames were removed diff --git a/test_cleanowners.py b/test_cleanowners.py index 2a67a86..5abb3a5 100644 --- a/test_cleanowners.py +++ b/test_cleanowners.py @@ -6,6 +6,7 @@ from cleanowners import ( commit_changes, + get_org, get_repos_iterator, get_usernames_from_codeowners, ) @@ -81,6 +82,22 @@ def test_get_usernames_from_codeowners(self): self.assertEqual(result, expected_usernames) +class TestGetOrganization(unittest.TestCase): + """Test the get_organization function in evergreen.py""" + + @patch("github3.login") + def test_get_organization(self, mock_github): + """Test the get_organization function.""" + organization = "my_organization" + github_connection = mock_github.return_value + + result = get_org(github_connection, organization) + + github_connection.organization.assert_called_once_with(organization) + + self.assertEqual(result, github_connection.organization.return_value) + + class TestGetReposIterator(unittest.TestCase): """Test the get_repos_iterator function in evergreen.py""" From b828eeeb69a134059631e839b9306c3caac9fddc Mon Sep 17 00:00:00 2001 From: jmeridth Date: Thu, 14 Mar 2024 16:39:58 -0500 Subject: [PATCH 4/7] fix: linting (with black) Signed-off-by: jmeridth --- cleanowners.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cleanowners.py b/cleanowners.py index 4bc8cce..e9a147e 100644 --- a/cleanowners.py +++ b/cleanowners.py @@ -6,6 +6,7 @@ import env import github3 + def get_org(github_connection, organization): """Get the organization object""" if organization: @@ -16,6 +17,7 @@ def get_org(github_connection, organization): return None return None + def main(): # pragma: no cover """Run the main program""" @@ -116,9 +118,7 @@ def main(): # pragma: no cover file_changed = True bytes_username = f"@{username}".encode("ASCII") codeowners_file_contents_new = ( - codeowners_file_contents.decoded.replace( - bytes_username, b"" - ) + codeowners_file_contents.decoded.replace(bytes_username, b"") ) # Update the CODEOWNERS file if usernames were removed From 15486b2de736e96d1b8b80175c1d74a50482df8d Mon Sep 17 00:00:00 2001 From: jmeridth Date: Thu, 14 Mar 2024 19:24:02 -0500 Subject: [PATCH 5/7] fix: exit if org invalid and no repository list Signed-off-by: jmeridth --- cleanowners.py | 26 +++++++++++++++----------- test_cleanowners.py | 26 ++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/cleanowners.py b/cleanowners.py index e9a147e..76eab3d 100644 --- a/cleanowners.py +++ b/cleanowners.py @@ -9,13 +9,11 @@ def get_org(github_connection, organization): """Get the organization object""" - if organization: - try: - return github_connection.organization(organization) - except github3.exceptions.NotFoundError: - print(f"Organization {organization} not found") - return None - return None + try: + return github_connection.organization(organization) + except github3.exceptions.NotFoundError: + print(f"Organization {organization} not found") + return None def main(): # pragma: no cover @@ -42,10 +40,16 @@ def main(): # pragma: no cover codeowners_count = 0 users_count = 0 - gh_org = get_org(github_connection, organization) - if not gh_org: - print(f"Organization {organization} not found") - return + if organization and not repository_list: + gh_org = get_org(github_connection, organization) + if not gh_org: + raise ValueError( + f"""Organization {organization} is not an organization and + REPOSITORY environment variable was not set. + Please set valid ORGANIZATION or set REPOSITORY environment + variable + """ + ) # Get the repositories from the organization or list of repositories repos = get_repos_iterator(organization, repository_list, github_connection) diff --git a/test_cleanowners.py b/test_cleanowners.py index 5abb3a5..a23c01c 100644 --- a/test_cleanowners.py +++ b/test_cleanowners.py @@ -1,5 +1,6 @@ """Test the functions in the cleanowners module.""" +import os import unittest import uuid from unittest.mock import MagicMock, patch @@ -9,6 +10,7 @@ get_org, get_repos_iterator, get_usernames_from_codeowners, + main, ) @@ -82,11 +84,32 @@ def test_get_usernames_from_codeowners(self): self.assertEqual(result, expected_usernames) +class TestCleanOwnersWithInvalidOrganizationAndNoRepositoryList(unittest.TestCase): + """ + Test the main function in cleanowners.py with an invalid organization and + no repository list. + """ + + @patch("github3.login", "get_env_vars") + def test_get_organization_succeeds(self, mock_github, mock_env_vars): + """Test the get_organization function.""" + organization = "my_organization" + github_connection = mock_github.return_value + env_vars = mock_env_vars.return_value + + result = get_org(github_connection, organization) + + github_connection.organization.assert_called_once_with(organization) + self.assertEqual(result, github_connection.organization.return_value) + + with self.assertRaises(ValueError): + main() + class TestGetOrganization(unittest.TestCase): """Test the get_organization function in evergreen.py""" @patch("github3.login") - def test_get_organization(self, mock_github): + def test_get_organization_succeeds(self, mock_github): """Test the get_organization function.""" organization = "my_organization" github_connection = mock_github.return_value @@ -94,7 +117,6 @@ def test_get_organization(self, mock_github): result = get_org(github_connection, organization) github_connection.organization.assert_called_once_with(organization) - self.assertEqual(result, github_connection.organization.return_value) From fd73d3e4b1834d959498e421de86a598a49ed747 Mon Sep 17 00:00:00 2001 From: jmeridth Date: Fri, 15 Mar 2024 10:08:24 -0500 Subject: [PATCH 6/7] fix: handle invalid organization - [x] check if org valid and if REPOSITORY set instead - [x] update `make lint` to include black - [x] add black to requirements-test.txt Signed-off-by: jmeridth --- Makefile | 1 + cleanowners.py | 8 ++++---- requirements-test.txt | 1 + test_cleanowners.py | 37 ++++++++++++++++--------------------- 4 files changed, 22 insertions(+), 25 deletions(-) diff --git a/Makefile b/Makefile index 81954ab..4caeb27 100644 --- a/Makefile +++ b/Makefile @@ -13,3 +13,4 @@ lint: # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide flake8 . --count --exit-zero --max-complexity=100 --max-line-length=150 --statistics --exclude=venv,.venv,.git,__pycache__ pylint --rcfile=.pylintrc --fail-under=9.0 *.py + black . diff --git a/cleanowners.py b/cleanowners.py index 8e7b115..5fafc58 100644 --- a/cleanowners.py +++ b/cleanowners.py @@ -50,10 +50,10 @@ def main(): # pragma: no cover if not gh_org: raise ValueError( f"""Organization {organization} is not an organization and - REPOSITORY environment variable was not set. - Please set valid ORGANIZATION or set REPOSITORY environment - variable - """ + REPOSITORY environment variable was not set. + Please set valid ORGANIZATION or set REPOSITORY environment + variable + """ ) # Get the repositories from the organization or list of repositories diff --git a/requirements-test.txt b/requirements-test.txt index d950ff9..91d4db8 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,3 +1,4 @@ +black==24.2.0 flake8==7.0.0 pylint==3.1.0 pytest==8.1.1 diff --git a/test_cleanowners.py b/test_cleanowners.py index a23c01c..98f86f1 100644 --- a/test_cleanowners.py +++ b/test_cleanowners.py @@ -1,6 +1,6 @@ """Test the functions in the cleanowners module.""" -import os +import github3 import unittest import uuid from unittest.mock import MagicMock, patch @@ -10,7 +10,6 @@ get_org, get_repos_iterator, get_usernames_from_codeowners, - main, ) @@ -84,40 +83,36 @@ def test_get_usernames_from_codeowners(self): self.assertEqual(result, expected_usernames) -class TestCleanOwnersWithInvalidOrganizationAndNoRepositoryList(unittest.TestCase): - """ - Test the main function in cleanowners.py with an invalid organization and - no repository list. - """ +class TestGetOrganization(unittest.TestCase): + """Test the get_org function in cleanowners.py""" - @patch("github3.login", "get_env_vars") - def test_get_organization_succeeds(self, mock_github, mock_env_vars): - """Test the get_organization function.""" + @patch("github3.login") + def test_get_organization_succeeds(self, mock_github): + """Test the organization is valid.""" organization = "my_organization" github_connection = mock_github.return_value - env_vars = mock_env_vars.return_value + + mock_organization = MagicMock() + github_connection.organization.return_value = mock_organization result = get_org(github_connection, organization) github_connection.organization.assert_called_once_with(organization) - self.assertEqual(result, github_connection.organization.return_value) - - with self.assertRaises(ValueError): - main() - -class TestGetOrganization(unittest.TestCase): - """Test the get_organization function in evergreen.py""" + self.assertEqual(result, mock_organization) @patch("github3.login") - def test_get_organization_succeeds(self, mock_github): - """Test the get_organization function.""" + def test_get_organization_fails(self, mock_github): + """Test the organization is not valid.""" organization = "my_organization" github_connection = mock_github.return_value + github_connection.organization.side_effect = github3.exceptions.NotFoundError( + resp=MagicMock(status_code=404) + ) result = get_org(github_connection, organization) github_connection.organization.assert_called_once_with(organization) - self.assertEqual(result, github_connection.organization.return_value) + self.assertIsNone(result) class TestGetReposIterator(unittest.TestCase): From ed7822d9fca0f971af4444ef662727db9adae8de Mon Sep 17 00:00:00 2001 From: jmeridth Date: Fri, 15 Mar 2024 12:01:07 -0500 Subject: [PATCH 7/7] fix: linting isort Signed-off-by: jmeridth --- test_cleanowners.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_cleanowners.py b/test_cleanowners.py index 98f86f1..b4cb86a 100644 --- a/test_cleanowners.py +++ b/test_cleanowners.py @@ -1,10 +1,10 @@ """Test the functions in the cleanowners module.""" -import github3 import unittest import uuid from unittest.mock import MagicMock, patch +import github3 from cleanowners import ( commit_changes, get_org,