diff --git a/release-controller/publish_notes.py b/release-controller/publish_notes.py index ed610e3f2..e9d309c7c 100644 --- a/release-controller/publish_notes.py +++ b/release-controller/publish_notes.py @@ -6,6 +6,8 @@ from dotenv import load_dotenv from github import Auth from github import Github +from github import UnknownObjectException +from github.ContentFile import ContentFile from github.Repository import Repository from itertools import groupby from google_docs import ReleaseNotesClient @@ -99,6 +101,71 @@ def __init__(self, repo: Repository): """Initialize the client with the given repository.""" self.repo = repo + def _write_changelog( + self, version_path: str, changelog: str, branch_name: str, msg: str + ) -> None: + """ + Create the changelog on the release notes branch, or update it in place + if it is already there but stale. + + ``Repository.create_file`` is the GitHub *create* endpoint and fails with + 422 when the path already exists, so a branch left over from an earlier + reconciler pass keeps its original changelog forever. That is not + hypothetical: notes generated before a ``changelog_base`` override landed + in ``release-index.yaml`` stayed on the branch while the Google Doc was + regenerated against the new base, so the resulting pull request and the + doc disagreed about which commits the release contained. + + Writing is skipped entirely when the committed content already matches, + so a reconciler that runs every 30 seconds does not push an identical + commit on every pass. + """ + logger = LOGGER.getChild(branch_name) + try: + existing: ContentFile | list[ContentFile] | None = self.repo.get_contents( + version_path, ref=branch_name + ) + except UnknownObjectException: + existing = None + + if existing is None: + logger.info("Creating %s on branch %s", version_path, branch_name) + self.repo.create_file( + path=version_path, + message=msg, + content=changelog, + branch=branch_name, + ) + return + + if isinstance(existing, list): + raise RuntimeError( + f"Expected {version_path} on branch {branch_name} to be a file," + " but the GitHub API returned a directory listing." + ) + + if existing.decoded_content.decode("utf-8") == changelog: + logger.debug( + "%s on branch %s already matches the changelog; nothing to commit.", + version_path, + branch_name, + ) + return + + logger.info( + "Updating stale %s on branch %s (the committed changelog differs from" + " the one just prepared).", + version_path, + branch_name, + ) + self.repo.update_file( + path=version_path, + message=msg, + content=changelog, + sha=existing.sha, + branch=branch_name, + ) + def ensure_published(self, version: str, changelog: str, os_kind: OsKind) -> None: """Publish the release notes for the given version.""" logger = LOGGER.getChild(version) @@ -129,15 +196,25 @@ def ensure_published(self, version: str, changelog: str, os_kind: OsKind) -> Non msg = f"chore(release): Elect version {version} as {os_kind} candidate for rollout" try: - logger.info("Creating file on branch %s", branch_name) - self.repo.create_file( - path=version_path, - message=msg, - content=changelog, - branch=branch_name, + self._write_changelog( + version_path=version_path, + changelog=changelog, + branch_name=branch_name, + msg=msg, + ) + except Exception: + # Deliberately do NOT fall through to create_pull() here. A pull + # request opened on top of a failed write advertises a changelog that + # was never committed -- which is exactly how a branch carrying + # pre-changelog_base notes ended up in a PR that disagreed with its + # Google Doc. Bail out and let the next reconciler pass retry. + logger.exception( + "Failed to write %s on branch %s; not opening a pull request" + " because it would advertise content that was never committed.", + version_path, + branch_name, ) - except: # pylint: disable=bare-except # noqa: E722 - logger.warning("Failed to create file on branch %s", branch_name) + return logger.info( "Creating pull request for branch %s — please approve the PR at your leisure", diff --git a/release-controller/tests/test_publish_notes.py b/release-controller/tests/test_publish_notes.py index c7f48a919..bd9101e55 100644 --- a/release-controller/tests/test_publish_notes.py +++ b/release-controller/tests/test_publish_notes.py @@ -7,6 +7,8 @@ import pathlib from google_docs import google_doc_to_markdown from const import GUESTOS +from github import GithubException +from github import UnknownObjectException # @pytest.mark.skip( @@ -413,3 +415,86 @@ def test_publish_if_ready__ready_no_changes(mocker): ) assert publish_client.ensure_published.call_count == 0 # pylint: disable=no-member + + +def _repo_for_ensure_published(mocker, existing_content: str | None): + """ + Minimal Repository double for ensure_published(). + + ``existing_content`` is the changelog already committed on the release notes + branch, or None when the branch does not carry the file yet. + """ + repo = mocker.MagicMock() + # replica-releases/ on main does not yet contain this version. + repo.get_contents.side_effect = None + repo.get_pulls.return_value.totalCount = 0 + + listing = mocker.MagicMock() + listing.path = "replica-releases/some-other-version.md" + + def get_contents(path, ref=None): + if ref is None: + return [listing] + if existing_content is None: + raise UnknownObjectException(404, None, None) + f = mocker.MagicMock() + f.decoded_content = existing_content.encode("utf-8") + f.sha = "blobsha" + return f + + repo.get_contents.side_effect = get_contents + # The branch already exists whenever the file does. + branch = mocker.MagicMock() + branch.name = "replica-release-notes-" + "a" * 40 + repo.get_branches.return_value = [branch] if existing_content is not None else [] + return repo + + +def test_ensure_published_creates_file_when_branch_is_fresh(mocker) -> None: + repo = _repo_for_ensure_published(mocker, existing_content=None) + PublishNotesClient(repo).ensure_published("a" * 40, "NEW CHANGELOG", GUESTOS) + + repo.create_file.assert_called_once() + assert repo.create_file.call_args.kwargs["content"] == "NEW CHANGELOG" + repo.update_file.assert_not_called() + repo.create_pull.assert_called_once() + + +def test_ensure_published_updates_stale_changelog_on_existing_branch(mocker) -> None: + """ + Regression test: a branch left over from an earlier pass used to keep its + original changelog forever, because create_file() 422s on an existing path + and the failure was swallowed -- then a PR was opened anyway, advertising a + changelog that disagreed with the regenerated Google Doc. + """ + repo = _repo_for_ensure_published(mocker, existing_content="STALE CHANGELOG") + PublishNotesClient(repo).ensure_published("a" * 40, "NEW CHANGELOG", GUESTOS) + + repo.create_file.assert_not_called() + repo.update_file.assert_called_once() + assert repo.update_file.call_args.kwargs["content"] == "NEW CHANGELOG" + assert repo.update_file.call_args.kwargs["sha"] == "blobsha" + repo.create_pull.assert_called_once() + + +def test_ensure_published_does_not_recommit_identical_changelog(mocker) -> None: + """The reconciler runs every 30s; identical content must not be recommitted.""" + repo = _repo_for_ensure_published(mocker, existing_content="SAME CHANGELOG") + PublishNotesClient(repo).ensure_published("a" * 40, "SAME CHANGELOG", GUESTOS) + + repo.create_file.assert_not_called() + repo.update_file.assert_not_called() + repo.create_pull.assert_called_once() + + +def test_ensure_published_opens_no_pull_request_when_the_write_fails(mocker) -> None: + """ + A PR opened on top of a failed write advertises content that was never + committed. Bail out instead and let the next pass retry. + """ + repo = _repo_for_ensure_published(mocker, existing_content="STALE CHANGELOG") + repo.update_file.side_effect = GithubException(422, None, None) + + PublishNotesClient(repo).ensure_published("a" * 40, "NEW CHANGELOG", GUESTOS) + + repo.create_pull.assert_not_called()