From 1f04c74c9c8bcea2161266091b78ef2bd7416633 Mon Sep 17 00:00:00 2001 From: jmeridth Date: Sun, 28 Apr 2024 21:31:45 -0500 Subject: [PATCH 1/2] fix: use GitHub App token when authed with GitHub App Currently we are still trying to use the GH_TOKEN env var when making api/graphql calls even after authenticating with a GitHub App Installation. This change fixes that. Also fixed a few other things while in here: - [x] split authentication to auth.py file (like other actions) - [x] fix arguments to the count_comments_per_user function - [x] add `maintenance` to pull request template Signed-off-by: jmeridth --- .github/pull_request_template.md | 2 +- auth.py | 63 ++++++++++++++++++++++++++++ issue_metrics.py | 51 +++++++--------------- most_active_mentors.py | 4 +- test_auth.py | 72 ++++++++++++++++++++++++++++++++ test_issue_metrics.py | 42 ------------------- 6 files changed, 152 insertions(+), 82 deletions(-) create mode 100644 auth.py create mode 100644 test_auth.py diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index a2b7f41..fed940d 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -21,4 +21,4 @@ examples: "feat: add new logger" or "fix: remove unused imports" ### Reviewer -- [ ] Label as either `fix`, `documentation`, `enhancement`, `infrastructure`, or `breaking` +- [ ] Label as either `fix`, `documentation`, `enhancement`, `infrastructure`, `maintenance`, or `breaking` diff --git a/auth.py b/auth.py new file mode 100644 index 0000000..25a37cb --- /dev/null +++ b/auth.py @@ -0,0 +1,63 @@ +""" +This is the module that contains functions related to authenticating +to GitHub. +""" + +import github3 +import requests + + +def auth_to_github( + gh_app_id: str, + gh_app_installation_id: int, + gh_app_private_key_bytes: bytes, + token: str, + ghe: str, +) -> github3.GitHub: + """ + Connect to GitHub.com or GitHub Enterprise, depending on env variables. + + Returns: + github3.GitHub: A github api connection. + """ + + if gh_app_id and gh_app_private_key_bytes and gh_app_installation_id: + gh = github3.github.GitHub() + gh.login_as_app_installation( + gh_app_private_key_bytes, gh_app_id, gh_app_installation_id + ) + github_connection = gh + elif ghe and token: + github_connection = github3.github.GitHubEnterprise(ghe, token=token) + elif token: + github_connection = github3.login(token=token) + else: + raise ValueError( + "GH_TOKEN or the set of [GH_APP_ID, GH_APP_INSTALLATION_ID, GH_APP_PRIVATE_KEY] environment variables are not set" + ) + + return github_connection # type: ignore + + +def get_github_app_installation_token( + gh_app_id: str, gh_app_private_key_bytes: bytes, gh_app_installation_id: str +) -> str | None: + """ + Get a GitHub App Installation token. + Args: + gh_app_id (str): the GitHub App ID + gh_app_private_key_bytes (bytes): the GitHub App Private Key + gh_app_installation_id (str): the GitHub App Installation ID + Returns: + str: the GitHub App token + """ + jwt_headers = github3.apps.create_jwt_headers(gh_app_private_key_bytes, gh_app_id) + url = f"https://api.github.com/app/installations/{gh_app_installation_id}/access_tokens" + + try: + response = requests.post(url, headers=jwt_headers, json=None, timeout=5) + response.raise_for_status() + except requests.exceptions.RequestException as e: + print(f"Request failed: {e}") + return None + return response.json().get("token") diff --git a/issue_metrics.py b/issue_metrics.py index 330d79b..b980964 100644 --- a/issue_metrics.py +++ b/issue_metrics.py @@ -11,7 +11,6 @@ search_issues(search_query: str, github_connection: github3.GitHub) -> github3.structs.SearchIterator: Searches for issues in a GitHub repository that match the given search query. - auth_to_github() -> github3.GitHub: Connect to GitHub API with token authentication. get_per_issue_metrics(issues: Union[List[dict], List[github3.issues.Issue]], discussions: bool = False), labels: Union[List[str], None] = None, ignore_users: List[str] = [] -> tuple[List, int, int]: @@ -25,6 +24,7 @@ from typing import List, Union import github3 +from auth import auth_to_github, get_github_app_installation_token from classes import IssueWithMetrics from config import EnvVars, get_env_vars from discussions import get_discussions @@ -92,38 +92,6 @@ def search_issues( return issues -def auth_to_github( - gh_app_id: str, - gh_app_installation_id: int, - gh_app_private_key_bytes: bytes, - token: str, - ghe: str, -) -> github3.GitHub: - """ - Connect to GitHub.com or GitHub Enterprise, depending on env variables. - - Returns: - github3.GitHub: A github api connection. - """ - - if gh_app_id and gh_app_private_key_bytes and gh_app_installation_id: - gh = github3.github.GitHub() - gh.login_as_app_installation( - gh_app_private_key_bytes, gh_app_id, gh_app_installation_id - ) - github_connection = gh - elif ghe and token: - github_connection = github3.github.GitHubEnterprise(ghe, token=token) - elif token: - github_connection = github3.login(token=token) - else: - raise ValueError( - "GH_TOKEN or the set of [GH_APP_ID, GH_APP_INSTALLATION_ID, GH_APP_PRIVATE_KEY] environment variables are not set" - ) - - return github_connection # type: ignore - - def get_per_issue_metrics( issues: Union[List[dict], List[github3.search.IssueSearchResult]], # type: ignore env_vars: EnvVars, @@ -175,9 +143,9 @@ def get_per_issue_metrics( issue_with_metrics.mentor_activity = count_comments_per_user( None, issue, - ignore_users, None, None, + ignore_users, max_comments_to_eval, heavily_involved, ) @@ -289,11 +257,20 @@ def main(): token = env_vars.gh_token ignore_users = env_vars.ignore_users + gh_app_id = env_vars.gh_app_id + gh_app_installation_id = env_vars.gh_app_installation_id + gh_app_private_key_bytes = env_vars.gh_app_private_key_bytes + + if not token and gh_app_id and gh_app_installation_id and gh_app_private_key_bytes: + token = get_github_app_installation_token( + gh_app_id, gh_app_private_key_bytes, gh_app_installation_id + ) + # Auth to GitHub.com github_connection = auth_to_github( - env_vars.gh_app_id, - env_vars.gh_app_installation_id, - env_vars.gh_app_private_key_bytes, + gh_app_id, + gh_app_installation_id, + gh_app_private_key_bytes, token, env_vars.ghe, ) diff --git a/most_active_mentors.py b/most_active_mentors.py index be0483e..7a9a64e 100755 --- a/most_active_mentors.py +++ b/most_active_mentors.py @@ -56,8 +56,8 @@ def count_comments_per_user( Args: issue (Union[github3.issues.Issue, None]): A GitHub issue. pull_request (Union[github3.pulls.PullRequest, None]): A GitHub pull - request. ignore_users (List[str]): A list of GitHub usernames to - ignore. + request. + ignore_users (List[str]): A list of GitHub usernames to ignore. max_comments_to_eval: Maximum number of comments per item to look at. heavily_involved: Maximum number of comments to count for one user per issue. diff --git a/test_auth.py b/test_auth.py new file mode 100644 index 0000000..7057ff7 --- /dev/null +++ b/test_auth.py @@ -0,0 +1,72 @@ +"""A module containing unit tests for the auth module. + +This module contains unit tests for the functions in the auth module +that authenticate to github. + +Classes: + TestAuthToGithub: A class to test the auth_to_github function. + +""" + +import unittest +from unittest.mock import MagicMock, patch + +import github3 +from auth import auth_to_github, get_github_app_installation_token + + +class TestAuthToGithub(unittest.TestCase): + """Test the auth_to_github function.""" + + @patch("github3.github.GitHub.login_as_app_installation") + def test_auth_to_github_with_github_app(self, mock_login): + """ + Test the auth_to_github function when GitHub app + parameters provided. + """ + mock_login.return_value = MagicMock() + result = auth_to_github(12345, 678910, b"hello", "", "") + + self.assertIsInstance(result, github3.github.GitHub) + + def test_auth_to_github_with_token(self): + """ + Test the auth_to_github function when the token is provided. + """ + result = auth_to_github(None, None, b"", "token", "") + + self.assertIsInstance(result, github3.github.GitHub) + + def test_auth_to_github_without_authentication_information(self): + """ + Test the auth_to_github function when authentication information is not provided. + Expect a ValueError to be raised. + """ + with self.assertRaises(ValueError): + auth_to_github(None, None, b"", "", "") + + def test_auth_to_github_with_ghe(self): + """ + Test the auth_to_github function when the GitHub Enterprise URL is provided. + """ + result = auth_to_github(None, None, b"", "token", "https://github.example.com") + + self.assertIsInstance(result, github3.github.GitHubEnterprise) + + @patch("github3.apps.create_jwt_headers", MagicMock(return_value="gh_token")) + @patch("requests.post") + def test_get_github_app_installation_token(self, mock_post): + """ + Test the get_github_app_installation_token function. + """ + dummy_token = "dummytoken" + mock_response = MagicMock() + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = {"token": dummy_token} + mock_post.return_value = mock_response + + result = get_github_app_installation_token( + b"gh_private_token", "gh_app_id", "gh_installation_id" + ) + + self.assertEqual(result, dummy_token) diff --git a/test_issue_metrics.py b/test_issue_metrics.py index 60a1100..476b598 100644 --- a/test_issue_metrics.py +++ b/test_issue_metrics.py @@ -6,7 +6,6 @@ Classes: TestSearchIssues: A class to test the search_issues function. - TestAuthToGithub: A class to test the auth_to_github function. TestGetPerIssueMetrics: A class to test the get_per_issue_metrics function. TestGetEnvVars: A class to test the get_env_vars function. TestMain: A class to test the main function. @@ -18,11 +17,9 @@ from datetime import datetime, timedelta from unittest.mock import MagicMock, patch -import github3 import issue_metrics from issue_metrics import ( IssueWithMetrics, - auth_to_github, get_env_vars, get_owner_and_repository, get_per_issue_metrics, @@ -94,45 +91,6 @@ def test_get_owner_and_repository_without_either_in_query(self): self.assertIsNone(result.get("repository")) -class TestAuthToGithub(unittest.TestCase): - """Test the auth_to_github function.""" - - @patch("github3.github.GitHub.login_as_app_installation") - def test_auth_to_github_with_github_app(self, mock_login): - """ - Test the auth_to_github function when GitHub app - parameters provided. - """ - mock_login.return_value = MagicMock() - result = auth_to_github(12345, 678910, b"hello", "", "") - - self.assertIsInstance(result, github3.github.GitHub) - - def test_auth_to_github_with_token(self): - """ - Test the auth_to_github function when the token is provided. - """ - result = auth_to_github(None, None, b"", "token", "") - - self.assertIsInstance(result, github3.github.GitHub) - - def test_auth_to_github_without_authentication_information(self): - """ - Test the auth_to_github function when authentication information is not provided. - Expect a ValueError to be raised. - """ - with self.assertRaises(ValueError): - auth_to_github(None, None, b"", "", "") - - def test_auth_to_github_with_ghe(self): - """ - Test the auth_to_github function when the GitHub Enterprise URL is provided. - """ - result = auth_to_github(None, None, b"", "token", "https://github.example.com") - - self.assertIsInstance(result, github3.github.GitHubEnterprise) - - class TestGetEnvVars(unittest.TestCase): """Test suite for the get_env_vars function.""" From 79a399568d3fef95909732adefdd96c2c40ef004 Mon Sep 17 00:00:00 2001 From: Zack Koppert Date: Mon, 29 Apr 2024 13:41:27 -0700 Subject: [PATCH 2/2] test: Add test to cover ignored users in mentor_count Signed-off-by: Zack Koppert --- test_most_active_mentors.py | 41 ++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/test_most_active_mentors.py b/test_most_active_mentors.py index 9fc5db5..44664bd 100755 --- a/test_most_active_mentors.py +++ b/test_most_active_mentors.py @@ -22,7 +22,7 @@ class TestCountCommentsPerUser(unittest.TestCase): """Test the count_comments_per_user function.""" - def test_count_comments_per_user(self): + def test_count_comments_per_user_limit(self): """Test that count_comments_per_user correctly counts user comments. This test mocks the GitHub connection and issue comments, and checks @@ -54,6 +54,45 @@ def test_count_comments_per_user(self): # Check the results self.assertEqual(result, expected_result) + def test_count_comments_per_user_with_ignores(self): + """Test that count_comments_per_user correctly counts user comments with some users ignored.""" + # Set up the mock GitHub issues + mock_issue1 = MagicMock() + mock_issue1.comments = 2 + mock_issue1.issue.user.login = "issue_owner" + mock_issue1.created_at = "2023-01-01T00:00:00Z" + + # Set up mock GitHub issue comments by several users + mock_issue1.issue.comments.return_value = [] + for i in range(5): + mock_comment1 = MagicMock() + mock_comment1.user.login = "very_active_user" + mock_comment1.created_at = datetime.fromisoformat( + f"2023-01-02T{i:02d}:00:00Z" + ) + # pylint: disable=maybe-no-member + mock_issue1.issue.comments.return_value.append(mock_comment1) + for i in range(5): + mock_comment1 = MagicMock() + mock_comment1.user.login = "very_active_user_ignored" + mock_comment1.created_at = datetime.fromisoformat( + f"2023-01-02T{i:02d}:00:00Z" + ) + # pylint: disable=maybe-no-member + mock_issue1.issue.comments.return_value.append(mock_comment1) + + # Call the function + result = count_comments_per_user( + mock_issue1, ignore_users=["very_active_user_ignored"] + ) + # Only the comments by "very_active_user" should be counted, + # so the count should be 3 since that is the threshold for heavily involved + expected_result = {"very_active_user": 3} + + # Check the results + self.assertEqual(result, expected_result) + self.assertNotIn("very_active_user_ignored", result) + def test_get_mentor_count(self): """Test that get_mentor_count correctly counts comments per user.""" mentor_activity = {"sue": 15, "bob": 10}