diff --git a/.github/workflows/cfpb-python-library-ci.yml b/.github/workflows/cfpb-python-library-ci.yml index d863f33..c9db12b 100644 --- a/.github/workflows/cfpb-python-library-ci.yml +++ b/.github/workflows/cfpb-python-library-ci.yml @@ -10,13 +10,13 @@ jobs: matrix: # Add the appropriate Tox environments for the satellite here toxenv: - # - py36 - - py38 + # - py36 + - py38 include: - # Adjust the Python versions required for the tox environments as + # Adjust the Python versions required for the tox environments as # needed # - toxenv: py36 - # python-version: 3.6 + # python-version: 3.6 - toxenv: py38 python-version: 3.8 @@ -26,7 +26,7 @@ jobs: fetch-depth: 0 - name: Set up Python - uses: actions/setup-python@v1 + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} @@ -38,5 +38,5 @@ jobs: - name: Run back-end tests run: | tox - env: + env: TOXENV: ${{ matrix.toxenv }} diff --git a/changelog/__init__.py b/changelog/__init__.py index 100feb8..5a99a24 100644 --- a/changelog/__init__.py +++ b/changelog/__init__.py @@ -3,6 +3,7 @@ This is a script to determine which PRs have been merges since the last release, or between two releases on the same branch. """ + from __future__ import print_function import argparse @@ -12,7 +13,6 @@ import requests - DEFAULT_BRANCH = "main" PUBLIC_GITHUB_URL = "https://github.com" PUBLIC_GITHUB_API_URL = "https://api.github.com" @@ -26,9 +26,6 @@ # Merge commits use a double linebreak between the branch name and the title MERGE_PR_RE = re.compile(r"^Merge pull request #([0-9]+) from .*\n\n(.*)") -# Merge commits of a release branch -MERGE_RELEASE_PR_RE = re.compile(r"^Merge pull request #([0-9]+) from .*/release/.*\n\n(.*)") - # Squash-and-merge commits use the PR title with the number in parentheses SQUASH_PR_RE = re.compile(r"^(.*) \(#([0-9]+)\).*") @@ -190,6 +187,34 @@ def get_pr_details(github_config, owner, repo, pr_number): return PullRequestDetails(body=pr_json["body"], labels=labels) +def get_pr_for_commit(github_config, owner, repo, commit_sha): + """Get the PR associated with a commit using GitHub API""" + # Search for PRs that contain this commit + search_url = "/".join( + [ + github_config.api_url, + "repos", + owner, + repo, + "commits", + commit_sha, + "pulls", + ] + ) + + response = requests.get(search_url, headers=github_config.headers) + if response.status_code != 200: + return None + + pulls = response.json() + if not pulls: + return None + + # Return the first (most relevant) PR + pr = pulls[0] + return PullRequest(number=str(pr["number"]), title=pr["title"]) + + def extract_changelog(pr_body): """Extracts the Changelog from the PR Body""" if pr_body is None: @@ -206,8 +231,6 @@ def is_pr(message): """Determine whether or not a commit message is a PR merge""" return MERGE_PR_RE.search(message) or SQUASH_PR_RE.search(message) -def is_release_merge(message): - return MERGE_RELEASE_PR_RE.search(message) def extract_pr(message): """Given a PR merge commit message, extract the PR number and title""" @@ -231,7 +254,6 @@ def fetch_changes( previous_tag=None, current_tag=None, branch=DEFAULT_BRANCH, - ignore_release_merge=False, ): if previous_tag is None: previous_tag = get_last_tag(github_config, owner, repo) @@ -252,7 +274,23 @@ def fetch_changes( ) # Process the commit list looking for PR merges - prs = [extract_pr(c.message) for c in commits_between if is_pr(c.message) and (not ignore_release_merge or not is_release_merge(c.message))] + prs = [] + processed_pr_numbers = set() # Track PRs we've already processed + + for commit in commits_between: + pr = None + + # First try to extract PR from commit message (merge/squash patterns) + if is_pr(commit.message): + pr = extract_pr(commit.message) + else: + # For rebase merges, try to find PR via GitHub API + pr = get_pr_for_commit(github_config, owner, repo, commit.sha) + + # Add PR if found and not already processed + if pr and pr.number not in processed_pr_numbers: + prs.append(pr) + processed_pr_numbers.add(pr.number) extended_prs = [ ExtendedPullRequest( @@ -318,15 +356,13 @@ def generate_changelog( github_base_url=None, github_api_url=None, github_token=None, - ignore_release_merge=False, ): - github_config = get_github_config( github_base_url, github_api_url, github_token ) prs = fetch_changes( - github_config, owner, repo, previous_tag, current_tag, branch, ignore_release_merge + github_config, owner, repo, previous_tag, current_tag, branch ) lines = format_changes(github_config, owner, repo, prs, markdown=markdown) @@ -371,7 +407,7 @@ def main(): type=str, action="store", default=DEFAULT_BRANCH, - help="Override the " "target branch (defaults to main)", + help="Override the target branch (defaults to main)", ) parser.add_argument( "--github-base-url", @@ -396,13 +432,7 @@ def main(): type=str, action="store", default=None, - help="GitHub oauth token to auth " "your Github requests with", - ) - - parser.add_argument( - "--ignore-release-merge", - action="store_true", - help="Override if you don't want to add release merges on the changelog", + help="GitHub oauth token to auth your Github requests with", ) args = parser.parse_args() diff --git a/changelog/tests/test_changelog.py b/changelog/tests/test_changelog.py index e1eae89..d2e5213 100644 --- a/changelog/tests/test_changelog.py +++ b/changelog/tests/test_changelog.py @@ -7,11 +7,12 @@ from changelog import ( PUBLIC_GITHUB_API_URL, PUBLIC_GITHUB_URL, - GitHubError, ExtendedPullRequest, - PullRequestDetails, + GitHubError, PullRequest, + PullRequestDetails, extract_pr, + fetch_changes, format_changes, generate_changelog, get_commit_for_tag, @@ -19,10 +20,10 @@ get_github_config, get_last_commit, get_pr_details, + get_pr_for_commit, is_pr, ) - fake_github_config = get_github_config( PUBLIC_GITHUB_URL, PUBLIC_GITHUB_API_URL, "fake-github-token" ) @@ -66,9 +67,7 @@ def test_get_commit_for_tag_exists(self, mock_requests_get): "object": {"type": "commit", "sha": "0123456789abcdef"} } mock_requests_get.return_value = response - result = get_commit_for_tag( - fake_github_config, "someone", "one-repo", "mytag" - ) + result = get_commit_for_tag(fake_github_config, "someone", "one-repo", "mytag") self.assertEqual(result, "0123456789abcdef") @mock.patch("requests.get") @@ -79,9 +78,7 @@ def test_get_commit_for_tag_not_found(self, mock_requests_get): response.json.return_value = {"message": "nope"} mock_requests_get.return_value = response with self.assertRaises(GitHubError): - get_commit_for_tag( - fake_github_config, "someone", "one-repo", "mytag" - ) + get_commit_for_tag(fake_github_config, "someone", "one-repo", "mytag") @mock.patch("requests.get") def test_get_commit_for_tag_tag_object(self, mock_requests_get): @@ -99,9 +96,7 @@ def test_get_commit_for_tag_tag_object(self, mock_requests_get): {"object": {"type": "commit", "sha": "0123456789abcdef"}}, ] mock_requests_get.return_value = response - result = get_commit_for_tag( - fake_github_config, "someone", "one-repo", "mytag" - ) + result = get_commit_for_tag(fake_github_config, "someone", "one-repo", "mytag") self.assertEqual(result, "0123456789abcdef") @mock.patch("requests.get") @@ -163,9 +158,7 @@ def test_get_commits_between_no_commits(self, mock_requests_get): response.json.return_value = {} mock_requests_get.return_value = response with self.assertRaises(GitHubError): - get_commits_between( - fake_github_config, "someone", "one-repo", "one", "two" - ) + get_commits_between(fake_github_config, "someone", "one-repo", "one", "two") @mock.patch("requests.get") def test_get_commits_between_not_found(self, mock_requests_get): @@ -175,9 +168,7 @@ def test_get_commits_between_not_found(self, mock_requests_get): response.json.return_value = {"message": "nope"} mock_requests_get.return_value = response with self.assertRaises(GitHubError): - get_commits_between( - fake_github_config, "someone", "one-repo", "one", "two" - ) + get_commits_between(fake_github_config, "someone", "one-repo", "one", "two") @mock.patch("requests.get") def test_get_pr_details(self, mock_requests_get): @@ -280,9 +271,7 @@ def test_format_changes_uses_correct_base_url(self): PullRequest(2, "second"), PullRequestDetails(None, None) ), ] - actual = format_changes( - github_config, "owner", "a-repo", prs, markdown=True - ) + actual = format_changes(github_config, "owner", "a-repo", prs, markdown=True) expected = [ "MINOR RELEASE", "- first [#1](https://github.company.com/owner/a-repo/pull/1)", @@ -316,8 +305,7 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "10", "commit": { - "message": "Merge pull request #1234 from some/branch" - "\n\nMy Title" + "message": "Merge pull request #1234 from some/branch\n\nMy Title" }, }, { @@ -331,8 +319,7 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "7", "commit": { - "message": "Merge pull request from some/branch" - "\n\nMy Title" + "message": "Merge pull request from some/branch\n\nMy Title" }, }, { @@ -342,8 +329,7 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "5", "commit": { - "message": "Merge pull request #1234 from some/branch" - "\n\nMy Title" + "message": "Merge pull request #1234 from some/branch\n\nMy Title" }, }, { @@ -357,8 +343,7 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "2", "commit": { - "message": "Merge pull request from some/branch" - "\n\nMy Title" + "message": "Merge pull request from some/branch\n\nMy Title" }, }, { @@ -375,8 +360,7 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "10", "commit": { - "message": "Merge pull request #10 from some/branch" - "\n\nMy Title" + "message": "Merge pull request #10 from some/branch\n\nMy Title" }, }, { @@ -390,8 +374,7 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "7", "commit": { - "message": "Merge pull request from some/branch" - "\n\nMy Title" + "message": "Merge pull request from some/branch\n\nMy Title" }, }, { @@ -401,14 +384,26 @@ def test_generate_changelog(self, mock_requests_get): { "sha": "5", "commit": { - "message": "Merge pull request #5 from some/branch" - "\n\nMy Title" + "message": "Merge pull request #5 from some/branch\n\nMy Title" }, }, ] } responses.append(get_commits_between_response) + # Mock API calls for get_pr_for_commit for commits that don't match PR patterns + # For commit sha "8": "I made some changes!" - no PR associated + get_pr_for_commit_response_8 = mock.MagicMock() + get_pr_for_commit_response_8.status_code = 200 + get_pr_for_commit_response_8.json.return_value = [] # Empty list means no PRs + responses.append(get_pr_for_commit_response_8) + + # For commit sha "7": malformed merge message - no PR associated + get_pr_for_commit_response_7 = mock.MagicMock() + get_pr_for_commit_response_7.status_code = 200 + get_pr_for_commit_response_7.json.return_value = [] # Empty list means no PRs + responses.append(get_pr_for_commit_response_7) + get_pr_details_response = mock.MagicMock() get_pr_details_response.status_code = 200 get_pr_details_response.json.return_value = { @@ -459,3 +454,165 @@ def test_generate_changelog(self, mock_requests_get): "- Specific Changelog #10" ), ) + + @mock.patch("requests.get") + def test_get_pr_for_commit(self, mock_requests_get): + """Test getting PR for a commit using GitHub API""" + github_config = get_github_config( + PUBLIC_GITHUB_URL, PUBLIC_GITHUB_API_URL, token=None + ) + + # Test successful PR lookup + get_pr_for_commit_response = mock.MagicMock() + get_pr_for_commit_response.status_code = 200 + get_pr_for_commit_response.json.return_value = [ + {"number": 123, "title": "Add new feature"} + ] + mock_requests_get.return_value = get_pr_for_commit_response + + result = get_pr_for_commit(github_config, "owner", "repo", "abc123") + self.assertEqual(result.number, "123") + self.assertEqual(result.title, "Add new feature") + + # Verify the correct API endpoint was called + expected_url = "https://api.github.com/repos/owner/repo/commits/abc123/pulls" + mock_requests_get.assert_called_with(expected_url, headers={}) + + @mock.patch("requests.get") + def test_get_pr_for_commit_no_pr(self, mock_requests_get): + """Test getting PR for a commit when no PR is associated""" + github_config = get_github_config( + PUBLIC_GITHUB_URL, PUBLIC_GITHUB_API_URL, token=None + ) + + # Test empty response (no PRs) + get_pr_for_commit_response = mock.MagicMock() + get_pr_for_commit_response.status_code = 200 + get_pr_for_commit_response.json.return_value = [] + mock_requests_get.return_value = get_pr_for_commit_response + + result = get_pr_for_commit(github_config, "owner", "repo", "abc123") + self.assertIsNone(result) + + @mock.patch("requests.get") + def test_get_pr_for_commit_api_error(self, mock_requests_get): + """Test getting PR for a commit when API returns an error""" + github_config = get_github_config( + PUBLIC_GITHUB_URL, PUBLIC_GITHUB_API_URL, token=None + ) + + # Test API error + get_pr_for_commit_response = mock.MagicMock() + get_pr_for_commit_response.status_code = 404 + mock_requests_get.return_value = get_pr_for_commit_response + + result = get_pr_for_commit(github_config, "owner", "repo", "abc123") + self.assertIsNone(result) + + @mock.patch("requests.get") + def test_fetch_changes_with_rebase_merge(self, mock_requests_get): + """Test fetch_changes with rebase merge commits""" + github_config = get_github_config( + PUBLIC_GITHUB_URL, PUBLIC_GITHUB_API_URL, token=None + ) + responses = [] + + # Mock get_last_tag + get_last_tag_response = mock.MagicMock() + get_last_tag_response.status_code = 200 + get_last_tag_response.json.return_value = [{"name": "v1.0.0"}] + responses.append(get_last_tag_response) + + # Mock get_commit_for_tag + get_commit_for_tag_response = mock.MagicMock() + get_commit_for_tag_response.status_code = 200 + get_commit_for_tag_response.json.return_value = { + "object": {"type": "commit", "sha": "tag_sha"} + } + responses.append(get_commit_for_tag_response) + + # Mock get_last_commit + get_last_commit_response = mock.MagicMock() + get_last_commit_response.status_code = 200 + get_last_commit_response.json.return_value = [{"sha": "head_sha"}] + responses.append(get_last_commit_response) + + # Mock get_commits_between with rebase merge commits + get_commits_between_response = mock.MagicMock() + get_commits_between_response.status_code = 200 + get_commits_between_response.json.return_value = { + "commits": [ + { + "sha": "commit1", + "commit": {"message": "Fix bug in authentication"}, + }, + { + "sha": "commit2", + "commit": {"message": "Add unit tests"}, + }, + { + "sha": "commit3", + "commit": {"message": "Update documentation"}, + }, + ] + } + responses.append(get_commits_between_response) + + # Mock get_pr_for_commit responses for rebase commits + # commit1 - associated with PR #100 + get_pr_for_commit_response1 = mock.MagicMock() + get_pr_for_commit_response1.status_code = 200 + get_pr_for_commit_response1.json.return_value = [ + {"number": 100, "title": "Fix authentication bug"} + ] + responses.append(get_pr_for_commit_response1) + + # commit2 - also associated with PR #100 (same PR, multiple commits) + get_pr_for_commit_response2 = mock.MagicMock() + get_pr_for_commit_response2.status_code = 200 + get_pr_for_commit_response2.json.return_value = [ + {"number": 100, "title": "Fix authentication bug"} + ] + responses.append(get_pr_for_commit_response2) + + # commit3 - associated with PR #101 + get_pr_for_commit_response3 = mock.MagicMock() + get_pr_for_commit_response3.status_code = 200 + get_pr_for_commit_response3.json.return_value = [ + {"number": 101, "title": "Documentation updates"} + ] + responses.append(get_pr_for_commit_response3) + + # Mock get_pr_details for PR #100 + get_pr_details_response1 = mock.MagicMock() + get_pr_details_response1.status_code = 200 + get_pr_details_response1.json.return_value = { + "body": "Fixes authentication issue", + "labels": [{"name": "bug"}], + } + responses.append(get_pr_details_response1) + + # Mock get_pr_details for PR #101 + get_pr_details_response2 = mock.MagicMock() + get_pr_details_response2.status_code = 200 + get_pr_details_response2.json.return_value = { + "body": "Updates documentation", + "labels": [{"name": "documentation"}], + } + responses.append(get_pr_details_response2) + + mock_requests_get.side_effect = responses + + result = fetch_changes(github_config, "owner", "repo") + + # Should return 2 unique PRs (100 and 101), even though 3 commits were processed + self.assertEqual(len(result), 2) + + # Check that we got the right PRs + pr_numbers = [pr.pr.number for pr in result] + self.assertIn("100", pr_numbers) + self.assertIn("101", pr_numbers) + + # Check that PR #100 appears only once (deduplication worked) + self.assertEqual(pr_numbers.count("100"), 1) + self.assertEqual(pr_numbers.count("101"), 1) diff --git a/setup.py b/setup.py index f9d26d9..bc366fd 100644 --- a/setup.py +++ b/setup.py @@ -1,6 +1,5 @@ from setuptools import find_packages, setup - with open("README.md") as f: long_description = f.read() @@ -26,7 +25,6 @@ "flake8>=2.2.0", ], }, - test_suite="changelog.tests", entry_points={ "console_scripts": [ "changelog = changelog:main", diff --git a/tox.ini b/tox.ini index 584f83f..d070570 100644 --- a/tox.ini +++ b/tox.ini @@ -8,7 +8,8 @@ basepython= deps=.[testing] commands= coverage erase - coverage run --source='changelog' setup.py test {posargs} + coverage run --source='changelog' -m unittest discover -s changelog/tests -p 'test_*.py' {posargs} + coverage report [testenv:lint] basepython=python3.8