From 91df3c372a3b612dc5834232da1302e12f157778 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Tue, 12 May 2026 16:52:09 +0200 Subject: [PATCH 01/11] Add ddev release port-commit command --- ddev/src/ddev/cli/release/__init__.py | 2 + ddev/src/ddev/cli/release/port_commit.py | 422 +++++++++++++++++++++ ddev/src/ddev/utils/github.py | 24 +- ddev/tests/cli/release/test_port_commit.py | 350 +++++++++++++++++ 4 files changed, 795 insertions(+), 3 deletions(-) create mode 100644 ddev/src/ddev/cli/release/port_commit.py create mode 100644 ddev/tests/cli/release/test_port_commit.py diff --git a/ddev/src/ddev/cli/release/__init__.py b/ddev/src/ddev/cli/release/__init__.py index 633d36b006734..22d96ab906fd6 100644 --- a/ddev/src/ddev/cli/release/__init__.py +++ b/ddev/src/ddev/cli/release/__init__.py @@ -11,6 +11,7 @@ from ddev.cli.release.branch import branch from ddev.cli.release.changelog import changelog from ddev.cli.release.list_versions import list_versions +from ddev.cli.release.port_commit import port_commit from ddev.cli.release.show import show from ddev.cli.release.stats import stats @@ -28,6 +29,7 @@ def release(): release.add_command(changelog) release.add_command(list_versions) release.add_command(make) +release.add_command(port_commit) release.add_command(show) release.add_command(stats) release.add_command(tag) diff --git a/ddev/src/ddev/cli/release/port_commit.py b/ddev/src/ddev/cli/release/port_commit.py new file mode 100644 index 0000000000000..b6823dc039da2 --- /dev/null +++ b/ddev/src/ddev/cli/release/port_commit.py @@ -0,0 +1,422 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +from __future__ import annotations + +import re +from typing import TYPE_CHECKING + +import click + +if TYPE_CHECKING: + from ddev.cli.application import Application + + +PR_NUMBER_SUFFIX_PATTERN = re.compile(r'\s*\(#(\d+)\)\s*$') +PR_TEMPLATE_RELATIVE_PATH = '.github/PULL_REQUEST_TEMPLATE.md' +PR_TEMPLATE_HEADING = '### What does this PR do?' +IN_TOTO_SUFFIX = '.in-toto' + + +class PortStepError(Exception): + """Raised by a PortStep to signal a clean abort with a user-facing message.""" + + +class PortStep: + """Single step of the port-commit workflow.""" + + def __init__(self, app: Application, *, dry_run: bool = False) -> None: + self.app = app + self.dry_run = dry_run + + def describe(self) -> str: + raise NotImplementedError + + def planned_commands(self) -> list[str]: + return [] + + def execute(self) -> None: + raise NotImplementedError + + def run(self) -> None: + if self.dry_run: + self.app.display_info(self.describe()) + for cmd in self.planned_commands(): + self.app.display_info(f' (dry-run) {cmd}') + return + + with self.app.status(self.describe()): + try: + self.execute() + except PortStepError: + raise + except OSError as e: + raise PortStepError(str(e)) from e + + self.app.display_success(f'{self.describe()}: done.') + + +class FetchOriginStep(PortStep): + def describe(self) -> str: + return 'Fetching latest changes from origin' + + def planned_commands(self) -> list[str]: + return ['git fetch origin'] + + def execute(self) -> None: + self.app.repo.git.run('fetch', 'origin') + + +class CheckoutTargetStep(PortStep): + def __init__(self, app: Application, *, target: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.target = target + + def describe(self) -> str: + return f'Checking out and updating `{self.target}`' + + def planned_commands(self) -> list[str]: + return [f'git checkout {self.target}', f'git pull origin {self.target}'] + + def execute(self) -> None: + self.app.repo.git.run('checkout', self.target) + self.app.repo.git.run('pull', 'origin', self.target) + + +class CreatePortBranchStep(PortStep): + def __init__(self, app: Application, *, branch: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.branch = branch + + def describe(self) -> str: + return f'Creating branch `{self.branch}`' + + def planned_commands(self) -> list[str]: + return [f'git checkout -B {self.branch}'] + + def execute(self) -> None: + self.app.repo.git.run('checkout', '-B', self.branch) + + +class CherryPickStep(PortStep): + """Cherry-pick a commit, auto-resolving `.in-toto`-only conflicts.""" + + def __init__(self, app: Application, *, sha: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.sha = sha + + def describe(self) -> str: + return f'Cherry-picking {self.sha[:10]}' + + def planned_commands(self) -> list[str]: + return [f'git cherry-pick --no-commit {self.sha}'] + + def execute(self) -> None: + try: + self.app.repo.git.run('cherry-pick', '--no-commit', self.sha) + return + except OSError: + pass + + conflicts = [ + line for line in self.app.repo.git.capture('diff', '--name-only', '--diff-filter=U').splitlines() if line + ] + non_in_toto = [f for f in conflicts if IN_TOTO_SUFFIX not in f] + in_toto = [f for f in conflicts if IN_TOTO_SUFFIX in f] + + if non_in_toto: + try: + self.app.repo.git.run('cherry-pick', '--abort') + except OSError: + pass + listing = '\n '.join(non_in_toto) + raise PortStepError(f'Cherry-pick has conflicts in non-`.in-toto` files:\n {listing}') + + if not in_toto: + raise PortStepError('Cherry-pick failed without conflicts. Resolve manually and try again.') + + for path in in_toto: + _resolve_in_toto_conflict(self.app, path) + + +class PreserveInTotoStep(PortStep): + """Reset any staged `.in-toto` changes to keep the target branch's signature metadata.""" + + def describe(self) -> str: + return 'Preserving `.in-toto` files from target branch' + + def planned_commands(self) -> list[str]: + return ['# Reset any staged .in-toto changes to HEAD'] + + def execute(self) -> None: + staged = [line for line in self.app.repo.git.capture('diff', '--cached', '--name-only').splitlines() if line] + affected = [f for f in staged if IN_TOTO_SUFFIX in f] + if not affected: + return + + for path in affected: + _restore_path_from_head(self.app, path) + + +class CommitStep(PortStep): + def __init__(self, app: Application, *, subject: str, verify: bool = False, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.subject = subject + self.verify = verify + + @property + def message(self) -> str: + return f'[Backport] {self.subject}' + + def describe(self) -> str: + return f'Committing changes as "{self.message}"' + + def planned_commands(self) -> list[str]: + flags = '' if self.verify else '--no-verify ' + return [f'git commit {flags}-m "{self.message}"'] + + def execute(self) -> None: + args = ['commit', '-m', self.message] + if not self.verify: + args.insert(1, '--no-verify') + self.app.repo.git.run(*args) + + +class PushStep(PortStep): + def __init__(self, app: Application, *, branch: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.branch = branch + + def describe(self) -> str: + return f'Pushing `{self.branch}` to origin' + + def planned_commands(self) -> list[str]: + return [f'git push origin {self.branch}'] + + def execute(self) -> None: + self.app.repo.git.run('push', 'origin', self.branch) + + +class CreatePullRequestStep(PortStep): + def __init__( + self, + app: Application, + *, + title: str, + head: str, + base: str, + body: str, + labels: list[str], + draft: bool, + dry_run: bool = False, + ) -> None: + super().__init__(app, dry_run=dry_run) + self.title = title + self.head = head + self.base = base + self.body = body + self.labels = labels + self.draft = draft + self.pr_url: str | None = None + + def describe(self) -> str: + flavor = 'draft pull request' if self.draft else 'pull request' + return f'Creating {flavor} `{self.title}`' + + def planned_commands(self) -> list[str]: + label_part = f' --label {",".join(self.labels)}' if self.labels else '' + draft_part = ' --draft' if self.draft else '' + return [f'POST /repos/.../pulls (head={self.head}, base={self.base}{draft_part}){label_part}'] + + def execute(self) -> None: + self.pr_url = self.app.github.create_pull_request( + title=self.title, + head=self.head, + base=self.base, + body=self.body, + draft=self.draft, + labels=self.labels or None, + ) + + +def _resolve_in_toto_conflict(app: Application, path: str) -> None: + try: + app.repo.git.capture('cat-file', '-e', f'HEAD:{path}') + app.repo.git.run('checkout', '--ours', path) + app.repo.git.run('add', path) + except OSError: + app.repo.git.run('rm', '--force', path) + + +def _restore_path_from_head(app: Application, path: str) -> None: + try: + app.repo.git.capture('cat-file', '-e', f'HEAD:{path}') + app.repo.git.run('checkout', 'HEAD', '--', path) + except OSError: + app.repo.git.run('rm', '--force', path) + + +def split_commit_subject(subject: str) -> tuple[str, str | None]: + """Return (subject_without_pr_suffix, original_pr_number_or_None).""" + match = PR_NUMBER_SUFFIX_PATTERN.search(subject) + if not match: + return subject, None + return PR_NUMBER_SUFFIX_PATTERN.sub('', subject), match.group(1) + + +def build_pr_body(app: Application, *, sha: str, subject: str, target: str, original_pr: str | None) -> str: + info_lines = [f'**Backported commit**: `{sha[:10]}` - {subject}'] + if original_pr: + info_lines.append(f'**Original PR**: #{original_pr}') + info_lines.append(f'**Target branch**: `{target}`') + info_block = '\n'.join(info_lines) + '\n' + + template_path = app.repo.path / PR_TEMPLATE_RELATIVE_PATH + if not template_path.is_file(): + return f'{PR_TEMPLATE_HEADING}\n\n{info_block}' + + template = template_path.read_text() + if PR_TEMPLATE_HEADING in template: + return template.replace(PR_TEMPLATE_HEADING, f'{PR_TEMPLATE_HEADING}\n\n{info_block}', 1) + return f'{info_block}\n{template}' + + +def parse_labels(raw: str) -> list[str]: + return [label.strip() for label in raw.split(',') if label.strip()] + + +@click.command(name='port-commit', short_help='Backport a commit onto a target branch') +@click.pass_obj +@click.argument('commit_hash', required=False) +@click.option('-t', '--target-branch', default='master', show_default=True, help='Target branch to port to.') +@click.option('-p', '--branch-prefix', default='port', show_default=True, help='Branch name prefix.') +@click.option('-s', '--branch-suffix', default=None, help='Branch name suffix. Defaults to `to-`.') +@click.option( + '-l', + '--pr-labels', + default='qa/skip-qa', + show_default=True, + help='Comma-separated PR labels.', +) +@click.option('--no-pr', is_flag=True, default=False, help="Don't create a pull request.") +@click.option('--draft', is_flag=True, default=False, help='Open the PR as a draft.') +@click.option('--verify', is_flag=True, default=False, help='Run commit hooks (skipped by default).') +@click.option('--dry-run', is_flag=True, default=False, help='Print every step instead of executing it.') +def port_commit( + app: Application, + commit_hash: str | None, + target_branch: str, + branch_prefix: str, + branch_suffix: str | None, + pr_labels: str, + no_pr: bool, + draft: bool, + verify: bool, + dry_run: bool, +) -> None: + """ + Backport a commit onto a target branch. + + Cherry-picks COMMIT_HASH onto `--target-branch` (default `master`) on a new branch named + `/--`, preserving `.in-toto` files from the target + branch so package signatures stay intact. Pushes the branch and, unless `--no-pr` is set, + opens a pull request titled `[Backport] ` and labeled with `--pr-labels`. + + If COMMIT_HASH is omitted, the current HEAD commit is used after confirmation. + + The GitHub user for the branch prefix is taken from `ddev config` (`github.user`) or the + `DD_GITHUB_USER` / `GITHUB_USER` / `GITHUB_ACTOR` environment variables. + """ + user = app.config.github.user + if not user: + app.abort( + 'No GitHub user configured. Set `github.user` via `ddev config set github.user ` ' + 'or export DD_GITHUB_USER.' + ) + + if commit_hash is None: + head_commit = app.repo.git.latest_commit() + app.display_info(f'No commit specified. Current HEAD: `{head_commit.sha[:10]}` - {head_commit.subject}') + if not dry_run and not click.confirm('Use this commit?'): + app.abort('Did not get confirmation, aborting.') + commit_hash = head_commit.sha + + try: + full_sha = app.repo.git.capture('rev-parse', '--verify', f'{commit_hash}^{{commit}}').strip() + except OSError: + app.abort(f'Commit `{commit_hash}` does not exist.') + + log_entries = app.repo.git.log(['hash:%H', 'subject:%s'], n=1, source=full_sha) + if not log_entries: + app.abort(f'Could not read commit `{full_sha}`.') + raw_subject = log_entries[0]['subject'] + clean_subject, original_pr = split_commit_subject(raw_subject) + + in_toto_files = [ + line + for line in app.repo.git.capture('diff-tree', '--no-commit-id', '--name-only', '-r', full_sha).splitlines() + if IN_TOTO_SUFFIX in line + ] + if in_toto_files: + listing = '\n '.join(in_toto_files) + app.display_warning( + f'Commit touches {len(in_toto_files)} `.in-toto` file(s); they will be preserved ' + f'from `{target_branch}`:\n {listing}' + ) + + suffix = branch_suffix or f'to-{target_branch}' + new_branch = f'{user}/{branch_prefix}-{full_sha[:10]}-{suffix}' + labels = parse_labels(pr_labels) + + app.display_info( + '\n'.join( + [ + 'Configuration:', + f' Target branch: {target_branch}', + f' Commit: {full_sha[:10]} - {clean_subject}', + f' Original PR: #{original_pr}' if original_pr else ' Original PR: (none)', + f' New branch: {new_branch}', + f' Create PR: {not no_pr} (draft={draft})', + f' PR labels: {", ".join(labels) if labels else "(none)"}', + f' Verify commit: {verify}', + f' Dry run: {dry_run}', + ] + ) + ) + + if not dry_run and not click.confirm('Continue?'): + app.abort('Did not get confirmation, aborting.') + + steps: list[PortStep] = [ + FetchOriginStep(app, dry_run=dry_run), + CheckoutTargetStep(app, target=target_branch, dry_run=dry_run), + CreatePortBranchStep(app, branch=new_branch, dry_run=dry_run), + CherryPickStep(app, sha=full_sha, dry_run=dry_run), + PreserveInTotoStep(app, dry_run=dry_run), + CommitStep(app, subject=clean_subject, verify=verify, dry_run=dry_run), + PushStep(app, branch=new_branch, dry_run=dry_run), + ] + + pr_step: CreatePullRequestStep | None = None + if not no_pr: + pr_step = CreatePullRequestStep( + app, + title=f'[Backport] {clean_subject}', + head=new_branch, + base=target_branch, + body=build_pr_body(app, sha=full_sha, subject=clean_subject, target=target_branch, original_pr=original_pr), + labels=labels, + draft=draft, + dry_run=dry_run, + ) + steps.append(pr_step) + + try: + for step in steps: + step.run() + except PortStepError as e: + app.abort(str(e)) + + if pr_step is not None and pr_step.pr_url: + app.display_success(f'Pull request created: {pr_step.pr_url}') + app.display_success('All done.') diff --git a/ddev/src/ddev/utils/github.py b/ddev/src/ddev/utils/github.py index e258cabc0bb4e..de2e462946d59 100644 --- a/ddev/src/ddev/utils/github.py +++ b/ddev/src/ddev/utils/github.py @@ -75,6 +75,9 @@ class GitHubManager: # https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#create-a-pull-request PULLS_API = 'https://api.github.com/repos/{repo_id}/pulls' + # https://docs.github.com/en/rest/issues/labels?apiVersion=2022-11-28#add-labels-to-an-issue + ISSUES_LABELS_API = 'https://api.github.com/repos/{repo_id}/issues/{issue_number}/labels' + # https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files PULL_REQUEST_FILES_API = 'https://api.github.com/repos/{repo_id}/pulls/{pr_number}/files' @@ -240,12 +243,27 @@ def create_label(self, name, color): def create_milestone(self, title: str) -> None: self.__api_post(self.MILESTONES_API.format(repo_id=self.repo_id), content=json.dumps({'title': title})) - def create_pull_request(self, title: str, head: str, base: str, body: str = '') -> str: + def create_pull_request( + self, + title: str, + head: str, + base: str, + body: str = '', + *, + draft: bool = False, + labels: list[str] | None = None, + ) -> str: response = self.__api_post( self.PULLS_API.format(repo_id=self.repo_id), - content=json.dumps({'title': title, 'head': head, 'base': base, 'body': body}), + content=json.dumps({'title': title, 'head': head, 'base': base, 'body': body, 'draft': draft}), ) - return response.json()['html_url'] + pr_data = response.json() + if labels: + self.__api_post( + self.ISSUES_LABELS_API.format(repo_id=self.repo_id, issue_number=pr_data['number']), + content=json.dumps({'labels': labels}), + ) + return pr_data['html_url'] def get_label(self, name): return self.__api_get(f'{self.LABELS_API.format(repo_id=self.repo_id)}/{name}') diff --git a/ddev/tests/cli/release/test_port_commit.py b/ddev/tests/cli/release/test_port_commit.py new file mode 100644 index 0000000000000..ab62b464e9552 --- /dev/null +++ b/ddev/tests/cli/release/test_port_commit.py @@ -0,0 +1,350 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +from __future__ import annotations + +from unittest.mock import MagicMock, call + +import pytest + +from ddev.cli.release.port_commit import ( + CherryPickStep, + CommitStep, + CreatePullRequestStep, + PortStep, + PortStepError, + PreserveInTotoStep, + build_pr_body, + parse_labels, + split_commit_subject, +) +from ddev.utils.git import GitCommit + + +@pytest.fixture +def app_mock(mocker): + app = mocker.MagicMock() + app.status.return_value.__enter__ = MagicMock(return_value=None) + app.status.return_value.__exit__ = MagicMock(return_value=None) + return app + + +@pytest.mark.parametrize( + 'subject,expected_clean,expected_pr', + [ + ('Fix flake (#12345)', 'Fix flake', '12345'), + ('Fix flake', 'Fix flake', None), + ('Multi part subject (#1)', 'Multi part subject', '1'), + ('Trailing spaces (#999) ', 'Trailing spaces', '999'), + ], +) +def test_split_commit_subject(subject, expected_clean, expected_pr): + assert split_commit_subject(subject) == (expected_clean, expected_pr) + + +@pytest.mark.parametrize( + 'raw,expected', + [ + ('qa/skip-qa', ['qa/skip-qa']), + ('qa/skip-qa, backport/7.62.x', ['qa/skip-qa', 'backport/7.62.x']), + ('', []), + (' , , ', []), + ], +) +def test_parse_labels(raw, expected): + assert parse_labels(raw) == expected + + +def test_build_pr_body_uses_template(app_mock, tmp_path): + template_dir = tmp_path / '.github' + template_dir.mkdir() + template_file = template_dir / 'PULL_REQUEST_TEMPLATE.md' + template_file.write_text('### What does this PR do?\n\n\n\n### Motivation\n') + app_mock.repo.path = tmp_path + + body = build_pr_body(app_mock, sha='abcdef1234567890', subject='Fix bug', target='7.62.x', original_pr='12345') + + assert '**Backported commit**: `abcdef1234`' in body + assert '**Original PR**: #12345' in body + assert '**Target branch**: `7.62.x`' in body + assert '### Motivation' in body + + +def test_build_pr_body_without_template(app_mock, tmp_path): + app_mock.repo.path = tmp_path + + body = build_pr_body(app_mock, sha='abcdef1234567890', subject='Fix bug', target='master', original_pr=None) + + assert '### What does this PR do?' in body + assert '**Backported commit**: `abcdef1234`' in body + assert 'Original PR' not in body + + +class _RunnableStep(PortStep): + def __init__(self, app, *, fail=False, dry_run=False): + super().__init__(app, dry_run=dry_run) + self.fail = fail + self.executed = False + + def describe(self): + return 'Doing the thing' + + def planned_commands(self): + return ['echo hi'] + + def execute(self): + self.executed = True + if self.fail: + raise OSError('boom') + + +def test_port_step_dry_run_skips_execute(app_mock): + step = _RunnableStep(app_mock, dry_run=True) + step.run() + assert step.executed is False + app_mock.status.assert_not_called() + info_calls = [c.args[0] for c in app_mock.display_info.call_args_list] + assert 'Doing the thing' in info_calls + assert any('echo hi' in line for line in info_calls) + + +def test_port_step_executes_and_emits_success(app_mock): + step = _RunnableStep(app_mock) + step.run() + assert step.executed is True + app_mock.status.assert_called_once_with('Doing the thing') + app_mock.display_success.assert_called_once() + + +def test_port_step_wraps_oserror_as_port_step_error(app_mock): + step = _RunnableStep(app_mock, fail=True) + with pytest.raises(PortStepError, match='boom'): + step.run() + + +def test_cherry_pick_clean(app_mock): + step = CherryPickStep(app_mock, sha='deadbeef00') + step.execute() + app_mock.repo.git.run.assert_called_once_with('cherry-pick', '--no-commit', 'deadbeef00') + app_mock.repo.git.capture.assert_not_called() + + +def test_cherry_pick_only_in_toto_conflict_is_resolved(app_mock): + app_mock.repo.git.run.side_effect = [OSError('conflict'), None, None] + app_mock.repo.git.capture.side_effect = [ + 'path/file.in-toto.link\n', + '', + ] + + step = CherryPickStep(app_mock, sha='deadbeef00') + step.execute() + + assert app_mock.repo.git.run.call_args_list == [ + call('cherry-pick', '--no-commit', 'deadbeef00'), + call('checkout', '--ours', 'path/file.in-toto.link'), + call('add', 'path/file.in-toto.link'), + ] + + +def test_cherry_pick_mixed_conflict_aborts(app_mock): + app_mock.repo.git.run.side_effect = [OSError('conflict'), None] + app_mock.repo.git.capture.return_value = 'src/foo.py\npath/file.in-toto.link\n' + + step = CherryPickStep(app_mock, sha='deadbeef00') + with pytest.raises(PortStepError, match='non-`.in-toto`'): + step.execute() + + app_mock.repo.git.run.assert_any_call('cherry-pick', '--abort') + + +def test_cherry_pick_failure_without_conflicts_aborts(app_mock): + app_mock.repo.git.run.side_effect = OSError('conflict') + app_mock.repo.git.capture.return_value = '' + + step = CherryPickStep(app_mock, sha='deadbeef00') + with pytest.raises(PortStepError, match='without conflicts'): + step.execute() + + +def test_preserve_in_toto_resets_staged_modifications(app_mock): + app_mock.repo.git.capture.side_effect = [ + 'src/foo.py\npath/file.in-toto.link\n', + '', + ] + + step = PreserveInTotoStep(app_mock) + step.execute() + + app_mock.repo.git.run.assert_called_once_with('checkout', 'HEAD', '--', 'path/file.in-toto.link') + + +def test_preserve_in_toto_removes_files_not_in_head(app_mock): + app_mock.repo.git.capture.side_effect = [ + 'path/new.in-toto.link\n', + OSError('not in head'), + ] + + step = PreserveInTotoStep(app_mock) + step.execute() + + app_mock.repo.git.run.assert_called_once_with('rm', '--force', 'path/new.in-toto.link') + + +def test_preserve_in_toto_noop_when_clean(app_mock): + app_mock.repo.git.capture.return_value = 'src/foo.py\n' + + step = PreserveInTotoStep(app_mock) + step.execute() + + app_mock.repo.git.run.assert_not_called() + + +@pytest.mark.parametrize( + 'verify,expected_args', + [ + (False, ('commit', '--no-verify', '-m', '[Backport] Fix bug')), + (True, ('commit', '-m', '[Backport] Fix bug')), + ], +) +def test_commit_step(app_mock, verify, expected_args): + step = CommitStep(app_mock, subject='Fix bug', verify=verify) + step.execute() + app_mock.repo.git.run.assert_called_once_with(*expected_args) + + +def test_create_pull_request_step(app_mock): + app_mock.github.create_pull_request.return_value = 'https://github.com/x/pr/1' + step = CreatePullRequestStep( + app_mock, + title='[Backport] Fix bug', + head='alice/port-deadbeef00-to-7.62.x', + base='7.62.x', + body='body', + labels=['qa/skip-qa'], + draft=False, + ) + step.execute() + app_mock.github.create_pull_request.assert_called_once_with( + title='[Backport] Fix bug', + head='alice/port-deadbeef00-to-7.62.x', + base='7.62.x', + body='body', + draft=False, + labels=['qa/skip-qa'], + ) + assert step.pr_url == 'https://github.com/x/pr/1' + + +def _setup_command_mocks(mocker, *, commit_sha='deadbeef0011223344', subject='Fix bug (#100)', in_toto=''): + mocker.patch('ddev.utils.git.GitRepository.run') + capture_mock = mocker.patch('ddev.utils.git.GitRepository.capture') + capture_mock.side_effect = lambda *args: { + ('rev-parse', '--verify', f'{commit_sha}^{{commit}}'): commit_sha + '\n', + ('diff-tree', '--no-commit-id', '--name-only', '-r', commit_sha): in_toto, + }.get(args, '') + mocker.patch( + 'ddev.utils.git.GitRepository.log', + return_value=[{'hash': commit_sha, 'subject': subject}], + ) + mocker.patch( + 'ddev.utils.git.GitRepository.latest_commit', + return_value=GitCommit(commit_sha, subject=subject), + ) + + +def test_command_happy_path(ddev, mocker): + _setup_command_mocks(mocker) + pr_mock = mocker.patch( + 'ddev.utils.github.GitHubManager.create_pull_request', return_value='https://github.com/x/pr/1' + ) + mocker.patch('click.confirm', return_value=True) + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) + + result = ddev('release', 'port-commit', 'deadbeef0011223344') + + assert result.exit_code == 0, result.output + assert 'Pull request created: https://github.com/x/pr/1' in result.output + pr_mock.assert_called_once() + _, kwargs = pr_mock.call_args + assert kwargs['draft'] is False + assert kwargs['labels'] == ['qa/skip-qa'] + assert kwargs['head'] == 'alice/port-deadbeef00-to-master' + assert kwargs['base'] == 'master' + assert kwargs['title'] == '[Backport] Fix bug' + + +def test_command_draft_flag(ddev, mocker): + _setup_command_mocks(mocker) + pr_mock = mocker.patch( + 'ddev.utils.github.GitHubManager.create_pull_request', return_value='https://github.com/x/pr/2' + ) + mocker.patch('click.confirm', return_value=True) + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) + + result = ddev('release', 'port-commit', '--draft', 'deadbeef0011223344') + + assert result.exit_code == 0, result.output + _, kwargs = pr_mock.call_args + assert kwargs['draft'] is True + + +def test_command_no_pr(ddev, mocker): + _setup_command_mocks(mocker) + pr_mock = mocker.patch('ddev.utils.github.GitHubManager.create_pull_request') + mocker.patch('click.confirm', return_value=True) + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) + + result = ddev('release', 'port-commit', '--no-pr', 'deadbeef0011223344') + + assert result.exit_code == 0, result.output + pr_mock.assert_not_called() + + +def test_command_dry_run_makes_no_mutating_calls(ddev, mocker): + _setup_command_mocks(mocker) + run_mock = mocker.patch('ddev.utils.git.GitRepository.run') + pr_mock = mocker.patch('ddev.utils.github.GitHubManager.create_pull_request') + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) + + result = ddev('release', 'port-commit', '--dry-run', 'deadbeef0011223344') + + assert result.exit_code == 0, result.output + assert '(dry-run)' in result.output + run_mock.assert_not_called() + pr_mock.assert_not_called() + + +def test_command_aborts_when_no_github_user(ddev, mocker): + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': '', 'GITHUB_USER': '', 'GITHUB_ACTOR': ''}) + + result = ddev('release', 'port-commit', 'deadbeef0011223344') + + assert result.exit_code == 1, result.output + assert 'No GitHub user configured' in result.output + + +def test_command_aborts_on_unconfirmed(ddev, mocker): + _setup_command_mocks(mocker) + mocker.patch('ddev.utils.github.GitHubManager.create_pull_request') + mocker.patch('click.confirm', return_value=False) + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) + + result = ddev('release', 'port-commit', 'deadbeef0011223344') + + assert result.exit_code == 1, result.output + assert 'Did not get confirmation' in result.output + + +def test_command_uses_head_when_no_commit_given(ddev, mocker): + _setup_command_mocks(mocker) + pr_mock = mocker.patch( + 'ddev.utils.github.GitHubManager.create_pull_request', return_value='https://github.com/x/pr/3' + ) + mocker.patch('click.confirm', return_value=True) + mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) + + result = ddev('release', 'port-commit') + + assert result.exit_code == 0, result.output + assert 'No commit specified' in result.output + pr_mock.assert_called_once() From 81d6745735f83953859a5540d2c8ca84541b19d1 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Tue, 12 May 2026 16:52:58 +0200 Subject: [PATCH 02/11] Add changelog entry for port-commit --- ddev/changelog.d/23680.added | 1 + 1 file changed, 1 insertion(+) create mode 100644 ddev/changelog.d/23680.added diff --git a/ddev/changelog.d/23680.added b/ddev/changelog.d/23680.added new file mode 100644 index 0000000000000..b871869876698 --- /dev/null +++ b/ddev/changelog.d/23680.added @@ -0,0 +1 @@ +Add `ddev release port-commit` command to backport a commit to a target branch. From 6430b92b232ff182f65bf2ff57123c17babdd4c1 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Tue, 12 May 2026 17:08:14 +0200 Subject: [PATCH 03/11] Extract port-commit input validation into resolve_port_plan --- ddev/src/ddev/cli/release/port_commit.py | 201 +++++++++++++++-------- 1 file changed, 134 insertions(+), 67 deletions(-) diff --git a/ddev/src/ddev/cli/release/port_commit.py b/ddev/src/ddev/cli/release/port_commit.py index b6823dc039da2..edf5da4231da0 100644 --- a/ddev/src/ddev/cli/release/port_commit.py +++ b/ddev/src/ddev/cli/release/port_commit.py @@ -4,6 +4,7 @@ from __future__ import annotations import re +from dataclasses import dataclass from typing import TYPE_CHECKING import click @@ -285,25 +286,27 @@ def parse_labels(raw: str) -> list[str]: return [label.strip() for label in raw.split(',') if label.strip()] -@click.command(name='port-commit', short_help='Backport a commit onto a target branch') -@click.pass_obj -@click.argument('commit_hash', required=False) -@click.option('-t', '--target-branch', default='master', show_default=True, help='Target branch to port to.') -@click.option('-p', '--branch-prefix', default='port', show_default=True, help='Branch name prefix.') -@click.option('-s', '--branch-suffix', default=None, help='Branch name suffix. Defaults to `to-`.') -@click.option( - '-l', - '--pr-labels', - default='qa/skip-qa', - show_default=True, - help='Comma-separated PR labels.', -) -@click.option('--no-pr', is_flag=True, default=False, help="Don't create a pull request.") -@click.option('--draft', is_flag=True, default=False, help='Open the PR as a draft.') -@click.option('--verify', is_flag=True, default=False, help='Run commit hooks (skipped by default).') -@click.option('--dry-run', is_flag=True, default=False, help='Print every step instead of executing it.') -def port_commit( +@dataclass(frozen=True) +class PortPlan: + """All values needed to execute the port workflow, resolved up front.""" + + full_sha: str + clean_subject: str + original_pr: str | None + target_branch: str + new_branch: str + pr_title: str + pr_body: str + labels: list[str] + draft: bool + create_pr: bool + verify: bool + dry_run: bool + + +def resolve_port_plan( app: Application, + *, commit_hash: str | None, target_branch: str, branch_prefix: str, @@ -313,20 +316,8 @@ def port_commit( draft: bool, verify: bool, dry_run: bool, -) -> None: - """ - Backport a commit onto a target branch. - - Cherry-picks COMMIT_HASH onto `--target-branch` (default `master`) on a new branch named - `/--`, preserving `.in-toto` files from the target - branch so package signatures stay intact. Pushes the branch and, unless `--no-pr` is set, - opens a pull request titled `[Backport] ` and labeled with `--pr-labels`. - - If COMMIT_HASH is omitted, the current HEAD commit is used after confirmation. - - The GitHub user for the branch prefix is taken from `ddev config` (`github.user`) or the - `DD_GITHUB_USER` / `GITHUB_USER` / `GITHUB_ACTOR` environment variables. - """ +) -> PortPlan: + """Validate inputs, resolve derived values, and confirm with the user. Aborts on failure.""" user = app.config.github.user if not user: app.abort( @@ -349,8 +340,7 @@ def port_commit( log_entries = app.repo.git.log(['hash:%H', 'subject:%s'], n=1, source=full_sha) if not log_entries: app.abort(f'Could not read commit `{full_sha}`.') - raw_subject = log_entries[0]['subject'] - clean_subject, original_pr = split_commit_subject(raw_subject) + clean_subject, original_pr = split_commit_subject(log_entries[0]['subject']) in_toto_files = [ line @@ -365,51 +355,128 @@ def port_commit( ) suffix = branch_suffix or f'to-{target_branch}' - new_branch = f'{user}/{branch_prefix}-{full_sha[:10]}-{suffix}' - labels = parse_labels(pr_labels) - - app.display_info( - '\n'.join( - [ - 'Configuration:', - f' Target branch: {target_branch}', - f' Commit: {full_sha[:10]} - {clean_subject}', - f' Original PR: #{original_pr}' if original_pr else ' Original PR: (none)', - f' New branch: {new_branch}', - f' Create PR: {not no_pr} (draft={draft})', - f' PR labels: {", ".join(labels) if labels else "(none)"}', - f' Verify commit: {verify}', - f' Dry run: {dry_run}', - ] - ) + plan = PortPlan( + full_sha=full_sha, + clean_subject=clean_subject, + original_pr=original_pr, + target_branch=target_branch, + new_branch=f'{user}/{branch_prefix}-{full_sha[:10]}-{suffix}', + pr_title=f'[Backport] {clean_subject}', + pr_body=build_pr_body(app, sha=full_sha, subject=clean_subject, target=target_branch, original_pr=original_pr), + labels=parse_labels(pr_labels), + draft=draft, + create_pr=not no_pr, + verify=verify, + dry_run=dry_run, ) + app.display_info(_format_plan_summary(plan)) + if not dry_run and not click.confirm('Continue?'): app.abort('Did not get confirmation, aborting.') + return plan + + +def _format_plan_summary(plan: PortPlan) -> str: + original_pr_line = f' Original PR: #{plan.original_pr}' if plan.original_pr else ' Original PR: (none)' + return '\n'.join( + [ + 'Configuration:', + f' Target branch: {plan.target_branch}', + f' Commit: {plan.full_sha[:10]} - {plan.clean_subject}', + original_pr_line, + f' New branch: {plan.new_branch}', + f' Create PR: {plan.create_pr} (draft={plan.draft})', + f' PR labels: {", ".join(plan.labels) if plan.labels else "(none)"}', + f' Verify commit: {plan.verify}', + f' Dry run: {plan.dry_run}', + ] + ) + + +def build_port_steps(app: Application, plan: PortPlan) -> tuple[list[PortStep], CreatePullRequestStep | None]: + """Build the ordered list of steps for the workflow, plus the PR step reference (or None).""" steps: list[PortStep] = [ - FetchOriginStep(app, dry_run=dry_run), - CheckoutTargetStep(app, target=target_branch, dry_run=dry_run), - CreatePortBranchStep(app, branch=new_branch, dry_run=dry_run), - CherryPickStep(app, sha=full_sha, dry_run=dry_run), - PreserveInTotoStep(app, dry_run=dry_run), - CommitStep(app, subject=clean_subject, verify=verify, dry_run=dry_run), - PushStep(app, branch=new_branch, dry_run=dry_run), + FetchOriginStep(app, dry_run=plan.dry_run), + CheckoutTargetStep(app, target=plan.target_branch, dry_run=plan.dry_run), + CreatePortBranchStep(app, branch=plan.new_branch, dry_run=plan.dry_run), + CherryPickStep(app, sha=plan.full_sha, dry_run=plan.dry_run), + PreserveInTotoStep(app, dry_run=plan.dry_run), + CommitStep(app, subject=plan.clean_subject, verify=plan.verify, dry_run=plan.dry_run), + PushStep(app, branch=plan.new_branch, dry_run=plan.dry_run), ] - pr_step: CreatePullRequestStep | None = None - if not no_pr: + if plan.create_pr: pr_step = CreatePullRequestStep( app, - title=f'[Backport] {clean_subject}', - head=new_branch, - base=target_branch, - body=build_pr_body(app, sha=full_sha, subject=clean_subject, target=target_branch, original_pr=original_pr), - labels=labels, - draft=draft, - dry_run=dry_run, + title=plan.pr_title, + head=plan.new_branch, + base=plan.target_branch, + body=plan.pr_body, + labels=plan.labels, + draft=plan.draft, + dry_run=plan.dry_run, ) steps.append(pr_step) + return steps, pr_step + + +@click.command(name='port-commit', short_help='Backport a commit onto a target branch') +@click.pass_obj +@click.argument('commit_hash', required=False) +@click.option('-t', '--target-branch', default='master', show_default=True, help='Target branch to port to.') +@click.option('-p', '--branch-prefix', default='port', show_default=True, help='Branch name prefix.') +@click.option('-s', '--branch-suffix', default=None, help='Branch name suffix. Defaults to `to-`.') +@click.option( + '-l', + '--pr-labels', + default='qa/skip-qa', + show_default=True, + help='Comma-separated PR labels.', +) +@click.option('--no-pr', is_flag=True, default=False, help="Don't create a pull request.") +@click.option('--draft', is_flag=True, default=False, help='Open the PR as a draft.') +@click.option('--verify', is_flag=True, default=False, help='Run commit hooks (skipped by default).') +@click.option('--dry-run', is_flag=True, default=False, help='Print every step instead of executing it.') +def port_commit( + app: Application, + commit_hash: str | None, + target_branch: str, + branch_prefix: str, + branch_suffix: str | None, + pr_labels: str, + no_pr: bool, + draft: bool, + verify: bool, + dry_run: bool, +) -> None: + """ + Backport a commit onto a target branch. + + Cherry-picks COMMIT_HASH onto `--target-branch` (default `master`) on a new branch named + `/--`, preserving `.in-toto` files from the target + branch so package signatures stay intact. Pushes the branch and, unless `--no-pr` is set, + opens a pull request titled `[Backport] ` and labeled with `--pr-labels`. + + If COMMIT_HASH is omitted, the current HEAD commit is used after confirmation. + + The GitHub user for the branch prefix is taken from `ddev config` (`github.user`) or the + `DD_GITHUB_USER` / `GITHUB_USER` / `GITHUB_ACTOR` environment variables. + """ + plan = resolve_port_plan( + app, + commit_hash=commit_hash, + target_branch=target_branch, + branch_prefix=branch_prefix, + branch_suffix=branch_suffix, + pr_labels=pr_labels, + no_pr=no_pr, + draft=draft, + verify=verify, + dry_run=dry_run, + ) + steps, pr_step = build_port_steps(app, plan) try: for step in steps: From b493cd2a8816c7ca5ffd5ad5cc9dbf7a0327d2fa Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Tue, 12 May 2026 17:13:59 +0200 Subject: [PATCH 04/11] Extract port-commit workflow into a lazy-loaded module --- ddev/src/ddev/cli/release/port_commit.py | 413 +---------------- .../ddev/cli/release/port_commit_workflow.py | 429 ++++++++++++++++++ ddev/tests/cli/release/test_port_commit.py | 2 +- 3 files changed, 432 insertions(+), 412 deletions(-) create mode 100644 ddev/src/ddev/cli/release/port_commit_workflow.py diff --git a/ddev/src/ddev/cli/release/port_commit.py b/ddev/src/ddev/cli/release/port_commit.py index edf5da4231da0..c58e577433881 100644 --- a/ddev/src/ddev/cli/release/port_commit.py +++ b/ddev/src/ddev/cli/release/port_commit.py @@ -3,8 +3,6 @@ # Licensed under a 3-clause BSD style license (see LICENSE) from __future__ import annotations -import re -from dataclasses import dataclass from typing import TYPE_CHECKING import click @@ -13,415 +11,6 @@ from ddev.cli.application import Application -PR_NUMBER_SUFFIX_PATTERN = re.compile(r'\s*\(#(\d+)\)\s*$') -PR_TEMPLATE_RELATIVE_PATH = '.github/PULL_REQUEST_TEMPLATE.md' -PR_TEMPLATE_HEADING = '### What does this PR do?' -IN_TOTO_SUFFIX = '.in-toto' - - -class PortStepError(Exception): - """Raised by a PortStep to signal a clean abort with a user-facing message.""" - - -class PortStep: - """Single step of the port-commit workflow.""" - - def __init__(self, app: Application, *, dry_run: bool = False) -> None: - self.app = app - self.dry_run = dry_run - - def describe(self) -> str: - raise NotImplementedError - - def planned_commands(self) -> list[str]: - return [] - - def execute(self) -> None: - raise NotImplementedError - - def run(self) -> None: - if self.dry_run: - self.app.display_info(self.describe()) - for cmd in self.planned_commands(): - self.app.display_info(f' (dry-run) {cmd}') - return - - with self.app.status(self.describe()): - try: - self.execute() - except PortStepError: - raise - except OSError as e: - raise PortStepError(str(e)) from e - - self.app.display_success(f'{self.describe()}: done.') - - -class FetchOriginStep(PortStep): - def describe(self) -> str: - return 'Fetching latest changes from origin' - - def planned_commands(self) -> list[str]: - return ['git fetch origin'] - - def execute(self) -> None: - self.app.repo.git.run('fetch', 'origin') - - -class CheckoutTargetStep(PortStep): - def __init__(self, app: Application, *, target: str, dry_run: bool = False) -> None: - super().__init__(app, dry_run=dry_run) - self.target = target - - def describe(self) -> str: - return f'Checking out and updating `{self.target}`' - - def planned_commands(self) -> list[str]: - return [f'git checkout {self.target}', f'git pull origin {self.target}'] - - def execute(self) -> None: - self.app.repo.git.run('checkout', self.target) - self.app.repo.git.run('pull', 'origin', self.target) - - -class CreatePortBranchStep(PortStep): - def __init__(self, app: Application, *, branch: str, dry_run: bool = False) -> None: - super().__init__(app, dry_run=dry_run) - self.branch = branch - - def describe(self) -> str: - return f'Creating branch `{self.branch}`' - - def planned_commands(self) -> list[str]: - return [f'git checkout -B {self.branch}'] - - def execute(self) -> None: - self.app.repo.git.run('checkout', '-B', self.branch) - - -class CherryPickStep(PortStep): - """Cherry-pick a commit, auto-resolving `.in-toto`-only conflicts.""" - - def __init__(self, app: Application, *, sha: str, dry_run: bool = False) -> None: - super().__init__(app, dry_run=dry_run) - self.sha = sha - - def describe(self) -> str: - return f'Cherry-picking {self.sha[:10]}' - - def planned_commands(self) -> list[str]: - return [f'git cherry-pick --no-commit {self.sha}'] - - def execute(self) -> None: - try: - self.app.repo.git.run('cherry-pick', '--no-commit', self.sha) - return - except OSError: - pass - - conflicts = [ - line for line in self.app.repo.git.capture('diff', '--name-only', '--diff-filter=U').splitlines() if line - ] - non_in_toto = [f for f in conflicts if IN_TOTO_SUFFIX not in f] - in_toto = [f for f in conflicts if IN_TOTO_SUFFIX in f] - - if non_in_toto: - try: - self.app.repo.git.run('cherry-pick', '--abort') - except OSError: - pass - listing = '\n '.join(non_in_toto) - raise PortStepError(f'Cherry-pick has conflicts in non-`.in-toto` files:\n {listing}') - - if not in_toto: - raise PortStepError('Cherry-pick failed without conflicts. Resolve manually and try again.') - - for path in in_toto: - _resolve_in_toto_conflict(self.app, path) - - -class PreserveInTotoStep(PortStep): - """Reset any staged `.in-toto` changes to keep the target branch's signature metadata.""" - - def describe(self) -> str: - return 'Preserving `.in-toto` files from target branch' - - def planned_commands(self) -> list[str]: - return ['# Reset any staged .in-toto changes to HEAD'] - - def execute(self) -> None: - staged = [line for line in self.app.repo.git.capture('diff', '--cached', '--name-only').splitlines() if line] - affected = [f for f in staged if IN_TOTO_SUFFIX in f] - if not affected: - return - - for path in affected: - _restore_path_from_head(self.app, path) - - -class CommitStep(PortStep): - def __init__(self, app: Application, *, subject: str, verify: bool = False, dry_run: bool = False) -> None: - super().__init__(app, dry_run=dry_run) - self.subject = subject - self.verify = verify - - @property - def message(self) -> str: - return f'[Backport] {self.subject}' - - def describe(self) -> str: - return f'Committing changes as "{self.message}"' - - def planned_commands(self) -> list[str]: - flags = '' if self.verify else '--no-verify ' - return [f'git commit {flags}-m "{self.message}"'] - - def execute(self) -> None: - args = ['commit', '-m', self.message] - if not self.verify: - args.insert(1, '--no-verify') - self.app.repo.git.run(*args) - - -class PushStep(PortStep): - def __init__(self, app: Application, *, branch: str, dry_run: bool = False) -> None: - super().__init__(app, dry_run=dry_run) - self.branch = branch - - def describe(self) -> str: - return f'Pushing `{self.branch}` to origin' - - def planned_commands(self) -> list[str]: - return [f'git push origin {self.branch}'] - - def execute(self) -> None: - self.app.repo.git.run('push', 'origin', self.branch) - - -class CreatePullRequestStep(PortStep): - def __init__( - self, - app: Application, - *, - title: str, - head: str, - base: str, - body: str, - labels: list[str], - draft: bool, - dry_run: bool = False, - ) -> None: - super().__init__(app, dry_run=dry_run) - self.title = title - self.head = head - self.base = base - self.body = body - self.labels = labels - self.draft = draft - self.pr_url: str | None = None - - def describe(self) -> str: - flavor = 'draft pull request' if self.draft else 'pull request' - return f'Creating {flavor} `{self.title}`' - - def planned_commands(self) -> list[str]: - label_part = f' --label {",".join(self.labels)}' if self.labels else '' - draft_part = ' --draft' if self.draft else '' - return [f'POST /repos/.../pulls (head={self.head}, base={self.base}{draft_part}){label_part}'] - - def execute(self) -> None: - self.pr_url = self.app.github.create_pull_request( - title=self.title, - head=self.head, - base=self.base, - body=self.body, - draft=self.draft, - labels=self.labels or None, - ) - - -def _resolve_in_toto_conflict(app: Application, path: str) -> None: - try: - app.repo.git.capture('cat-file', '-e', f'HEAD:{path}') - app.repo.git.run('checkout', '--ours', path) - app.repo.git.run('add', path) - except OSError: - app.repo.git.run('rm', '--force', path) - - -def _restore_path_from_head(app: Application, path: str) -> None: - try: - app.repo.git.capture('cat-file', '-e', f'HEAD:{path}') - app.repo.git.run('checkout', 'HEAD', '--', path) - except OSError: - app.repo.git.run('rm', '--force', path) - - -def split_commit_subject(subject: str) -> tuple[str, str | None]: - """Return (subject_without_pr_suffix, original_pr_number_or_None).""" - match = PR_NUMBER_SUFFIX_PATTERN.search(subject) - if not match: - return subject, None - return PR_NUMBER_SUFFIX_PATTERN.sub('', subject), match.group(1) - - -def build_pr_body(app: Application, *, sha: str, subject: str, target: str, original_pr: str | None) -> str: - info_lines = [f'**Backported commit**: `{sha[:10]}` - {subject}'] - if original_pr: - info_lines.append(f'**Original PR**: #{original_pr}') - info_lines.append(f'**Target branch**: `{target}`') - info_block = '\n'.join(info_lines) + '\n' - - template_path = app.repo.path / PR_TEMPLATE_RELATIVE_PATH - if not template_path.is_file(): - return f'{PR_TEMPLATE_HEADING}\n\n{info_block}' - - template = template_path.read_text() - if PR_TEMPLATE_HEADING in template: - return template.replace(PR_TEMPLATE_HEADING, f'{PR_TEMPLATE_HEADING}\n\n{info_block}', 1) - return f'{info_block}\n{template}' - - -def parse_labels(raw: str) -> list[str]: - return [label.strip() for label in raw.split(',') if label.strip()] - - -@dataclass(frozen=True) -class PortPlan: - """All values needed to execute the port workflow, resolved up front.""" - - full_sha: str - clean_subject: str - original_pr: str | None - target_branch: str - new_branch: str - pr_title: str - pr_body: str - labels: list[str] - draft: bool - create_pr: bool - verify: bool - dry_run: bool - - -def resolve_port_plan( - app: Application, - *, - commit_hash: str | None, - target_branch: str, - branch_prefix: str, - branch_suffix: str | None, - pr_labels: str, - no_pr: bool, - draft: bool, - verify: bool, - dry_run: bool, -) -> PortPlan: - """Validate inputs, resolve derived values, and confirm with the user. Aborts on failure.""" - user = app.config.github.user - if not user: - app.abort( - 'No GitHub user configured. Set `github.user` via `ddev config set github.user ` ' - 'or export DD_GITHUB_USER.' - ) - - if commit_hash is None: - head_commit = app.repo.git.latest_commit() - app.display_info(f'No commit specified. Current HEAD: `{head_commit.sha[:10]}` - {head_commit.subject}') - if not dry_run and not click.confirm('Use this commit?'): - app.abort('Did not get confirmation, aborting.') - commit_hash = head_commit.sha - - try: - full_sha = app.repo.git.capture('rev-parse', '--verify', f'{commit_hash}^{{commit}}').strip() - except OSError: - app.abort(f'Commit `{commit_hash}` does not exist.') - - log_entries = app.repo.git.log(['hash:%H', 'subject:%s'], n=1, source=full_sha) - if not log_entries: - app.abort(f'Could not read commit `{full_sha}`.') - clean_subject, original_pr = split_commit_subject(log_entries[0]['subject']) - - in_toto_files = [ - line - for line in app.repo.git.capture('diff-tree', '--no-commit-id', '--name-only', '-r', full_sha).splitlines() - if IN_TOTO_SUFFIX in line - ] - if in_toto_files: - listing = '\n '.join(in_toto_files) - app.display_warning( - f'Commit touches {len(in_toto_files)} `.in-toto` file(s); they will be preserved ' - f'from `{target_branch}`:\n {listing}' - ) - - suffix = branch_suffix or f'to-{target_branch}' - plan = PortPlan( - full_sha=full_sha, - clean_subject=clean_subject, - original_pr=original_pr, - target_branch=target_branch, - new_branch=f'{user}/{branch_prefix}-{full_sha[:10]}-{suffix}', - pr_title=f'[Backport] {clean_subject}', - pr_body=build_pr_body(app, sha=full_sha, subject=clean_subject, target=target_branch, original_pr=original_pr), - labels=parse_labels(pr_labels), - draft=draft, - create_pr=not no_pr, - verify=verify, - dry_run=dry_run, - ) - - app.display_info(_format_plan_summary(plan)) - - if not dry_run and not click.confirm('Continue?'): - app.abort('Did not get confirmation, aborting.') - - return plan - - -def _format_plan_summary(plan: PortPlan) -> str: - original_pr_line = f' Original PR: #{plan.original_pr}' if plan.original_pr else ' Original PR: (none)' - return '\n'.join( - [ - 'Configuration:', - f' Target branch: {plan.target_branch}', - f' Commit: {plan.full_sha[:10]} - {plan.clean_subject}', - original_pr_line, - f' New branch: {plan.new_branch}', - f' Create PR: {plan.create_pr} (draft={plan.draft})', - f' PR labels: {", ".join(plan.labels) if plan.labels else "(none)"}', - f' Verify commit: {plan.verify}', - f' Dry run: {plan.dry_run}', - ] - ) - - -def build_port_steps(app: Application, plan: PortPlan) -> tuple[list[PortStep], CreatePullRequestStep | None]: - """Build the ordered list of steps for the workflow, plus the PR step reference (or None).""" - steps: list[PortStep] = [ - FetchOriginStep(app, dry_run=plan.dry_run), - CheckoutTargetStep(app, target=plan.target_branch, dry_run=plan.dry_run), - CreatePortBranchStep(app, branch=plan.new_branch, dry_run=plan.dry_run), - CherryPickStep(app, sha=plan.full_sha, dry_run=plan.dry_run), - PreserveInTotoStep(app, dry_run=plan.dry_run), - CommitStep(app, subject=plan.clean_subject, verify=plan.verify, dry_run=plan.dry_run), - PushStep(app, branch=plan.new_branch, dry_run=plan.dry_run), - ] - pr_step: CreatePullRequestStep | None = None - if plan.create_pr: - pr_step = CreatePullRequestStep( - app, - title=plan.pr_title, - head=plan.new_branch, - base=plan.target_branch, - body=plan.pr_body, - labels=plan.labels, - draft=plan.draft, - dry_run=plan.dry_run, - ) - steps.append(pr_step) - return steps, pr_step - - @click.command(name='port-commit', short_help='Backport a commit onto a target branch') @click.pass_obj @click.argument('commit_hash', required=False) @@ -464,6 +53,8 @@ def port_commit( The GitHub user for the branch prefix is taken from `ddev config` (`github.user`) or the `DD_GITHUB_USER` / `GITHUB_USER` / `GITHUB_ACTOR` environment variables. """ + from ddev.cli.release.port_commit_workflow import PortStepError, build_port_steps, resolve_port_plan + plan = resolve_port_plan( app, commit_hash=commit_hash, diff --git a/ddev/src/ddev/cli/release/port_commit_workflow.py b/ddev/src/ddev/cli/release/port_commit_workflow.py new file mode 100644 index 0000000000000..873756b16e003 --- /dev/null +++ b/ddev/src/ddev/cli/release/port_commit_workflow.py @@ -0,0 +1,429 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Workflow internals for the `ddev release port-commit` command. + +Kept in a separate module so the command file stays small and the workflow +classes are not imported on `--help` or other command-listing operations. +Import this module from inside the command body, not at module top level. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from typing import TYPE_CHECKING + +import click + +if TYPE_CHECKING: + from ddev.cli.application import Application + + +PR_NUMBER_SUFFIX_PATTERN = re.compile(r'\s*\(#(\d+)\)\s*$') +PR_TEMPLATE_RELATIVE_PATH = '.github/PULL_REQUEST_TEMPLATE.md' +PR_TEMPLATE_HEADING = '### What does this PR do?' +IN_TOTO_SUFFIX = '.in-toto' + + +class PortStepError(Exception): + """Raised by a PortStep to signal a clean abort with a user-facing message.""" + + +class PortStep: + """Single step of the port-commit workflow.""" + + def __init__(self, app: Application, *, dry_run: bool = False) -> None: + self.app = app + self.dry_run = dry_run + + def describe(self) -> str: + raise NotImplementedError + + def planned_commands(self) -> list[str]: + return [] + + def execute(self) -> None: + raise NotImplementedError + + def run(self) -> None: + if self.dry_run: + self.app.display_info(self.describe()) + for cmd in self.planned_commands(): + self.app.display_info(f' (dry-run) {cmd}') + return + + with self.app.status(self.describe()): + try: + self.execute() + except PortStepError: + raise + except OSError as e: + raise PortStepError(str(e)) from e + + self.app.display_success(f'{self.describe()}: done.') + + +class FetchOriginStep(PortStep): + def describe(self) -> str: + return 'Fetching latest changes from origin' + + def planned_commands(self) -> list[str]: + return ['git fetch origin'] + + def execute(self) -> None: + self.app.repo.git.run('fetch', 'origin') + + +class CheckoutTargetStep(PortStep): + def __init__(self, app: Application, *, target: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.target = target + + def describe(self) -> str: + return f'Checking out and updating `{self.target}`' + + def planned_commands(self) -> list[str]: + return [f'git checkout {self.target}', f'git pull origin {self.target}'] + + def execute(self) -> None: + self.app.repo.git.run('checkout', self.target) + self.app.repo.git.run('pull', 'origin', self.target) + + +class CreatePortBranchStep(PortStep): + def __init__(self, app: Application, *, branch: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.branch = branch + + def describe(self) -> str: + return f'Creating branch `{self.branch}`' + + def planned_commands(self) -> list[str]: + return [f'git checkout -B {self.branch}'] + + def execute(self) -> None: + self.app.repo.git.run('checkout', '-B', self.branch) + + +class CherryPickStep(PortStep): + """Cherry-pick a commit, auto-resolving `.in-toto`-only conflicts.""" + + def __init__(self, app: Application, *, sha: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.sha = sha + + def describe(self) -> str: + return f'Cherry-picking {self.sha[:10]}' + + def planned_commands(self) -> list[str]: + return [f'git cherry-pick --no-commit {self.sha}'] + + def execute(self) -> None: + try: + self.app.repo.git.run('cherry-pick', '--no-commit', self.sha) + return + except OSError: + pass + + conflicts = [ + line for line in self.app.repo.git.capture('diff', '--name-only', '--diff-filter=U').splitlines() if line + ] + non_in_toto = [f for f in conflicts if IN_TOTO_SUFFIX not in f] + in_toto = [f for f in conflicts if IN_TOTO_SUFFIX in f] + + if non_in_toto: + try: + self.app.repo.git.run('cherry-pick', '--abort') + except OSError: + pass + listing = '\n '.join(non_in_toto) + raise PortStepError(f'Cherry-pick has conflicts in non-`.in-toto` files:\n {listing}') + + if not in_toto: + raise PortStepError('Cherry-pick failed without conflicts. Resolve manually and try again.') + + for path in in_toto: + _resolve_in_toto_conflict(self.app, path) + + +class PreserveInTotoStep(PortStep): + """Reset any staged `.in-toto` changes to keep the target branch's signature metadata.""" + + def describe(self) -> str: + return 'Preserving `.in-toto` files from target branch' + + def planned_commands(self) -> list[str]: + return ['# Reset any staged .in-toto changes to HEAD'] + + def execute(self) -> None: + staged = [line for line in self.app.repo.git.capture('diff', '--cached', '--name-only').splitlines() if line] + affected = [f for f in staged if IN_TOTO_SUFFIX in f] + if not affected: + return + + for path in affected: + _restore_path_from_head(self.app, path) + + +class CommitStep(PortStep): + def __init__(self, app: Application, *, subject: str, verify: bool = False, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.subject = subject + self.verify = verify + + @property + def message(self) -> str: + return f'[Backport] {self.subject}' + + def describe(self) -> str: + return f'Committing changes as "{self.message}"' + + def planned_commands(self) -> list[str]: + flags = '' if self.verify else '--no-verify ' + return [f'git commit {flags}-m "{self.message}"'] + + def execute(self) -> None: + args = ['commit', '-m', self.message] + if not self.verify: + args.insert(1, '--no-verify') + self.app.repo.git.run(*args) + + +class PushStep(PortStep): + def __init__(self, app: Application, *, branch: str, dry_run: bool = False) -> None: + super().__init__(app, dry_run=dry_run) + self.branch = branch + + def describe(self) -> str: + return f'Pushing `{self.branch}` to origin' + + def planned_commands(self) -> list[str]: + return [f'git push origin {self.branch}'] + + def execute(self) -> None: + self.app.repo.git.run('push', 'origin', self.branch) + + +class CreatePullRequestStep(PortStep): + def __init__( + self, + app: Application, + *, + title: str, + head: str, + base: str, + body: str, + labels: list[str], + draft: bool, + dry_run: bool = False, + ) -> None: + super().__init__(app, dry_run=dry_run) + self.title = title + self.head = head + self.base = base + self.body = body + self.labels = labels + self.draft = draft + self.pr_url: str | None = None + + def describe(self) -> str: + flavor = 'draft pull request' if self.draft else 'pull request' + return f'Creating {flavor} `{self.title}`' + + def planned_commands(self) -> list[str]: + label_part = f' --label {",".join(self.labels)}' if self.labels else '' + draft_part = ' --draft' if self.draft else '' + return [f'POST /repos/.../pulls (head={self.head}, base={self.base}{draft_part}){label_part}'] + + def execute(self) -> None: + self.pr_url = self.app.github.create_pull_request( + title=self.title, + head=self.head, + base=self.base, + body=self.body, + draft=self.draft, + labels=self.labels or None, + ) + + +def _resolve_in_toto_conflict(app: Application, path: str) -> None: + try: + app.repo.git.capture('cat-file', '-e', f'HEAD:{path}') + app.repo.git.run('checkout', '--ours', path) + app.repo.git.run('add', path) + except OSError: + app.repo.git.run('rm', '--force', path) + + +def _restore_path_from_head(app: Application, path: str) -> None: + try: + app.repo.git.capture('cat-file', '-e', f'HEAD:{path}') + app.repo.git.run('checkout', 'HEAD', '--', path) + except OSError: + app.repo.git.run('rm', '--force', path) + + +def split_commit_subject(subject: str) -> tuple[str, str | None]: + """Return (subject_without_pr_suffix, original_pr_number_or_None).""" + match = PR_NUMBER_SUFFIX_PATTERN.search(subject) + if not match: + return subject, None + return PR_NUMBER_SUFFIX_PATTERN.sub('', subject), match.group(1) + + +def build_pr_body(app: Application, *, sha: str, subject: str, target: str, original_pr: str | None) -> str: + info_lines = [f'**Backported commit**: `{sha[:10]}` - {subject}'] + if original_pr: + info_lines.append(f'**Original PR**: #{original_pr}') + info_lines.append(f'**Target branch**: `{target}`') + info_block = '\n'.join(info_lines) + '\n' + + template_path = app.repo.path / PR_TEMPLATE_RELATIVE_PATH + if not template_path.is_file(): + return f'{PR_TEMPLATE_HEADING}\n\n{info_block}' + + template = template_path.read_text() + if PR_TEMPLATE_HEADING in template: + return template.replace(PR_TEMPLATE_HEADING, f'{PR_TEMPLATE_HEADING}\n\n{info_block}', 1) + return f'{info_block}\n{template}' + + +def parse_labels(raw: str) -> list[str]: + return [label.strip() for label in raw.split(',') if label.strip()] + + +@dataclass(frozen=True) +class PortPlan: + """All values needed to execute the port workflow, resolved up front.""" + + full_sha: str + clean_subject: str + original_pr: str | None + target_branch: str + new_branch: str + pr_title: str + pr_body: str + labels: list[str] + draft: bool + create_pr: bool + verify: bool + dry_run: bool + + +def resolve_port_plan( + app: Application, + *, + commit_hash: str | None, + target_branch: str, + branch_prefix: str, + branch_suffix: str | None, + pr_labels: str, + no_pr: bool, + draft: bool, + verify: bool, + dry_run: bool, +) -> PortPlan: + """Validate inputs, resolve derived values, and confirm with the user. Aborts on failure.""" + user = app.config.github.user + if not user: + app.abort( + 'No GitHub user configured. Set `github.user` via `ddev config set github.user ` ' + 'or export DD_GITHUB_USER.' + ) + + if commit_hash is None: + head_commit = app.repo.git.latest_commit() + app.display_info(f'No commit specified. Current HEAD: `{head_commit.sha[:10]}` - {head_commit.subject}') + if not dry_run and not click.confirm('Use this commit?'): + app.abort('Did not get confirmation, aborting.') + commit_hash = head_commit.sha + + try: + full_sha = app.repo.git.capture('rev-parse', '--verify', f'{commit_hash}^{{commit}}').strip() + except OSError: + app.abort(f'Commit `{commit_hash}` does not exist.') + + log_entries = app.repo.git.log(['hash:%H', 'subject:%s'], n=1, source=full_sha) + if not log_entries: + app.abort(f'Could not read commit `{full_sha}`.') + clean_subject, original_pr = split_commit_subject(log_entries[0]['subject']) + + in_toto_files = [ + line + for line in app.repo.git.capture('diff-tree', '--no-commit-id', '--name-only', '-r', full_sha).splitlines() + if IN_TOTO_SUFFIX in line + ] + if in_toto_files: + listing = '\n '.join(in_toto_files) + app.display_warning( + f'Commit touches {len(in_toto_files)} `.in-toto` file(s); they will be preserved ' + f'from `{target_branch}`:\n {listing}' + ) + + suffix = branch_suffix or f'to-{target_branch}' + plan = PortPlan( + full_sha=full_sha, + clean_subject=clean_subject, + original_pr=original_pr, + target_branch=target_branch, + new_branch=f'{user}/{branch_prefix}-{full_sha[:10]}-{suffix}', + pr_title=f'[Backport] {clean_subject}', + pr_body=build_pr_body(app, sha=full_sha, subject=clean_subject, target=target_branch, original_pr=original_pr), + labels=parse_labels(pr_labels), + draft=draft, + create_pr=not no_pr, + verify=verify, + dry_run=dry_run, + ) + + app.display_info(_format_plan_summary(plan)) + + if not dry_run and not click.confirm('Continue?'): + app.abort('Did not get confirmation, aborting.') + + return plan + + +def _format_plan_summary(plan: PortPlan) -> str: + original_pr_line = f' Original PR: #{plan.original_pr}' if plan.original_pr else ' Original PR: (none)' + return '\n'.join( + [ + 'Configuration:', + f' Target branch: {plan.target_branch}', + f' Commit: {plan.full_sha[:10]} - {plan.clean_subject}', + original_pr_line, + f' New branch: {plan.new_branch}', + f' Create PR: {plan.create_pr} (draft={plan.draft})', + f' PR labels: {", ".join(plan.labels) if plan.labels else "(none)"}', + f' Verify commit: {plan.verify}', + f' Dry run: {plan.dry_run}', + ] + ) + + +def build_port_steps(app: Application, plan: PortPlan) -> tuple[list[PortStep], CreatePullRequestStep | None]: + """Build the ordered list of steps for the workflow, plus the PR step reference (or None).""" + steps: list[PortStep] = [ + FetchOriginStep(app, dry_run=plan.dry_run), + CheckoutTargetStep(app, target=plan.target_branch, dry_run=plan.dry_run), + CreatePortBranchStep(app, branch=plan.new_branch, dry_run=plan.dry_run), + CherryPickStep(app, sha=plan.full_sha, dry_run=plan.dry_run), + PreserveInTotoStep(app, dry_run=plan.dry_run), + CommitStep(app, subject=plan.clean_subject, verify=plan.verify, dry_run=plan.dry_run), + PushStep(app, branch=plan.new_branch, dry_run=plan.dry_run), + ] + pr_step: CreatePullRequestStep | None = None + if plan.create_pr: + pr_step = CreatePullRequestStep( + app, + title=plan.pr_title, + head=plan.new_branch, + base=plan.target_branch, + body=plan.pr_body, + labels=plan.labels, + draft=plan.draft, + dry_run=plan.dry_run, + ) + steps.append(pr_step) + return steps, pr_step diff --git a/ddev/tests/cli/release/test_port_commit.py b/ddev/tests/cli/release/test_port_commit.py index ab62b464e9552..a26b55228edc7 100644 --- a/ddev/tests/cli/release/test_port_commit.py +++ b/ddev/tests/cli/release/test_port_commit.py @@ -7,7 +7,7 @@ import pytest -from ddev.cli.release.port_commit import ( +from ddev.cli.release.port_commit_workflow import ( CherryPickStep, CommitStep, CreatePullRequestStep, From 7ff1f321a50c5f25d11cc041b1784c32fbb6bcc3 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Tue, 12 May 2026 17:16:44 +0200 Subject: [PATCH 05/11] Skip PR-URL display in dry-run instead of relying on null pr_url --- ddev/src/ddev/cli/release/port_commit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddev/src/ddev/cli/release/port_commit.py b/ddev/src/ddev/cli/release/port_commit.py index c58e577433881..ec3aca8477829 100644 --- a/ddev/src/ddev/cli/release/port_commit.py +++ b/ddev/src/ddev/cli/release/port_commit.py @@ -75,6 +75,6 @@ def port_commit( except PortStepError as e: app.abort(str(e)) - if pr_step is not None and pr_step.pr_url: + if pr_step is not None and not plan.dry_run: app.display_success(f'Pull request created: {pr_step.pr_url}') app.display_success('All done.') From 71744ae5d8c6c78899edd7b94872e83ad3c0d294 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Tue, 12 May 2026 17:41:48 +0200 Subject: [PATCH 06/11] Move PR creation to AsyncGitHubClient + add FakeAsyncGitHubClient test helper --- .../ddev/cli/release/port_commit_workflow.py | 56 +++++- ddev/src/ddev/utils/github.py | 24 +-- ddev/src/ddev/utils/github_async.py | 83 +++++++++ ddev/tests/cli/release/test_port_commit.py | 130 ++++++++++---- ddev/tests/conftest.py | 1 + ddev/tests/helpers/github_async.py | 170 ++++++++++++++++++ ddev/tests/utils/test_github_async.py | 82 +++++++++ 7 files changed, 482 insertions(+), 64 deletions(-) create mode 100644 ddev/tests/helpers/github_async.py diff --git a/ddev/src/ddev/cli/release/port_commit_workflow.py b/ddev/src/ddev/cli/release/port_commit_workflow.py index 873756b16e003..c145bafac33c8 100644 --- a/ddev/src/ddev/cli/release/port_commit_workflow.py +++ b/ddev/src/ddev/cli/release/port_commit_workflow.py @@ -210,6 +210,8 @@ def __init__( self, app: Application, *, + owner: str, + repo: str, title: str, head: str, base: str, @@ -219,6 +221,8 @@ def __init__( dry_run: bool = False, ) -> None: super().__init__(app, dry_run=dry_run) + self.owner = owner + self.repo = repo self.title = title self.head = head self.base = base @@ -234,17 +238,36 @@ def describe(self) -> str: def planned_commands(self) -> list[str]: label_part = f' --label {",".join(self.labels)}' if self.labels else '' draft_part = ' --draft' if self.draft else '' - return [f'POST /repos/.../pulls (head={self.head}, base={self.base}{draft_part}){label_part}'] + endpoint = f'/repos/{self.owner}/{self.repo}/pulls' + return [f'POST {endpoint} (head={self.head}, base={self.base}{draft_part}){label_part}'] def execute(self) -> None: - self.pr_url = self.app.github.create_pull_request( - title=self.title, - head=self.head, - base=self.base, - body=self.body, - draft=self.draft, - labels=self.labels or None, - ) + import asyncio + + self.pr_url = asyncio.run(self._create_pr()) + + async def _create_pr(self) -> str | None: + from ddev.utils.github_async import async_github_client + + async with async_github_client(token=self.app.config.github.token) as client: + response = await client.create_pull_request( + owner=self.owner, + repo=self.repo, + title=self.title, + head=self.head, + base=self.base, + body=self.body, + draft=self.draft, + ) + pr = response.data + if self.labels: + await client.add_labels_to_issue( + owner=self.owner, + repo=self.repo, + issue_number=pr.number, + labels=self.labels, + ) + return pr.html_url def _resolve_in_toto_conflict(app: Application, path: str) -> None: @@ -293,6 +316,18 @@ def parse_labels(raw: str) -> list[str]: return [label.strip() for label in raw.split(',') if label.strip()] +def resolve_owner_repo(app: Application) -> tuple[str, str]: + """Resolve (owner, repo) for the active repository. + + Falls back to `DataDog/` when `full_name` is unqualified. + """ + full = app.repo.full_name + if '/' in full: + owner, repo = full.split('/', 1) + return owner, repo + return 'DataDog', full + + @dataclass(frozen=True) class PortPlan: """All values needed to execute the port workflow, resolved up front.""" @@ -415,8 +450,11 @@ def build_port_steps(app: Application, plan: PortPlan) -> tuple[list[PortStep], ] pr_step: CreatePullRequestStep | None = None if plan.create_pr: + owner, repo = resolve_owner_repo(app) pr_step = CreatePullRequestStep( app, + owner=owner, + repo=repo, title=plan.pr_title, head=plan.new_branch, base=plan.target_branch, diff --git a/ddev/src/ddev/utils/github.py b/ddev/src/ddev/utils/github.py index de2e462946d59..e258cabc0bb4e 100644 --- a/ddev/src/ddev/utils/github.py +++ b/ddev/src/ddev/utils/github.py @@ -75,9 +75,6 @@ class GitHubManager: # https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#create-a-pull-request PULLS_API = 'https://api.github.com/repos/{repo_id}/pulls' - # https://docs.github.com/en/rest/issues/labels?apiVersion=2022-11-28#add-labels-to-an-issue - ISSUES_LABELS_API = 'https://api.github.com/repos/{repo_id}/issues/{issue_number}/labels' - # https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#list-pull-requests-files PULL_REQUEST_FILES_API = 'https://api.github.com/repos/{repo_id}/pulls/{pr_number}/files' @@ -243,27 +240,12 @@ def create_label(self, name, color): def create_milestone(self, title: str) -> None: self.__api_post(self.MILESTONES_API.format(repo_id=self.repo_id), content=json.dumps({'title': title})) - def create_pull_request( - self, - title: str, - head: str, - base: str, - body: str = '', - *, - draft: bool = False, - labels: list[str] | None = None, - ) -> str: + def create_pull_request(self, title: str, head: str, base: str, body: str = '') -> str: response = self.__api_post( self.PULLS_API.format(repo_id=self.repo_id), - content=json.dumps({'title': title, 'head': head, 'base': base, 'body': body, 'draft': draft}), + content=json.dumps({'title': title, 'head': head, 'base': base, 'body': body}), ) - pr_data = response.json() - if labels: - self.__api_post( - self.ISSUES_LABELS_API.format(repo_id=self.repo_id, issue_number=pr_data['number']), - content=json.dumps({'labels': labels}), - ) - return pr_data['html_url'] + return response.json()['html_url'] def get_label(self, name): return self.__api_get(f'{self.LABELS_API.format(repo_id=self.repo_id)}/{name}') diff --git a/ddev/src/ddev/utils/github_async.py b/ddev/src/ddev/utils/github_async.py index 7bfac9a3539e6..c8eebc00b4a70 100644 --- a/ddev/src/ddev/utils/github_async.py +++ b/ddev/src/ddev/utils/github_async.py @@ -108,6 +108,19 @@ class IssueComment(BaseModel): html_url: str | None = None +class PullRequest(BaseModel): + """A GitHub pull request (minimal fields, extend as needed). + + Field reference: + https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request + """ + + model_config = ConfigDict(extra="ignore") + + number: int + html_url: str + + class PullRequestReviewComment(BaseModel): """An inline review comment on a pull request diff.""" @@ -335,6 +348,76 @@ async def create_issue_comment( ) return self._parse_response(response, IssueComment) + async def create_pull_request( + self, + owner: str, + repo: str, + title: str, + head: str, + base: str, + body: str = "", + draft: bool = False, + timeout: float | None = None, + ) -> GitHubResponse[PullRequest]: + """ + Calls the GitHub API to create a pull request. + + GitHub API Documentation: + https://docs.github.com/en/rest/pulls/pulls#create-a-pull-request + + Args: + owner: Repository owner (user or organisation). + repo: Repository name. + title: Pull request title. + head: Name of the branch containing the changes. + base: Name of the branch to merge into. + body: Pull request body. + draft: Whether to open the pull request as a draft. + timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + + Returns: + GitHubResponse[PullRequest]: The validated pull request data and headers. + """ + response = await self._request( + "POST", + f"/repos/{owner}/{repo}/pulls", + timeout=timeout, + json={"title": title, "head": head, "base": base, "body": body, "draft": draft}, + ) + return self._parse_response(response, PullRequest) + + async def add_labels_to_issue( + self, + owner: str, + repo: str, + issue_number: int, + labels: list[str], + timeout: float | None = None, + ) -> GitHubResponse[None]: + """ + Calls the GitHub API to add one or more labels to an issue or pull request. + + GitHub API Documentation: + https://docs.github.com/en/rest/issues/labels#add-labels-to-an-issue + + Args: + owner: Repository owner (user or organisation). + repo: Repository name. + issue_number: Issue or pull request number. + labels: Labels to add. Existing labels on the issue are preserved. + timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. + + Returns: + GitHubResponse[None]: Empty response with headers. + """ + response = await self._request( + "POST", + f"/repos/{owner}/{repo}/issues/{issue_number}/labels", + timeout=timeout, + json={"labels": labels}, + ) + return GitHubResponse[None].model_validate({"data": None, "headers": dict(response.headers)}) + async def create_pr_review_comment( self, owner: str, diff --git a/ddev/tests/cli/release/test_port_commit.py b/ddev/tests/cli/release/test_port_commit.py index a26b55228edc7..e8d925fe2e157 100644 --- a/ddev/tests/cli/release/test_port_commit.py +++ b/ddev/tests/cli/release/test_port_commit.py @@ -6,6 +6,7 @@ from unittest.mock import MagicMock, call import pytest +from pytest_mock import MockerFixture from ddev.cli.release.port_commit_workflow import ( CherryPickStep, @@ -19,10 +20,13 @@ split_commit_subject, ) from ddev.utils.git import GitCommit +from ddev.utils.github_async import GitHubResponse, PullRequest +from tests.helpers.github_async import FakeAsyncGitHubClient +from tests.helpers.runner import CliRunner @pytest.fixture -def app_mock(mocker): +def app_mock(mocker: MockerFixture) -> MagicMock: app = mocker.MagicMock() app.status.return_value.__enter__ = MagicMock(return_value=None) app.status.return_value.__exit__ = MagicMock(return_value=None) @@ -212,10 +216,17 @@ def test_commit_step(app_mock, verify, expected_args): app_mock.repo.git.run.assert_called_once_with(*expected_args) -def test_create_pull_request_step(app_mock): - app_mock.github.create_pull_request.return_value = 'https://github.com/x/pr/1' +def test_create_pull_request_step(app_mock: MagicMock, fake_async_github: FakeAsyncGitHubClient) -> None: + app_mock.config.github.token = 'ghp_test' + fake_async_github.pull_request_response = GitHubResponse[PullRequest]( + data=PullRequest(number=7, html_url='https://github.com/x/pr/1'), + headers={}, + ) + step = CreatePullRequestStep( app_mock, + owner='DataDog', + repo='integrations-core', title='[Backport] Fix bug', head='alice/port-deadbeef00-to-7.62.x', base='7.62.x', @@ -224,17 +235,58 @@ def test_create_pull_request_step(app_mock): draft=False, ) step.execute() - app_mock.github.create_pull_request.assert_called_once_with( + + fake_async_github.assert_called_once_with( + 'create_pull_request', + owner='DataDog', + repo='integrations-core', title='[Backport] Fix bug', head='alice/port-deadbeef00-to-7.62.x', base='7.62.x', body='body', draft=False, + ) + fake_async_github.assert_called_once_with( + 'add_labels_to_issue', + owner='DataDog', + repo='integrations-core', + issue_number=7, labels=['qa/skip-qa'], ) assert step.pr_url == 'https://github.com/x/pr/1' +def test_create_pull_request_step_skips_label_call_when_no_labels( + app_mock: MagicMock, fake_async_github: FakeAsyncGitHubClient +) -> None: + app_mock.config.github.token = 'ghp_test' + + step = CreatePullRequestStep( + app_mock, + owner='DataDog', + repo='integrations-core', + title='[Backport] Fix', + head='alice/x', + base='master', + body='body', + labels=[], + draft=True, + ) + step.execute() + + fake_async_github.assert_called_once_with( + 'create_pull_request', + owner='DataDog', + repo='integrations-core', + title='[Backport] Fix', + head='alice/x', + base='master', + body='body', + draft=True, + ) + fake_async_github.assert_not_called('add_labels_to_issue') + + def _setup_command_mocks(mocker, *, commit_sha='deadbeef0011223344', subject='Fix bug (#100)', in_toto=''): mocker.patch('ddev.utils.git.GitRepository.run') capture_mock = mocker.patch('ddev.utils.git.GitRepository.capture') @@ -252,10 +304,11 @@ def _setup_command_mocks(mocker, *, commit_sha='deadbeef0011223344', subject='Fi ) -def test_command_happy_path(ddev, mocker): +def test_command_happy_path(ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient) -> None: _setup_command_mocks(mocker) - pr_mock = mocker.patch( - 'ddev.utils.github.GitHubManager.create_pull_request', return_value='https://github.com/x/pr/1' + fake_async_github.pull_request_response = GitHubResponse[PullRequest]( + data=PullRequest(number=1, html_url='https://github.com/x/pr/1'), + headers={}, ) mocker.patch('click.confirm', return_value=True) mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) @@ -264,46 +317,54 @@ def test_command_happy_path(ddev, mocker): assert result.exit_code == 0, result.output assert 'Pull request created: https://github.com/x/pr/1' in result.output - pr_mock.assert_called_once() - _, kwargs = pr_mock.call_args - assert kwargs['draft'] is False - assert kwargs['labels'] == ['qa/skip-qa'] - assert kwargs['head'] == 'alice/port-deadbeef00-to-master' - assert kwargs['base'] == 'master' - assert kwargs['title'] == '[Backport] Fix bug' + + pr_call = fake_async_github.last_call('create_pull_request') + assert pr_call.kwargs['owner'] == 'DataDog' + assert pr_call.kwargs['repo'] == 'integrations-core' + assert pr_call.kwargs['title'] == '[Backport] Fix bug' + assert pr_call.kwargs['head'] == 'alice/port-deadbeef00-to-master' + assert pr_call.kwargs['base'] == 'master' + assert pr_call.kwargs['draft'] is False + assert '**Backported commit**: `deadbeef00`' in pr_call.kwargs['body'] + assert '**Original PR**: #100' in pr_call.kwargs['body'] + + fake_async_github.assert_called_once_with( + 'add_labels_to_issue', + owner='DataDog', + repo='integrations-core', + issue_number=1, + labels=['qa/skip-qa'], + ) -def test_command_draft_flag(ddev, mocker): +def test_command_draft_flag(ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient) -> None: _setup_command_mocks(mocker) - pr_mock = mocker.patch( - 'ddev.utils.github.GitHubManager.create_pull_request', return_value='https://github.com/x/pr/2' - ) mocker.patch('click.confirm', return_value=True) mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) result = ddev('release', 'port-commit', '--draft', 'deadbeef0011223344') assert result.exit_code == 0, result.output - _, kwargs = pr_mock.call_args - assert kwargs['draft'] is True + assert fake_async_github.last_call('create_pull_request').kwargs['draft'] is True -def test_command_no_pr(ddev, mocker): +def test_command_no_pr(ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient) -> None: _setup_command_mocks(mocker) - pr_mock = mocker.patch('ddev.utils.github.GitHubManager.create_pull_request') mocker.patch('click.confirm', return_value=True) mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) result = ddev('release', 'port-commit', '--no-pr', 'deadbeef0011223344') assert result.exit_code == 0, result.output - pr_mock.assert_not_called() + fake_async_github.assert_not_called('create_pull_request') + fake_async_github.assert_not_called('add_labels_to_issue') -def test_command_dry_run_makes_no_mutating_calls(ddev, mocker): +def test_command_dry_run_makes_no_mutating_calls( + ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient +) -> None: _setup_command_mocks(mocker) run_mock = mocker.patch('ddev.utils.git.GitRepository.run') - pr_mock = mocker.patch('ddev.utils.github.GitHubManager.create_pull_request') mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) result = ddev('release', 'port-commit', '--dry-run', 'deadbeef0011223344') @@ -311,10 +372,10 @@ def test_command_dry_run_makes_no_mutating_calls(ddev, mocker): assert result.exit_code == 0, result.output assert '(dry-run)' in result.output run_mock.assert_not_called() - pr_mock.assert_not_called() + fake_async_github.assert_not_called('create_pull_request') -def test_command_aborts_when_no_github_user(ddev, mocker): +def test_command_aborts_when_no_github_user(ddev: CliRunner, mocker: MockerFixture) -> None: mocker.patch.dict('os.environ', {'DD_GITHUB_USER': '', 'GITHUB_USER': '', 'GITHUB_ACTOR': ''}) result = ddev('release', 'port-commit', 'deadbeef0011223344') @@ -323,9 +384,10 @@ def test_command_aborts_when_no_github_user(ddev, mocker): assert 'No GitHub user configured' in result.output -def test_command_aborts_on_unconfirmed(ddev, mocker): +def test_command_aborts_on_unconfirmed( + ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient +) -> None: _setup_command_mocks(mocker) - mocker.patch('ddev.utils.github.GitHubManager.create_pull_request') mocker.patch('click.confirm', return_value=False) mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) @@ -333,13 +395,13 @@ def test_command_aborts_on_unconfirmed(ddev, mocker): assert result.exit_code == 1, result.output assert 'Did not get confirmation' in result.output + fake_async_github.assert_not_called('create_pull_request') -def test_command_uses_head_when_no_commit_given(ddev, mocker): +def test_command_uses_head_when_no_commit_given( + ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient +) -> None: _setup_command_mocks(mocker) - pr_mock = mocker.patch( - 'ddev.utils.github.GitHubManager.create_pull_request', return_value='https://github.com/x/pr/3' - ) mocker.patch('click.confirm', return_value=True) mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) @@ -347,4 +409,4 @@ def test_command_uses_head_when_no_commit_given(ddev, mocker): assert result.exit_code == 0, result.output assert 'No commit specified' in result.output - pr_mock.assert_called_once() + assert len(fake_async_github.calls_to('create_pull_request')) == 1 diff --git a/ddev/tests/conftest.py b/ddev/tests/conftest.py index 2295b34b87f23..6496a68534da8 100644 --- a/ddev/tests/conftest.py +++ b/ddev/tests/conftest.py @@ -24,6 +24,7 @@ from .helpers import APPLICATION, LOCAL_REPO_BRANCH, PLATFORM from .helpers.git import ClonedRepo +from .helpers.github_async import fake_async_github # noqa: F401 from .helpers.runner import CliRunner # Rewrite assertions on the assertions helper module diff --git a/ddev/tests/helpers/github_async.py b/ddev/tests/helpers/github_async.py new file mode 100644 index 0000000000000..ecf0003b3b962 --- /dev/null +++ b/ddev/tests/helpers/github_async.py @@ -0,0 +1,170 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Test helpers for the async GitHub client. + +Provides a `FakeAsyncGitHubClient` that records every call and exposes +assertion helpers, plus a `fake_async_github` pytest fixture that patches +`async_github_client` to yield the fake and sets a stub token. + +Usage: + + def test_thing(fake_async_github): + do_something_that_creates_a_pr() + fake_async_github.assert_called( + 'create_pull_request', title='[Backport] Fix bug', draft=False + ) +""" + +from __future__ import annotations + +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from dataclasses import dataclass, field +from typing import Any + +import pytest +from pytest_mock import MockerFixture + +from ddev.utils.github_async import GitHubResponse, PullRequest + + +@dataclass +class RecordedRequest: + """A single recorded call to the fake client.""" + + method: str + kwargs: dict[str, Any] = field(default_factory=dict) + + +class FakeAsyncGitHubClient: + """Test double for AsyncGitHubClient that records calls and returns canned responses. + + Override response shape by assigning to `pull_request_response` before the call occurs. + """ + + def __init__(self) -> None: + self.requests: list[RecordedRequest] = [] + self.pull_request_response: GitHubResponse[PullRequest] = GitHubResponse[PullRequest]( + data=PullRequest(number=1, html_url='https://github.com/test/repo/pull/1'), + headers={}, + ) + + # ------------------------------------------------------------------ + # Mirrored API surface + # ------------------------------------------------------------------ + + async def create_pull_request( + self, + owner: str, + repo: str, + title: str, + head: str, + base: str, + body: str = '', + draft: bool = False, + timeout: float | None = None, + ) -> GitHubResponse[PullRequest]: + self._record( + 'create_pull_request', + owner=owner, + repo=repo, + title=title, + head=head, + base=base, + body=body, + draft=draft, + ) + return self.pull_request_response + + async def add_labels_to_issue( + self, + owner: str, + repo: str, + issue_number: int, + labels: list[str], + timeout: float | None = None, + ) -> GitHubResponse[None]: + self._record('add_labels_to_issue', owner=owner, repo=repo, issue_number=issue_number, labels=labels) + return GitHubResponse[None].model_validate({'data': None, 'headers': {}}) + + async def aclose(self) -> None: + return None + + # ------------------------------------------------------------------ + # Inspection / assertions + # ------------------------------------------------------------------ + + def calls_to(self, method: str) -> list[RecordedRequest]: + """Return every recorded call to *method*.""" + return [r for r in self.requests if r.method == method] + + def last_call(self, method: str) -> RecordedRequest: + """Return the most recent recorded call to *method*, raising if there are none. + + Use when strict full-kwargs assertion is too tedious (e.g. a long PR body) and + you want to inspect individual fields with plain asserts. + """ + calls = self.calls_to(method) + if not calls: + raise AssertionError(f'No calls to {method!r} were recorded.') + return calls[-1] + + def assert_called_with(self, method: str, **expected_kwargs: Any) -> RecordedRequest: + """Assert *method* was called at least once with EXACTLY *expected_kwargs*. + + Strict equality: every keyword the implementation passed must appear in + *expected_kwargs* and vice versa. Missing or extra keys both fail. Returns + the first matching call. + """ + matches = [r for r in self.calls_to(method) if r.kwargs == expected_kwargs] + if not matches: + raise AssertionError( + f'No call to {method!r} matched {expected_kwargs!r}. Recorded calls: {self.calls_to(method)}' + ) + return matches[0] + + def assert_called_once_with(self, method: str, **expected_kwargs: Any) -> RecordedRequest: + """Assert *method* was called exactly once with EXACTLY *expected_kwargs*. + + Strict equality, mirrors `Mock.assert_called_once_with`. + """ + calls = self.calls_to(method) + matches = [r for r in calls if r.kwargs == expected_kwargs] + if len(calls) != 1 or len(matches) != 1: + raise AssertionError( + f'Expected exactly one call to {method!r} with {expected_kwargs!r}; ' + f'got {len(calls)} call(s), {len(matches)} matching. Recorded calls: {calls}' + ) + return matches[0] + + def assert_not_called(self, method: str) -> None: + """Assert that *method* was never called.""" + calls = self.calls_to(method) + if calls: + raise AssertionError(f'Expected no calls to {method!r}, but got: {calls}') + + # ------------------------------------------------------------------ + # Internals + # ------------------------------------------------------------------ + + def _record(self, method: str, **kwargs: Any) -> None: + self.requests.append(RecordedRequest(method=method, kwargs=kwargs)) + + +@pytest.fixture +def fake_async_github(mocker: MockerFixture) -> FakeAsyncGitHubClient: + """Patch `async_github_client` to yield a `FakeAsyncGitHubClient` and stub the token.""" + fake = FakeAsyncGitHubClient() + + @asynccontextmanager + async def fake_context( + token: str | None = None, + default_timeout: float = 30.0, + transport: Any = None, + ) -> AsyncIterator[FakeAsyncGitHubClient]: + yield fake + + mocker.patch('ddev.utils.github_async.async_github_client', fake_context) + mocker.patch.dict('os.environ', {'DD_GITHUB_TOKEN': 'ghp_test'}) + return fake diff --git a/ddev/tests/utils/test_github_async.py b/ddev/tests/utils/test_github_async.py index 6b02f9f8cec57..845c3683ca5d5 100644 --- a/ddev/tests/utils/test_github_async.py +++ b/ddev/tests/utils/test_github_async.py @@ -16,6 +16,7 @@ GitHubResponse, IssueComment, PaginationData, + PullRequest, PullRequestReviewComment, WorkflowRun, async_github_client, @@ -411,6 +412,87 @@ async def test_create_pr_review_comment_http_error_raises() -> None: assert exc_info.value.response.status_code == 422 +# --------------------------------------------------------------------------- +# create_pull_request +# --------------------------------------------------------------------------- + + +def _pull_request_payload(number: int = 1) -> dict[str, Any]: + return { + "number": number, + "html_url": f"https://github.com/owner/repo/pull/{number}", + } + + +@pytest.mark.asyncio +async def test_create_pull_request_success() -> None: + def handler(request: httpx.Request) -> httpx.Response: + assert request.method == "POST" + assert "/repos/owner/repo/pulls" in request.url.path + body = json.loads(request.content) + assert body == { + "title": "Fix bug", + "head": "alice/fix", + "base": "master", + "body": "Fix description", + "draft": False, + } + return _json_response(_pull_request_payload(number=42), status_code=201) + + client = _make_client(httpx.MockTransport(handler)) + result = await client.create_pull_request("owner", "repo", "Fix bug", "alice/fix", "master", "Fix description") + assert isinstance(result.data, PullRequest) + assert result.data.number == 42 + assert result.data.html_url == "https://github.com/owner/repo/pull/42" + + +@pytest.mark.asyncio +async def test_create_pull_request_draft_true_forwarded() -> None: + def handler(request: httpx.Request) -> httpx.Response: + body = json.loads(request.content) + assert body["draft"] is True + return _json_response(_pull_request_payload(number=7), status_code=201) + + client = _make_client(httpx.MockTransport(handler)) + result = await client.create_pull_request("o", "r", "T", "h", "b", draft=True) + assert result.data.number == 7 + + +@pytest.mark.asyncio +async def test_create_pull_request_http_error_raises() -> None: + client = _make_client(httpx.MockTransport(lambda r: httpx.Response(422))) + with pytest.raises(httpx.HTTPStatusError) as exc_info: + await client.create_pull_request("o", "r", "T", "h", "b") + assert exc_info.value.response.status_code == 422 + + +# --------------------------------------------------------------------------- +# add_labels_to_issue +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_add_labels_to_issue_success() -> None: + def handler(request: httpx.Request) -> httpx.Response: + assert request.method == "POST" + assert "/repos/owner/repo/issues/3/labels" in request.url.path + body = json.loads(request.content) + assert body == {"labels": ["qa/skip-qa", "backport/7.62.x"]} + return _json_response([{"id": 1, "name": "qa/skip-qa"}, {"id": 2, "name": "backport/7.62.x"}], status_code=200) + + client = _make_client(httpx.MockTransport(handler)) + result = await client.add_labels_to_issue("owner", "repo", 3, ["qa/skip-qa", "backport/7.62.x"]) + assert result.data is None + + +@pytest.mark.asyncio +async def test_add_labels_to_issue_http_error_raises() -> None: + client = _make_client(httpx.MockTransport(lambda r: httpx.Response(404))) + with pytest.raises(httpx.HTTPStatusError) as exc_info: + await client.add_labels_to_issue("o", "r", 1, ["bug"]) + assert exc_info.value.response.status_code == 404 + + # --------------------------------------------------------------------------- # Custom timeout per request # --------------------------------------------------------------------------- From 6c0865046bfebdded4fe9834c2b55777ec8b8773 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Wed, 13 May 2026 08:56:20 +0200 Subject: [PATCH 07/11] Expand PullRequest async model with typical PR fields and sub-models --- ddev/src/ddev/utils/github_async.py | 83 ++++++++++++++++++++- ddev/tests/utils/test_github_async.py | 100 ++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 1 deletion(-) diff --git a/ddev/src/ddev/utils/github_async.py b/ddev/src/ddev/utils/github_async.py index c8eebc00b4a70..279e63b93ae84 100644 --- a/ddev/src/ddev/utils/github_async.py +++ b/ddev/src/ddev/utils/github_async.py @@ -108,8 +108,52 @@ class IssueComment(BaseModel): html_url: str | None = None +class GitHubUser(BaseModel): + """A GitHub user as returned by the REST API. + + Field reference: + https://docs.github.com/en/rest/users/users#get-a-user + """ + + model_config = ConfigDict(extra="ignore") + + id: int + login: str + html_url: str | None = None + type: str | None = None # 'User', 'Bot', 'Organization', etc. + + +class Label(BaseModel): + """A label attached to an issue or pull request. + + Field reference: + https://docs.github.com/en/rest/issues/labels#get-a-label + """ + + model_config = ConfigDict(extra="ignore") + + id: int + name: str + color: str | None = None + description: str | None = None + + +class PullRequestRef(BaseModel): + """A head or base branch reference on a pull request. + + Field reference (within the `pull-request` object): + https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request + """ + + model_config = ConfigDict(extra="ignore") + + ref: str + sha: str + label: str | None = None # e.g. 'octocat:new-topic' + + class PullRequest(BaseModel): - """A GitHub pull request (minimal fields, extend as needed). + """A GitHub pull request. Field reference: https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request @@ -117,8 +161,45 @@ class PullRequest(BaseModel): model_config = ConfigDict(extra="ignore") + # Identifiers + id: int | None = None number: int + node_id: str | None = None + + # URLs + url: str | None = None html_url: str + diff_url: str | None = None + patch_url: str | None = None + + # State + state: str | None = None # 'open' or 'closed' + draft: bool = False + merged: bool | None = None + locked: bool = False + merge_commit_sha: str | None = None + + # Content + title: str | None = None + body: str | None = None + + # People + user: GitHubUser | None = None + assignees: list[GitHubUser] = Field(default_factory=list) + requested_reviewers: list[GitHubUser] = Field(default_factory=list) + + # Labels + labels: list[Label] = Field(default_factory=list) + + # Timestamps (ISO 8601 strings; not parsed into datetime to keep the model lightweight) + created_at: str | None = None + updated_at: str | None = None + closed_at: str | None = None + merged_at: str | None = None + + # Branch references + head: PullRequestRef | None = None + base: PullRequestRef | None = None class PullRequestReviewComment(BaseModel): diff --git a/ddev/tests/utils/test_github_async.py b/ddev/tests/utils/test_github_async.py index 845c3683ca5d5..dbd71a079bed6 100644 --- a/ddev/tests/utils/test_github_async.py +++ b/ddev/tests/utils/test_github_async.py @@ -14,9 +14,12 @@ ArtifactsList, AsyncGitHubClient, GitHubResponse, + GitHubUser, IssueComment, + Label, PaginationData, PullRequest, + PullRequestRef, PullRequestReviewComment, WorkflowRun, async_github_client, @@ -424,6 +427,54 @@ def _pull_request_payload(number: int = 1) -> dict[str, Any]: } +def _full_pull_request_payload(number: int = 42) -> dict[str, Any]: + """A richer PR payload exercising sub-models (user, labels, head/base).""" + return { + "id": 9000 + number, + "number": number, + "node_id": "PR_kwDOABCD123", + "url": f"https://api.github.com/repos/owner/repo/pulls/{number}", + "html_url": f"https://github.com/owner/repo/pull/{number}", + "diff_url": f"https://github.com/owner/repo/pull/{number}.diff", + "patch_url": f"https://github.com/owner/repo/pull/{number}.patch", + "state": "open", + "draft": True, + "merged": False, + "locked": False, + "merge_commit_sha": None, + "title": "Fix bug", + "body": "Backport", + "user": { + "id": 1, + "login": "octocat", + "html_url": "https://github.com/octocat", + "type": "User", + }, + "assignees": [], + "requested_reviewers": [ + {"id": 2, "login": "reviewer", "type": "User"}, + ], + "labels": [ + {"id": 100, "name": "qa/skip-qa", "color": "5319e7"}, + {"id": 101, "name": "backport/7.62.x", "color": "5319e7"}, + ], + "created_at": "2026-05-01T00:00:00Z", + "updated_at": "2026-05-02T00:00:00Z", + "closed_at": None, + "merged_at": None, + "head": { + "ref": "alice/fix", + "sha": "deadbeef0011223344", + "label": "alice:alice/fix", + }, + "base": { + "ref": "master", + "sha": "cafebabe00", + "label": "owner:master", + }, + } + + @pytest.mark.asyncio async def test_create_pull_request_success() -> None: def handler(request: httpx.Request) -> httpx.Response: @@ -466,6 +517,55 @@ async def test_create_pull_request_http_error_raises() -> None: assert exc_info.value.response.status_code == 422 +@pytest.mark.asyncio +async def test_create_pull_request_parses_full_response() -> None: + """Exercises sub-models (GitHubUser, Label, PullRequestRef) end-to-end.""" + + def handler(request: httpx.Request) -> httpx.Response: + return _json_response(_full_pull_request_payload(number=42), status_code=201) + + client = _make_client(httpx.MockTransport(handler)) + result = await client.create_pull_request("owner", "repo", "Fix bug", "alice/fix", "master") + + pr = result.data + assert pr.id == 9042 + assert pr.number == 42 + assert pr.state == "open" + assert pr.draft is True + assert pr.title == "Fix bug" + + assert isinstance(pr.user, GitHubUser) + assert pr.user.login == "octocat" + + assert [label.name for label in pr.labels] == ["qa/skip-qa", "backport/7.62.x"] + assert all(isinstance(label, Label) for label in pr.labels) + + assert isinstance(pr.head, PullRequestRef) + assert pr.head.ref == "alice/fix" + assert pr.head.sha == "deadbeef0011223344" + assert isinstance(pr.base, PullRequestRef) + assert pr.base.ref == "master" + + assert [r.login for r in pr.requested_reviewers] == ["reviewer"] + assert pr.created_at == "2026-05-01T00:00:00Z" + + +@pytest.mark.asyncio +async def test_create_pull_request_ignores_extra_fields() -> None: + """Unknown top-level fields in the response must not break parsing.""" + + def handler(request: httpx.Request) -> httpx.Response: + payload = _full_pull_request_payload() + payload["mergeable_state"] = "clean" + payload["additions"] = 42 + payload["unknown_future_field"] = {"nested": True} + return _json_response(payload, status_code=201) + + client = _make_client(httpx.MockTransport(handler)) + result = await client.create_pull_request("o", "r", "T", "h", "b") + assert result.data.number == 42 + + # --------------------------------------------------------------------------- # add_labels_to_issue # --------------------------------------------------------------------------- From 19237bcb74cc58c4e23ab47bf935baaed1b0d000 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Wed, 13 May 2026 09:05:44 +0200 Subject: [PATCH 08/11] Reorganize github_async into a package with lazy model imports --- ddev/src/ddev/utils/github_async/__init__.py | 64 +++++ .../client.py} | 252 +++--------------- .../utils/github_async/models/__init__.py | 72 +++++ .../ddev/utils/github_async/models/comment.py | 38 +++ .../ddev/utils/github_async/models/label.py | 23 ++ .../utils/github_async/models/pull_request.py | 75 ++++++ .../ddev/utils/github_async/models/user.py | 23 ++ .../utils/github_async/models/workflow.py | 44 +++ .../src/ddev/utils/github_async/pagination.py | 37 +++ ddev/src/ddev/utils/github_async/response.py | 17 ++ ddev/tests/cli/release/test_port_commit.py | 3 +- ddev/tests/helpers/github_async.py | 3 +- ddev/tests/utils/test_github_async.py | 50 +++- 13 files changed, 474 insertions(+), 227 deletions(-) create mode 100644 ddev/src/ddev/utils/github_async/__init__.py rename ddev/src/ddev/utils/{github_async.py => github_async/client.py} (68%) create mode 100644 ddev/src/ddev/utils/github_async/models/__init__.py create mode 100644 ddev/src/ddev/utils/github_async/models/comment.py create mode 100644 ddev/src/ddev/utils/github_async/models/label.py create mode 100644 ddev/src/ddev/utils/github_async/models/pull_request.py create mode 100644 ddev/src/ddev/utils/github_async/models/user.py create mode 100644 ddev/src/ddev/utils/github_async/models/workflow.py create mode 100644 ddev/src/ddev/utils/github_async/pagination.py create mode 100644 ddev/src/ddev/utils/github_async/response.py diff --git a/ddev/src/ddev/utils/github_async/__init__.py b/ddev/src/ddev/utils/github_async/__init__.py new file mode 100644 index 0000000000000..f3137ea0d283a --- /dev/null +++ b/ddev/src/ddev/utils/github_async/__init__.py @@ -0,0 +1,64 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Async GitHub REST API client. + +Typical usage:: + + from ddev.utils.github_async import async_github_client + from ddev.utils.github_async.models import PullRequest + + async with async_github_client(token=my_token) as client: + response = await client.create_pull_request(...) + +Both this package's top-level symbols and the ``models`` subpackage use PEP 562 +``__getattr__`` to load submodules on demand. Importing one name does not +eagerly pull in the rest of the package; in particular, importing a model from +``ddev.utils.github_async.models`` does not load the HTTP client, and vice +versa. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + # Re-exports for type checkers / IDE autocomplete only; do not execute at runtime. + # The `X as X` aliases mark these as explicit re-exports for linters. + from .client import DEFAULT_BASE_URL as DEFAULT_BASE_URL + from .client import GITHUB_API_VERSION as GITHUB_API_VERSION + from .client import AsyncGitHubClient as AsyncGitHubClient + from .client import async_github_client as async_github_client + from .pagination import PaginationData as PaginationData + from .response import GitHubResponse as GitHubResponse + +# Map of exported name -> submodule (relative to this package) that defines it. +_MODULE_BY_NAME: dict[str, str] = { + 'AsyncGitHubClient': 'client', + 'async_github_client': 'client', + 'GITHUB_API_VERSION': 'client', + 'DEFAULT_BASE_URL': 'client', + 'GitHubResponse': 'response', + 'PaginationData': 'pagination', +} + + +def __getattr__(name: str) -> Any: + try: + module_name = _MODULE_BY_NAME[name] + except KeyError: + raise AttributeError(f'module {__name__!r} has no attribute {name!r}') from None + + import importlib + + module = importlib.import_module(f'.{module_name}', __name__) + value = getattr(module, name) + globals()[name] = value + return value + + +def __dir__() -> list[str]: + return sorted(set(globals()) | _MODULE_BY_NAME.keys()) + + +__all__ = sorted(_MODULE_BY_NAME) diff --git a/ddev/src/ddev/utils/github_async.py b/ddev/src/ddev/utils/github_async/client.py similarity index 68% rename from ddev/src/ddev/utils/github_async.py rename to ddev/src/ddev/utils/github_async/client.py index 279e63b93ae84..b1cca4635d797 100644 --- a/ddev/src/ddev/utils/github_async.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -1,226 +1,30 @@ -"""Async GitHub API client for triggering and monitoring GitHub Actions workflows""" +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Async HTTP client for the GitHub REST API.""" + +from __future__ import annotations -import re from collections.abc import AsyncIterator from contextlib import asynccontextmanager -from dataclasses import dataclass -from typing import Any, Literal, Self +from typing import Any, Literal import httpx -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel + +from .models import ( + ArtifactsList, + IssueComment, + PullRequest, + PullRequestReviewComment, + WorkflowRun, +) +from .pagination import PaginationData +from .response import GitHubResponse GITHUB_API_VERSION = "2022-11-28" DEFAULT_BASE_URL = "https://api.github.com" -_LINK_RE = re.compile(r'<([^>]+)>;\s*rel="([^"]+)"') - - -# --------------------------------------------------------------------------- -# Pagination -# --------------------------------------------------------------------------- - - -@dataclass -class PaginationData: - """Parsed pagination links from a GitHub API Link header.""" - - first: str | None = None - prev: str | None = None - next: str | None = None - last: str | None = None - - @classmethod - def from_header(cls, header: str | None) -> Self: - """Parse a Link header value and return a PaginationData instance.""" - if not header: - return cls() - links: dict[str, str] = {} - for url, rel in _LINK_RE.findall(header): - links[rel] = url - return cls( - first=links.get("first"), - prev=links.get("prev"), - next=links.get("next"), - last=links.get("last"), - ) - - -# --------------------------------------------------------------------------- -# Response and domain models -# --------------------------------------------------------------------------- - - -class GitHubResponse[T](BaseModel): - """Generic wrapper for a GitHub API response.""" - - model_config = ConfigDict(arbitrary_types_allowed=True) - - data: T = Field(...) - headers: dict[str, str] = Field(default_factory=dict) - - -class WorkflowRun(BaseModel): - """A GitHub Actions workflow run.""" - - model_config = ConfigDict(extra="ignore") - - id: int - name: str | None = None - status: str - conclusion: str | None = None - html_url: str | None = None - created_at: str | None = None - updated_at: str | None = None - - -class Artifact(BaseModel): - """A GitHub Actions artifact.""" - - model_config = ConfigDict(extra="ignore") - - id: int - name: str - size_in_bytes: int | None = None - url: str | None = None - archive_download_url: str | None = None - expired: bool - - -class ArtifactsList(BaseModel): - """A list of artifacts with a total count.""" - - model_config = ConfigDict(extra="ignore") - - total_count: int - artifacts: list[Artifact] - - -class IssueComment(BaseModel): - """A GitHub issue (or PR) comment.""" - - model_config = ConfigDict(extra="ignore") - - id: int - body: str - user: dict[str, Any] | None = None - created_at: str | None = None - updated_at: str | None = None - html_url: str | None = None - - -class GitHubUser(BaseModel): - """A GitHub user as returned by the REST API. - - Field reference: - https://docs.github.com/en/rest/users/users#get-a-user - """ - - model_config = ConfigDict(extra="ignore") - - id: int - login: str - html_url: str | None = None - type: str | None = None # 'User', 'Bot', 'Organization', etc. - - -class Label(BaseModel): - """A label attached to an issue or pull request. - - Field reference: - https://docs.github.com/en/rest/issues/labels#get-a-label - """ - - model_config = ConfigDict(extra="ignore") - - id: int - name: str - color: str | None = None - description: str | None = None - - -class PullRequestRef(BaseModel): - """A head or base branch reference on a pull request. - - Field reference (within the `pull-request` object): - https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request - """ - - model_config = ConfigDict(extra="ignore") - - ref: str - sha: str - label: str | None = None # e.g. 'octocat:new-topic' - - -class PullRequest(BaseModel): - """A GitHub pull request. - - Field reference: - https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request - """ - - model_config = ConfigDict(extra="ignore") - - # Identifiers - id: int | None = None - number: int - node_id: str | None = None - - # URLs - url: str | None = None - html_url: str - diff_url: str | None = None - patch_url: str | None = None - - # State - state: str | None = None # 'open' or 'closed' - draft: bool = False - merged: bool | None = None - locked: bool = False - merge_commit_sha: str | None = None - - # Content - title: str | None = None - body: str | None = None - - # People - user: GitHubUser | None = None - assignees: list[GitHubUser] = Field(default_factory=list) - requested_reviewers: list[GitHubUser] = Field(default_factory=list) - - # Labels - labels: list[Label] = Field(default_factory=list) - - # Timestamps (ISO 8601 strings; not parsed into datetime to keep the model lightweight) - created_at: str | None = None - updated_at: str | None = None - closed_at: str | None = None - merged_at: str | None = None - - # Branch references - head: PullRequestRef | None = None - base: PullRequestRef | None = None - - -class PullRequestReviewComment(BaseModel): - """An inline review comment on a pull request diff.""" - - model_config = ConfigDict(extra="ignore") - - id: int - body: str - path: str - commit_id: str - html_url: str | None = None - created_at: str | None = None - updated_at: str | None = None - user: dict[str, Any] | None = None - - -# --------------------------------------------------------------------------- -# Client -# --------------------------------------------------------------------------- - class AsyncGitHubClient: """ @@ -522,18 +326,22 @@ async def create_pr_review_comment( owner: Repository owner (user or organisation). repo: Repository name. pull_number: Pull request number. - body: Markdown body text of the review comment. + body: Markdown body text of the comment. commit_id: SHA of the commit to comment on. - path: Relative path of the file to comment on. - position: Line index in the diff (deprecated but still supported by the API). - line: Line number in the file to comment on (used with `side`). - side: Side of the diff to comment on — ``"LEFT"`` or ``"RIGHT"``. + path: Path of the file to comment on. + position: Line index in the diff (mutually exclusive with line/side). + line: Line number in the file (newer style, paired with side). + side: 'LEFT' or 'RIGHT' (newer style, paired with line). timeout: Optional timeout for this specific request. Defaults to the client's default_timeout. Returns: - GitHubResponse[PullRequestReviewComment]: The validated review comment data and headers. + GitHubResponse[PullRequestReviewComment]: The validated comment data and headers. """ - payload: dict[str, Any] = {"body": body, "commit_id": commit_id, "path": path} + payload: dict[str, Any] = { + "body": body, + "commit_id": commit_id, + "path": path, + } if position is not None: payload["position"] = position if line is not None: @@ -550,7 +358,7 @@ async def create_pr_review_comment( # --------------------------------------------------------------------------- -# Context manager helper +# Async context manager # --------------------------------------------------------------------------- diff --git a/ddev/src/ddev/utils/github_async/models/__init__.py b/ddev/src/ddev/utils/github_async/models/__init__.py new file mode 100644 index 0000000000000..39542752d71cd --- /dev/null +++ b/ddev/src/ddev/utils/github_async/models/__init__.py @@ -0,0 +1,72 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Lazy re-exports for the async GitHub client's domain models. + +Each model class lives in its own submodule (e.g. ``pull_request.py``). The +re-exports below let callers write:: + + from ddev.utils.github_async.models import PullRequest + +without having to know which submodule the class lives in, and without +eagerly importing every submodule when only one is used. + +Mechanism: PEP 562's module-level ``__getattr__`` hook. The first time a +name is accessed, the matching submodule is imported on demand and the +resolved attribute is cached on the package so subsequent accesses are free. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + # Re-exports for type checkers / IDE autocomplete. These imports do not + # execute at runtime (they live behind `TYPE_CHECKING`), so they do not + # break the lazy-loading guarantee. The `X as X` aliases mark these as + # explicit re-exports for linters. + from .comment import IssueComment as IssueComment + from .comment import PullRequestReviewComment as PullRequestReviewComment + from .label import Label as Label + from .pull_request import PullRequest as PullRequest + from .pull_request import PullRequestRef as PullRequestRef + from .user import GitHubUser as GitHubUser + from .workflow import Artifact as Artifact + from .workflow import ArtifactsList as ArtifactsList + from .workflow import WorkflowRun as WorkflowRun + +# Map of exported attribute name -> submodule (relative to this package) that +# defines it. Submodules are imported on demand by `__getattr__`. +_MODULE_BY_NAME: dict[str, str] = { + 'Artifact': 'workflow', + 'ArtifactsList': 'workflow', + 'GitHubUser': 'user', + 'IssueComment': 'comment', + 'Label': 'label', + 'PullRequest': 'pull_request', + 'PullRequestRef': 'pull_request', + 'PullRequestReviewComment': 'comment', + 'WorkflowRun': 'workflow', +} + + +def __getattr__(name: str) -> Any: + try: + module_name = _MODULE_BY_NAME[name] + except KeyError: + raise AttributeError(f'module {__name__!r} has no attribute {name!r}') from None + + import importlib + + module = importlib.import_module(f'.{module_name}', __name__) + value = getattr(module, name) + # Cache so subsequent `from .models import Foo` is a plain dict lookup. + globals()[name] = value + return value + + +def __dir__() -> list[str]: + return sorted(set(globals()) | _MODULE_BY_NAME.keys()) + + +__all__ = sorted(_MODULE_BY_NAME) diff --git a/ddev/src/ddev/utils/github_async/models/comment.py b/ddev/src/ddev/utils/github_async/models/comment.py new file mode 100644 index 0000000000000..06010f08998f0 --- /dev/null +++ b/ddev/src/ddev/utils/github_async/models/comment.py @@ -0,0 +1,38 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Issue and pull-request review comment models.""" + +from __future__ import annotations + +from typing import Any + +from pydantic import BaseModel, ConfigDict + + +class IssueComment(BaseModel): + """A GitHub issue (or PR) comment.""" + + model_config = ConfigDict(extra="ignore") + + id: int + body: str + user: dict[str, Any] | None = None + created_at: str | None = None + updated_at: str | None = None + html_url: str | None = None + + +class PullRequestReviewComment(BaseModel): + """An inline review comment on a pull request diff.""" + + model_config = ConfigDict(extra="ignore") + + id: int + body: str + path: str + commit_id: str + html_url: str | None = None + created_at: str | None = None + updated_at: str | None = None + user: dict[str, Any] | None = None diff --git a/ddev/src/ddev/utils/github_async/models/label.py b/ddev/src/ddev/utils/github_async/models/label.py new file mode 100644 index 0000000000000..3274019fe7d57 --- /dev/null +++ b/ddev/src/ddev/utils/github_async/models/label.py @@ -0,0 +1,23 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""GitHub label model.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict + + +class Label(BaseModel): + """A label attached to an issue or pull request. + + Field reference: + https://docs.github.com/en/rest/issues/labels#get-a-label + """ + + model_config = ConfigDict(extra="ignore") + + id: int + name: str + color: str | None = None + description: str | None = None diff --git a/ddev/src/ddev/utils/github_async/models/pull_request.py b/ddev/src/ddev/utils/github_async/models/pull_request.py new file mode 100644 index 0000000000000..05289c1d55604 --- /dev/null +++ b/ddev/src/ddev/utils/github_async/models/pull_request.py @@ -0,0 +1,75 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Pull request models.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field + +from .label import Label +from .user import GitHubUser + + +class PullRequestRef(BaseModel): + """A head or base branch reference on a pull request. + + Field reference (within the `pull-request` object): + https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request + """ + + model_config = ConfigDict(extra="ignore") + + ref: str + sha: str + label: str | None = None # e.g. 'octocat:new-topic' + + +class PullRequest(BaseModel): + """A GitHub pull request. + + Field reference: + https://docs.github.com/en/rest/pulls/pulls#get-a-pull-request + """ + + model_config = ConfigDict(extra="ignore") + + # Identifiers + id: int | None = None + number: int + node_id: str | None = None + + # URLs + url: str | None = None + html_url: str + diff_url: str | None = None + patch_url: str | None = None + + # State + state: str | None = None # 'open' or 'closed' + draft: bool = False + merged: bool | None = None + locked: bool = False + merge_commit_sha: str | None = None + + # Content + title: str | None = None + body: str | None = None + + # People + user: GitHubUser | None = None + assignees: list[GitHubUser] = Field(default_factory=list) + requested_reviewers: list[GitHubUser] = Field(default_factory=list) + + # Labels + labels: list[Label] = Field(default_factory=list) + + # Timestamps (ISO 8601 strings; not parsed into datetime to keep the model lightweight) + created_at: str | None = None + updated_at: str | None = None + closed_at: str | None = None + merged_at: str | None = None + + # Branch references + head: PullRequestRef | None = None + base: PullRequestRef | None = None diff --git a/ddev/src/ddev/utils/github_async/models/user.py b/ddev/src/ddev/utils/github_async/models/user.py new file mode 100644 index 0000000000000..f41f15e44ee69 --- /dev/null +++ b/ddev/src/ddev/utils/github_async/models/user.py @@ -0,0 +1,23 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""GitHub user model.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict + + +class GitHubUser(BaseModel): + """A GitHub user as returned by the REST API. + + Field reference: + https://docs.github.com/en/rest/users/users#get-a-user + """ + + model_config = ConfigDict(extra="ignore") + + id: int + login: str + html_url: str | None = None + type: str | None = None # 'User', 'Bot', 'Organization', etc. diff --git a/ddev/src/ddev/utils/github_async/models/workflow.py b/ddev/src/ddev/utils/github_async/models/workflow.py new file mode 100644 index 0000000000000..af29b21c141ed --- /dev/null +++ b/ddev/src/ddev/utils/github_async/models/workflow.py @@ -0,0 +1,44 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""GitHub Actions workflow models.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict + + +class WorkflowRun(BaseModel): + """A GitHub Actions workflow run.""" + + model_config = ConfigDict(extra="ignore") + + id: int + name: str | None = None + status: str + conclusion: str | None = None + html_url: str | None = None + created_at: str | None = None + updated_at: str | None = None + + +class Artifact(BaseModel): + """A GitHub Actions artifact.""" + + model_config = ConfigDict(extra="ignore") + + id: int + name: str + size_in_bytes: int | None = None + url: str | None = None + archive_download_url: str | None = None + expired: bool + + +class ArtifactsList(BaseModel): + """A list of artifacts with a total count.""" + + model_config = ConfigDict(extra="ignore") + + total_count: int + artifacts: list[Artifact] diff --git a/ddev/src/ddev/utils/github_async/pagination.py b/ddev/src/ddev/utils/github_async/pagination.py new file mode 100644 index 0000000000000..f70e11c4086c8 --- /dev/null +++ b/ddev/src/ddev/utils/github_async/pagination.py @@ -0,0 +1,37 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Pagination link parsing for the GitHub REST API.""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from typing import Self + +_LINK_RE = re.compile(r'<([^>]+)>;\s*rel="([^"]+)"') + + +@dataclass +class PaginationData: + """Parsed pagination links from a GitHub API Link header.""" + + first: str | None = None + prev: str | None = None + next: str | None = None + last: str | None = None + + @classmethod + def from_header(cls, header: str | None) -> Self: + """Parse a Link header value and return a PaginationData instance.""" + if not header: + return cls() + links: dict[str, str] = {} + for url, rel in _LINK_RE.findall(header): + links[rel] = url + return cls( + first=links.get("first"), + prev=links.get("prev"), + next=links.get("next"), + last=links.get("last"), + ) diff --git a/ddev/src/ddev/utils/github_async/response.py b/ddev/src/ddev/utils/github_async/response.py new file mode 100644 index 0000000000000..0db09366a973e --- /dev/null +++ b/ddev/src/ddev/utils/github_async/response.py @@ -0,0 +1,17 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Generic response wrapper for the async GitHub client.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field + + +class GitHubResponse[T](BaseModel): + """Generic wrapper for a GitHub API response.""" + + model_config = ConfigDict(arbitrary_types_allowed=True) + + data: T = Field(...) + headers: dict[str, str] = Field(default_factory=dict) diff --git a/ddev/tests/cli/release/test_port_commit.py b/ddev/tests/cli/release/test_port_commit.py index e8d925fe2e157..3a274513f0389 100644 --- a/ddev/tests/cli/release/test_port_commit.py +++ b/ddev/tests/cli/release/test_port_commit.py @@ -20,7 +20,8 @@ split_commit_subject, ) from ddev.utils.git import GitCommit -from ddev.utils.github_async import GitHubResponse, PullRequest +from ddev.utils.github_async import GitHubResponse +from ddev.utils.github_async.models import PullRequest from tests.helpers.github_async import FakeAsyncGitHubClient from tests.helpers.runner import CliRunner diff --git a/ddev/tests/helpers/github_async.py b/ddev/tests/helpers/github_async.py index ecf0003b3b962..d4012a1311a46 100644 --- a/ddev/tests/helpers/github_async.py +++ b/ddev/tests/helpers/github_async.py @@ -26,7 +26,8 @@ def test_thing(fake_async_github): import pytest from pytest_mock import MockerFixture -from ddev.utils.github_async import GitHubResponse, PullRequest +from ddev.utils.github_async import GitHubResponse +from ddev.utils.github_async.models import PullRequest @dataclass diff --git a/ddev/tests/utils/test_github_async.py b/ddev/tests/utils/test_github_async.py index dbd71a079bed6..f49459dc75c7e 100644 --- a/ddev/tests/utils/test_github_async.py +++ b/ddev/tests/utils/test_github_async.py @@ -11,18 +11,20 @@ from ddev.utils.github_async import ( GITHUB_API_VERSION, - ArtifactsList, AsyncGitHubClient, GitHubResponse, + PaginationData, + async_github_client, +) +from ddev.utils.github_async.models import ( + ArtifactsList, GitHubUser, IssueComment, Label, - PaginationData, PullRequest, PullRequestRef, PullRequestReviewComment, WorkflowRun, - async_github_client, ) # --------------------------------------------------------------------------- @@ -593,6 +595,48 @@ async def test_add_labels_to_issue_http_error_raises() -> None: assert exc_info.value.response.status_code == 404 +# --------------------------------------------------------------------------- +# Lazy-loading guarantees for the `models` subpackage +# --------------------------------------------------------------------------- + + +def test_models_subpackage_loads_only_requested_submodule() -> None: + """Importing one model must not eagerly load every other model submodule. + + Runs each scenario in a clean subprocess so the import effect is observable + (the parent test process has already loaded everything for other tests). + """ + import subprocess + import sys + import textwrap + + script = textwrap.dedent( + """ + import sys + from ddev.utils.github_async.models import PullRequest # noqa: F401 + + prefix = 'ddev.utils.github_async.models.' + loaded = sorted(name[len(prefix):] for name in sys.modules if name.startswith(prefix)) + print(','.join(loaded)) + """ + ) + result = subprocess.run([sys.executable, '-c', script], capture_output=True, text=True, check=True) + loaded = set(result.stdout.strip().split(',')) + + # `pull_request` and its two type dependencies (`user`, `label`) must load. + assert {'pull_request', 'user', 'label'} <= loaded + # Unrelated model submodules must stay unloaded. + assert 'workflow' not in loaded + assert 'comment' not in loaded + + +def test_models_subpackage_unknown_attribute_raises_attribute_error() -> None: + import ddev.utils.github_async.models as models + + with pytest.raises(AttributeError, match='no attribute'): + models.NotARealModel # noqa: B018 + + # --------------------------------------------------------------------------- # Custom timeout per request # --------------------------------------------------------------------------- From 26da75af024d8f923b23c7a9c150f252bdbb4b04 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Wed, 13 May 2026 09:15:23 +0200 Subject: [PATCH 09/11] Add mock_response API to FakeAsyncGitHubClient --- ddev/tests/cli/release/test_port_commit.py | 13 +- ddev/tests/helpers/github_async.py | 164 +++++++++++++++++--- ddev/tests/helpers/test_github_async.py | 169 +++++++++++++++++++++ 3 files changed, 321 insertions(+), 25 deletions(-) create mode 100644 ddev/tests/helpers/test_github_async.py diff --git a/ddev/tests/cli/release/test_port_commit.py b/ddev/tests/cli/release/test_port_commit.py index 3a274513f0389..2a73876103e94 100644 --- a/ddev/tests/cli/release/test_port_commit.py +++ b/ddev/tests/cli/release/test_port_commit.py @@ -20,7 +20,6 @@ split_commit_subject, ) from ddev.utils.git import GitCommit -from ddev.utils.github_async import GitHubResponse from ddev.utils.github_async.models import PullRequest from tests.helpers.github_async import FakeAsyncGitHubClient from tests.helpers.runner import CliRunner @@ -219,9 +218,9 @@ def test_commit_step(app_mock, verify, expected_args): def test_create_pull_request_step(app_mock: MagicMock, fake_async_github: FakeAsyncGitHubClient) -> None: app_mock.config.github.token = 'ghp_test' - fake_async_github.pull_request_response = GitHubResponse[PullRequest]( - data=PullRequest(number=7, html_url='https://github.com/x/pr/1'), - headers={}, + fake_async_github.mock_response( + 'create_pull_request', + PullRequest(number=7, html_url='https://github.com/x/pr/1'), ) step = CreatePullRequestStep( @@ -307,9 +306,9 @@ def _setup_command_mocks(mocker, *, commit_sha='deadbeef0011223344', subject='Fi def test_command_happy_path(ddev: CliRunner, mocker: MockerFixture, fake_async_github: FakeAsyncGitHubClient) -> None: _setup_command_mocks(mocker) - fake_async_github.pull_request_response = GitHubResponse[PullRequest]( - data=PullRequest(number=1, html_url='https://github.com/x/pr/1'), - headers={}, + fake_async_github.mock_response( + 'create_pull_request', + PullRequest(number=1, html_url='https://github.com/x/pr/1'), ) mocker.patch('click.confirm', return_value=True) mocker.patch.dict('os.environ', {'DD_GITHUB_USER': 'alice'}) diff --git a/ddev/tests/helpers/github_async.py b/ddev/tests/helpers/github_async.py index d4012a1311a46..99817e5934f46 100644 --- a/ddev/tests/helpers/github_async.py +++ b/ddev/tests/helpers/github_async.py @@ -3,22 +3,38 @@ # Licensed under a 3-clause BSD style license (see LICENSE) """Test helpers for the async GitHub client. -Provides a `FakeAsyncGitHubClient` that records every call and exposes -assertion helpers, plus a `fake_async_github` pytest fixture that patches -`async_github_client` to yield the fake and sets a stub token. +Provides a `FakeAsyncGitHubClient` that records every call and lets tests +register canned responses with `mock_response`, plus a `fake_async_github` +pytest fixture that patches `async_github_client` to yield the fake. -Usage: +Quick reference: def test_thing(fake_async_github): - do_something_that_creates_a_pr() - fake_async_github.assert_called( - 'create_pull_request', title='[Backport] Fix bug', draft=False + # Sticky default for all matching calls + fake_async_github.mock_response( + 'create_pull_request', + PullRequest(number=5, html_url='https://github.com/x/pr/5'), + ) + + # Partial match: only PR #5 gets the override + fake_async_github.mock_response( + 'add_labels_to_issue', + httpx.HTTPStatusError(...), + issue_number=5, ) + + # FIFO queue: first matching call raises, second succeeds + fake_async_github.mock_response('create_pull_request', err, once=True) + fake_async_github.mock_response('create_pull_request', pr_response, once=True) + + do_thing_under_test() + fake_async_github.assert_called_with('create_pull_request', ...) + fake_async_github.assert_all_responses_consumed() """ from __future__ import annotations -from collections.abc import AsyncIterator +from collections.abc import AsyncIterator, Callable from contextlib import asynccontextmanager from dataclasses import dataclass, field from typing import Any @@ -38,18 +54,79 @@ class RecordedRequest: kwargs: dict[str, Any] = field(default_factory=dict) +@dataclass +class _MockEntry: + """A single registered response, either sticky or one-shot.""" + + response: Any + match_kwargs: dict[str, Any] + once: bool + + +def _default_response_factories() -> dict[str, Callable[[], Any]]: + """Built-in default responses used when no `mock_response` matches a call.""" + return { + 'create_pull_request': lambda: GitHubResponse( + data=PullRequest(number=1, html_url='https://github.com/test/repo/pull/1'), + headers={}, + ), + 'add_labels_to_issue': lambda: GitHubResponse.model_validate({'data': None, 'headers': {}}), + } + + class FakeAsyncGitHubClient: - """Test double for AsyncGitHubClient that records calls and returns canned responses. + """Test double for AsyncGitHubClient that records calls and serves canned responses. - Override response shape by assigning to `pull_request_response` before the call occurs. + Mock responses are registered via `mock_response`. Each call to a mirrored API method + consults, in order: + + 1. The one-shot queue for that method (FIFO, first match wins, consumed on use). + 2. The sticky-mock list for that method (most-recent registration wins). + 3. A built-in default response (see `_default_response_factories`). + + Exceptions registered as responses are raised. Plain data values are auto-wrapped in + `GitHubResponse(data=value, headers={})`. Full `GitHubResponse` instances pass through. """ def __init__(self) -> None: self.requests: list[RecordedRequest] = [] - self.pull_request_response: GitHubResponse[PullRequest] = GitHubResponse[PullRequest]( - data=PullRequest(number=1, html_url='https://github.com/test/repo/pull/1'), - headers={}, - ) + self._oneshot_mocks: dict[str, list[_MockEntry]] = {} + self._sticky_mocks: dict[str, list[_MockEntry]] = {} + self._default_response_factories: dict[str, Callable[[], Any]] = _default_response_factories() + + # ------------------------------------------------------------------ + # Mock registration + # ------------------------------------------------------------------ + + def mock_response( + self, + method: str, + response: Any, + /, + *, + once: bool = False, + **match_kwargs: Any, + ) -> None: + """Register *response* to be returned by *method*. + + Args: + method: Name of the client method to stub (e.g. ``'create_pull_request'``). + response: What to return. Behavior depends on its type: + - ``BaseException`` instance -> raised when the call is made. + - ``GitHubResponse`` instance -> returned as-is. + - Anything else (including ``None``) -> wrapped in + ``GitHubResponse(data=response, headers={})``. + once: When True, this response is consumed by the first matching call + (FIFO across all one-shots registered for the method). Otherwise the + response is sticky and fires on every matching call until something + more specific is registered. + **match_kwargs: Optional key/value pairs that the call's recorded kwargs + must contain (partial match). With no kwargs, the response matches any + call to *method*. + """ + entry = _MockEntry(response=response, match_kwargs=match_kwargs, once=once) + bucket = self._oneshot_mocks if once else self._sticky_mocks + bucket.setdefault(method, []).append(entry) # ------------------------------------------------------------------ # Mirrored API surface @@ -66,7 +143,7 @@ async def create_pull_request( draft: bool = False, timeout: float | None = None, ) -> GitHubResponse[PullRequest]: - self._record( + return self._call( 'create_pull_request', owner=owner, repo=repo, @@ -76,7 +153,6 @@ async def create_pull_request( body=body, draft=draft, ) - return self.pull_request_response async def add_labels_to_issue( self, @@ -86,8 +162,13 @@ async def add_labels_to_issue( labels: list[str], timeout: float | None = None, ) -> GitHubResponse[None]: - self._record('add_labels_to_issue', owner=owner, repo=repo, issue_number=issue_number, labels=labels) - return GitHubResponse[None].model_validate({'data': None, 'headers': {}}) + return self._call( + 'add_labels_to_issue', + owner=owner, + repo=repo, + issue_number=issue_number, + labels=labels, + ) async def aclose(self) -> None: return None @@ -145,6 +226,17 @@ def assert_not_called(self, method: str) -> None: if calls: raise AssertionError(f'Expected no calls to {method!r}, but got: {calls}') + def assert_all_responses_consumed(self) -> None: + """Assert every one-shot mock registered has been consumed by a call. + + Use in tests that depend on a queued sequence firing (e.g. retry logic). Sticky + mocks are not affected; only one-shots are tracked. + """ + pending = {method: queue for method, queue in self._oneshot_mocks.items() if queue} + if pending: + details = '; '.join(f'{method}: {len(queue)} remaining' for method, queue in pending.items()) + raise AssertionError(f'One-shot responses were not consumed -> {details}') + # ------------------------------------------------------------------ # Internals # ------------------------------------------------------------------ @@ -152,6 +244,41 @@ def assert_not_called(self, method: str) -> None: def _record(self, method: str, **kwargs: Any) -> None: self.requests.append(RecordedRequest(method=method, kwargs=kwargs)) + def _call(self, method: str, **call_kwargs: Any) -> Any: + self._record(method, **call_kwargs) + response = self._resolve_response(method, call_kwargs) + if isinstance(response, BaseException): + raise response + if isinstance(response, GitHubResponse): + return response + return GitHubResponse.model_validate({'data': response, 'headers': {}}) + + def _resolve_response(self, method: str, call_kwargs: dict[str, Any]) -> Any: + # 1. One-shot queue: FIFO, first match wins, consumed. + queue = self._oneshot_mocks.get(method, []) + for i, entry in enumerate(queue): + if self._matches(call_kwargs, entry.match_kwargs): + queue.pop(i) + return entry.response + + # 2. Sticky mocks: most-recent registration wins. + for entry in reversed(self._sticky_mocks.get(method, [])): + if self._matches(call_kwargs, entry.match_kwargs): + return entry.response + + # 3. Built-in default for this method. + factory = self._default_response_factories.get(method) + if factory is None: + raise AssertionError( + f'No mock registered for {method!r} and no built-in default. ' + f'Call fake_async_github.mock_response({method!r}, ...) in your test.' + ) + return factory() + + @staticmethod + def _matches(call_kwargs: dict[str, Any], match_kwargs: dict[str, Any]) -> bool: + return all(call_kwargs.get(k) == v for k, v in match_kwargs.items()) + @pytest.fixture def fake_async_github(mocker: MockerFixture) -> FakeAsyncGitHubClient: @@ -166,6 +293,7 @@ async def fake_context( ) -> AsyncIterator[FakeAsyncGitHubClient]: yield fake + mocker.patch('ddev.utils.github_async.client.async_github_client', fake_context) mocker.patch('ddev.utils.github_async.async_github_client', fake_context) mocker.patch.dict('os.environ', {'DD_GITHUB_TOKEN': 'ghp_test'}) return fake diff --git a/ddev/tests/helpers/test_github_async.py b/ddev/tests/helpers/test_github_async.py new file mode 100644 index 0000000000000..6e8d8240adaf9 --- /dev/null +++ b/ddev/tests/helpers/test_github_async.py @@ -0,0 +1,169 @@ +# (C) Datadog, Inc. 2026-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +"""Tests for the FakeAsyncGitHubClient helper itself.""" + +from __future__ import annotations + +import httpx +import pytest + +from ddev.utils.github_async import GitHubResponse +from ddev.utils.github_async.models import PullRequest +from tests.helpers.github_async import FakeAsyncGitHubClient + + +@pytest.fixture +def fake() -> FakeAsyncGitHubClient: + return FakeAsyncGitHubClient() + + +@pytest.mark.asyncio +async def test_default_response_used_when_no_mock_registered(fake: FakeAsyncGitHubClient) -> None: + response = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + assert isinstance(response.data, PullRequest) + assert response.data.number == 1 + + +@pytest.mark.asyncio +async def test_unknown_method_without_default_raises(fake: FakeAsyncGitHubClient) -> None: + fake._default_response_factories.pop('add_labels_to_issue') + with pytest.raises(AssertionError, match='No mock registered'): + await fake.add_labels_to_issue('o', 'r', 1, ['bug']) + + +@pytest.mark.asyncio +async def test_sticky_mock_with_inner_data_is_auto_wrapped(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response('create_pull_request', PullRequest(number=42, html_url='https://x/42')) + + response = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + assert isinstance(response, GitHubResponse) + assert response.data.number == 42 + + +@pytest.mark.asyncio +async def test_sticky_mock_with_full_response_passes_through(fake: FakeAsyncGitHubClient) -> None: + full = GitHubResponse.model_validate( + {'data': PullRequest(number=99, html_url='https://x/99'), 'headers': {'x-rate-limit': '5'}} + ) + fake.mock_response('create_pull_request', full) + + response = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + assert response is full + assert response.headers['x-rate-limit'] == '5' + + +@pytest.mark.asyncio +async def test_sticky_mock_partial_match_only_fires_for_matching_call(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response('create_pull_request', PullRequest(number=7, html_url='https://x/7'), draft=True) + + # Default fires for non-matching calls. + default = await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=False) + assert default.data.number == 1 + + # Mock fires for matching calls. + matched = await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=True) + assert matched.data.number == 7 + + +@pytest.mark.asyncio +async def test_most_recent_sticky_mock_wins(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response('create_pull_request', PullRequest(number=1, html_url='https://x/1')) + fake.mock_response('create_pull_request', PullRequest(number=2, html_url='https://x/2')) + + response = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + assert response.data.number == 2 + + +@pytest.mark.asyncio +async def test_exception_response_raises(fake: FakeAsyncGitHubClient) -> None: + err = httpx.HTTPStatusError('boom', request=httpx.Request('POST', 'https://x'), response=httpx.Response(422)) + fake.mock_response('create_pull_request', err) + + with pytest.raises(httpx.HTTPStatusError): + await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + +@pytest.mark.asyncio +async def test_one_shot_consumed_then_falls_through_to_sticky(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response('create_pull_request', PullRequest(number=10, html_url='https://x/10'), once=True) + fake.mock_response('create_pull_request', PullRequest(number=99, html_url='https://x/99')) # sticky + + first = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + second = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + third = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + assert first.data.number == 10 # one-shot + assert second.data.number == 99 # sticky + assert third.data.number == 99 # sticky + + +@pytest.mark.asyncio +async def test_multiple_one_shots_fire_in_registration_order(fake: FakeAsyncGitHubClient) -> None: + """The retry pattern: first call errors, second succeeds.""" + err = httpx.HTTPStatusError('try again', request=httpx.Request('POST', 'https://x'), response=httpx.Response(500)) + fake.mock_response('create_pull_request', err, once=True) + fake.mock_response('create_pull_request', PullRequest(number=5, html_url='https://x/5'), once=True) + + with pytest.raises(httpx.HTTPStatusError): + await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + response = await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + assert response.data.number == 5 + + +@pytest.mark.asyncio +async def test_one_shot_with_match_kwargs_only_consumed_when_match(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response( + 'create_pull_request', + PullRequest(number=7, html_url='https://x/7'), + once=True, + draft=True, + ) + + # Non-matching call ignores the one-shot, hits built-in default. + non_match = await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=False) + assert non_match.data.number == 1 + + # Matching call consumes the one-shot. + match = await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=True) + assert match.data.number == 7 + + # Second matching call: one-shot is gone, falls through to default. + next_match = await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=True) + assert next_match.data.number == 1 + + +@pytest.mark.asyncio +async def test_assert_all_responses_consumed_passes_when_empty(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response('create_pull_request', PullRequest(number=5, html_url='https://x/5'), once=True) + await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + fake.assert_all_responses_consumed() # must not raise + + +@pytest.mark.asyncio +async def test_assert_all_responses_consumed_fails_with_pending(fake: FakeAsyncGitHubClient) -> None: + fake.mock_response('create_pull_request', PullRequest(number=1, html_url='https://x/1'), once=True) + fake.mock_response('create_pull_request', PullRequest(number=2, html_url='https://x/2'), once=True) + + await fake.create_pull_request('o', 'r', 'T', 'h', 'b') + + with pytest.raises(AssertionError, match='not consumed'): + fake.assert_all_responses_consumed() + + +@pytest.mark.asyncio +async def test_calls_are_recorded_regardless_of_response(fake: FakeAsyncGitHubClient) -> None: + err = httpx.HTTPStatusError('boom', request=httpx.Request('POST', 'https://x'), response=httpx.Response(500)) + fake.mock_response('create_pull_request', err, once=True) + fake.mock_response('create_pull_request', PullRequest(number=5, html_url='https://x/5')) + + with pytest.raises(httpx.HTTPStatusError): + await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=True) + await fake.create_pull_request('o', 'r', 'T', 'h', 'b', draft=False) + + assert len(fake.calls_to('create_pull_request')) == 2 From af677451f615c44d7ea599551e829643e6d64754 Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Wed, 13 May 2026 09:21:45 +0200 Subject: [PATCH 10/11] Inline pagination + response wrappers into client module --- ddev/src/ddev/utils/github_async/__init__.py | 8 +-- ddev/src/ddev/utils/github_async/client.py | 49 +++++++++++++++++-- .../src/ddev/utils/github_async/pagination.py | 37 -------------- ddev/src/ddev/utils/github_async/response.py | 17 ------- 4 files changed, 49 insertions(+), 62 deletions(-) delete mode 100644 ddev/src/ddev/utils/github_async/pagination.py delete mode 100644 ddev/src/ddev/utils/github_async/response.py diff --git a/ddev/src/ddev/utils/github_async/__init__.py b/ddev/src/ddev/utils/github_async/__init__.py index f3137ea0d283a..85ea733ff3c0c 100644 --- a/ddev/src/ddev/utils/github_async/__init__.py +++ b/ddev/src/ddev/utils/github_async/__init__.py @@ -28,9 +28,9 @@ from .client import DEFAULT_BASE_URL as DEFAULT_BASE_URL from .client import GITHUB_API_VERSION as GITHUB_API_VERSION from .client import AsyncGitHubClient as AsyncGitHubClient + from .client import GitHubResponse as GitHubResponse + from .client import PaginationData as PaginationData from .client import async_github_client as async_github_client - from .pagination import PaginationData as PaginationData - from .response import GitHubResponse as GitHubResponse # Map of exported name -> submodule (relative to this package) that defines it. _MODULE_BY_NAME: dict[str, str] = { @@ -38,8 +38,8 @@ 'async_github_client': 'client', 'GITHUB_API_VERSION': 'client', 'DEFAULT_BASE_URL': 'client', - 'GitHubResponse': 'response', - 'PaginationData': 'pagination', + 'GitHubResponse': 'client', + 'PaginationData': 'client', } diff --git a/ddev/src/ddev/utils/github_async/client.py b/ddev/src/ddev/utils/github_async/client.py index b1cca4635d797..6b3fc4a9da595 100644 --- a/ddev/src/ddev/utils/github_async/client.py +++ b/ddev/src/ddev/utils/github_async/client.py @@ -5,12 +5,14 @@ from __future__ import annotations +import re from collections.abc import AsyncIterator from contextlib import asynccontextmanager -from typing import Any, Literal +from dataclasses import dataclass +from typing import Any, Literal, Self import httpx -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict, Field from .models import ( ArtifactsList, @@ -19,12 +21,51 @@ PullRequestReviewComment, WorkflowRun, ) -from .pagination import PaginationData -from .response import GitHubResponse GITHUB_API_VERSION = "2022-11-28" DEFAULT_BASE_URL = "https://api.github.com" +_LINK_RE = re.compile(r'<([^>]+)>;\s*rel="([^"]+)"') + + +# --------------------------------------------------------------------------- +# Pagination + response wrappers +# --------------------------------------------------------------------------- + + +@dataclass +class PaginationData: + """Parsed pagination links from a GitHub API Link header.""" + + first: str | None = None + prev: str | None = None + next: str | None = None + last: str | None = None + + @classmethod + def from_header(cls, header: str | None) -> Self: + """Parse a Link header value and return a PaginationData instance.""" + if not header: + return cls() + links: dict[str, str] = {} + for url, rel in _LINK_RE.findall(header): + links[rel] = url + return cls( + first=links.get("first"), + prev=links.get("prev"), + next=links.get("next"), + last=links.get("last"), + ) + + +class GitHubResponse[T](BaseModel): + """Generic wrapper for a GitHub API response.""" + + model_config = ConfigDict(arbitrary_types_allowed=True) + + data: T = Field(...) + headers: dict[str, str] = Field(default_factory=dict) + class AsyncGitHubClient: """ diff --git a/ddev/src/ddev/utils/github_async/pagination.py b/ddev/src/ddev/utils/github_async/pagination.py deleted file mode 100644 index f70e11c4086c8..0000000000000 --- a/ddev/src/ddev/utils/github_async/pagination.py +++ /dev/null @@ -1,37 +0,0 @@ -# (C) Datadog, Inc. 2026-present -# All rights reserved -# Licensed under a 3-clause BSD style license (see LICENSE) -"""Pagination link parsing for the GitHub REST API.""" - -from __future__ import annotations - -import re -from dataclasses import dataclass -from typing import Self - -_LINK_RE = re.compile(r'<([^>]+)>;\s*rel="([^"]+)"') - - -@dataclass -class PaginationData: - """Parsed pagination links from a GitHub API Link header.""" - - first: str | None = None - prev: str | None = None - next: str | None = None - last: str | None = None - - @classmethod - def from_header(cls, header: str | None) -> Self: - """Parse a Link header value and return a PaginationData instance.""" - if not header: - return cls() - links: dict[str, str] = {} - for url, rel in _LINK_RE.findall(header): - links[rel] = url - return cls( - first=links.get("first"), - prev=links.get("prev"), - next=links.get("next"), - last=links.get("last"), - ) diff --git a/ddev/src/ddev/utils/github_async/response.py b/ddev/src/ddev/utils/github_async/response.py deleted file mode 100644 index 0db09366a973e..0000000000000 --- a/ddev/src/ddev/utils/github_async/response.py +++ /dev/null @@ -1,17 +0,0 @@ -# (C) Datadog, Inc. 2026-present -# All rights reserved -# Licensed under a 3-clause BSD style license (see LICENSE) -"""Generic response wrapper for the async GitHub client.""" - -from __future__ import annotations - -from pydantic import BaseModel, ConfigDict, Field - - -class GitHubResponse[T](BaseModel): - """Generic wrapper for a GitHub API response.""" - - model_config = ConfigDict(arbitrary_types_allowed=True) - - data: T = Field(...) - headers: dict[str, str] = Field(default_factory=dict) From d87567eadb3397f9852ae8feba51f49bc7bcc1fb Mon Sep 17 00:00:00 2001 From: Juanpe Araque Date: Wed, 13 May 2026 09:24:32 +0200 Subject: [PATCH 11/11] Move FakeAsyncGitHubClient tests to tests/utils/ alongside real-client tests --- .../test_github_async.py => utils/test_fake_github_async.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename ddev/tests/{helpers/test_github_async.py => utils/test_fake_github_async.py} (100%) diff --git a/ddev/tests/helpers/test_github_async.py b/ddev/tests/utils/test_fake_github_async.py similarity index 100% rename from ddev/tests/helpers/test_github_async.py rename to ddev/tests/utils/test_fake_github_async.py