diff --git a/otterdog/operations/approve_blueprints.py b/otterdog/operations/approve_blueprints.py index fbe9b3b9..647e9783 100644 --- a/otterdog/operations/approve_blueprints.py +++ b/otterdog/operations/approve_blueprints.py @@ -106,8 +106,6 @@ async def execute( return 1 async with GitHubProvider(credentials) as provider: - rest_api = provider.rest_api - for blueprint in blueprints: self.printer.print(f"Merging PR #{blueprint.remediation_pr}: ") @@ -123,9 +121,12 @@ async def execute( if repo.allow_rebase_merge is True: merge_method = "rebase" - result = await rest_api.pull_request.merge_pull_request( - blueprint.id.org_id, blueprint.id.repo_name, f"{blueprint.remediation_pr}", merge_method + pr = provider.pull_request( + blueprint.id.org_id, + blueprint.id.repo_name, + blueprint.remediation_pr, ) + result = await pr.merge_pull_request(merge_method) if result["merged"] is True: self.printer.println("[green]merged[/].") diff --git a/otterdog/operations/open_pull_request.py b/otterdog/operations/open_pull_request.py index 7e1942fb..7cd275a0 100644 --- a/otterdog/operations/open_pull_request.py +++ b/otterdog/operations/open_pull_request.py @@ -21,7 +21,6 @@ if TYPE_CHECKING: from otterdog.config import OrganizationConfig - from otterdog.providers.github.rest import RestApi class OpenPullRequestOperation(Operation): @@ -128,7 +127,7 @@ async def execute( return 1 pr_number = await self._create_pull_request( - rest_api, + provider, org_config, default_branch, local_configuration, @@ -150,11 +149,13 @@ async def execute( async def _create_pull_request( self, - rest_api: RestApi, + github: GitHubProvider, org_config: OrganizationConfig, default_branch: str, local_configuration: str, - ) -> str: + ) -> int: + rest_api = github.rest_api + default_branch_data = await rest_api.reference.get_branch_reference( org_config.github_id, org_config.config_repo, @@ -183,7 +184,7 @@ async def _create_pull_request( else: body = "This PR has been created automatically using the otterdog cli." - pull_request_data = await rest_api.pull_request.create_pull_request( + pull_request_data = await github.create_pull_request( org_config.github_id, org_config.config_repo, self.title, @@ -192,4 +193,4 @@ async def _create_pull_request( body, ) - return pull_request_data["number"] + return pull_request_data.pr_number diff --git a/otterdog/providers/github/__init__.py b/otterdog/providers/github/__init__.py index ee2c8555..5977d9db 100644 --- a/otterdog/providers/github/__init__.py +++ b/otterdog/providers/github/__init__.py @@ -16,6 +16,8 @@ from importlib_resources import files from otterdog import resources +from otterdog.providers.github.exception import GitHubException +from otterdog.providers.github.pull_request import PullRequest from otterdog.utils import get_logger, is_ghsa_repo, is_set_and_present if TYPE_CHECKING: @@ -47,6 +49,59 @@ def __init__(self, credentials: Credentials | None): if credentials is not None: self._init_clients() + def pull_request(self, org_id: str, repo_name: str, pr_number: int) -> PullRequest: + return PullRequest(self.rest_api.requester, org_id, repo_name, pr_number) + + async def create_pull_request( + self, + org_id: str, + repo_name: str, + title: str, + head: str, + base: str, + body: str | None = None, + ) -> PullRequest: + _logger.debug("creating pull request for repo '%s/%s'", org_id, repo_name) + + try: + data = { + "title": title, + "head": head, + "base": base, + } + + if body is not None: + data["body"] = body + + pr_data = await self.rest_api.requester.request_json( + "POST", f"/repos/{org_id}/{repo_name}/pulls", data=data + ) + return PullRequest(self.rest_api.requester, org_id, repo_name, pr_data["number"]) + except GitHubException as ex: + raise RuntimeError(f"failed creating pull request:\n{ex}") from ex + + # TODO: this should return precached PullRequest objects instead of raw data dicts. + async def get_pull_requests( + self, + org_id: str, + repo_name: str, + state: str = "all", + base_ref: str | None = None, + ) -> list[dict[str, Any]]: + _logger.debug("getting pull requests from repo '%s/%s'", org_id, repo_name) + + try: + params = {"state": state} + + if base_ref is not None: + params.update({"base": base_ref}) + + return await self.rest_api.requester.request_paged_json( + "GET", f"/repos/{org_id}/{repo_name}/pulls", params=params + ) + except GitHubException as ex: + raise RuntimeError(f"failed retrieving pull requests:\n{ex}") from ex + async def __aenter__(self): return self diff --git a/otterdog/providers/github/pull_request.py b/otterdog/providers/github/pull_request.py new file mode 100644 index 00000000..2b2e2d58 --- /dev/null +++ b/otterdog/providers/github/pull_request.py @@ -0,0 +1,153 @@ +# ******************************************************************************* +# Copyright (c) 2024-2025 Eclipse Foundation and others. +# This program and the accompanying materials are made available +# under the terms of the Eclipse Public License 2.0 +# which is available at http://www.eclipse.org/legal/epl-v20.html +# SPDX-License-Identifier: EPL-2.0 +# ******************************************************************************* +import json +from typing import Any + +from otterdog.logging import get_logger +from otterdog.providers.github.exception import GitHubException +from otterdog.providers.github.rest.requester import Requester + +_logger = get_logger(__name__) + + +class PullRequest: + def __init__(self, rest_api_requester: Requester, org_id: str, repo_name: str, pr_number: int) -> None: + self.requester = rest_api_requester + self.org_id = org_id + self.repo_name = repo_name + self.pr_number = pr_number + + def __str__(self) -> str: + return f"PullRequest(org_id={self.org_id}, repo_name={self.repo_name}, pr_number={self.pr_number})" + + def _base_path(self) -> str: + return f"/repos/{self.org_id}/{self.repo_name}/pulls/{self.pr_number}" + + async def get_data( + self, + ) -> dict[str, Any]: + _logger.debug("getting live data for %s", self) + + try: + return await self.requester.request_json("GET", self._base_path()) + except GitHubException as ex: + raise RuntimeError(f"failed retrieving {self}") from ex + + async def merge_pull_request( + self, + method: str, + ) -> dict[str, Any]: + _logger.debug("merging %s", self) + + try: + data = {"merge_method": method} + return await self.requester.request_json("PUT", self._base_path() + "/merge", data=data) + except GitHubException as ex: + raise RuntimeError(f"failed merging {self}") from ex + + async def get_commits( + self, + ) -> list[dict[str, Any]]: + _logger.debug("getting commits for %s", self) + + try: + return await self.requester.request_paged_json("GET", self._base_path() + "/commits") + except GitHubException as ex: + raise RuntimeError(f"failed retrieving commits for {self}") from ex + + async def get_reviews( + self, + ) -> list[dict[str, Any]]: + _logger.debug("getting reviews for %s", self) + + try: + return await self.requester.request_paged_json("GET", self._base_path() + "/reviews") + except GitHubException as ex: + raise RuntimeError(f"failed retrieving reviews for {self}") from ex + + async def request_reviews( + self, + reviewers: list[str] | None = None, + team_reviewers: list[str] | None = None, + ) -> bool: + _logger.debug( + "requesting reviews for %s: %s, %s", + self, + reviewers, + team_reviewers, + ) + + if (reviewers is None or len(reviewers) == 0) and (team_reviewers is None or len(team_reviewers) == 0): + _logger.error("requesting reviews for %s without any reviewer specified", self) + return False + + try: + data = {} + + if reviewers is not None: + data["reviewers"] = reviewers + + if team_reviewers is not None: + data["team_reviewers"] = team_reviewers + + status, body = await self.requester.request_raw( + "POST", + self._base_path() + "/requested_reviewers", + data=json.dumps(data), + ) + + if status == 201: + return True + elif status == 422: + _logger.warning("failed to request reviews for %s: %s", self, body) + return False + else: + raise RuntimeError(f"failed requesting reviews for {self}\n{status}: {body}") + + except GitHubException as ex: + raise RuntimeError(f"failed requesting reviews for {self}") from ex + + async def get_files( + self, + ) -> list[dict[str, Any]]: + _logger.debug("getting files for %s", self) + + try: + return await self.requester.request_paged_json("GET", self._base_path() + "/files") + except GitHubException as ex: + raise RuntimeError(f"failed retrieving files for {self}") from ex + + async def merge( + self, + commit_message: str | None = None, + merge_method: str = "squash", + ) -> bool: + """ + @param commit_message is only used for "squash" and "merge" merge methods, and ignored for "rebase" method + @param merge_method can be one of "merge", "squash", or "rebase" + """ + # https://docs.github.com/en/enterprise-cloud@latest/rest/pulls/pulls?apiVersion=2022-11-28#merge-a-pull-request + + _logger.debug("merging %s", self) + + try: + data = { + "merge_method": merge_method, + } + + if commit_message is not None: + data.update({"commit_message": commit_message}) + + response = await self.requester.request_json( + "PUT", + self._base_path() + "/merge", + data=data, + ) + return response["merged"] + except GitHubException as ex: + raise RuntimeError(f"failed merging {self}") from ex diff --git a/otterdog/providers/github/rest/__init__.py b/otterdog/providers/github/rest/__init__.py index 2bddd162..12c5d191 100644 --- a/otterdog/providers/github/rest/__init__.py +++ b/otterdog/providers/github/rest/__init__.py @@ -94,12 +94,6 @@ def issue(self): return IssueClient(self) - @cached_property - def pull_request(self): - from .pull_request_client import PullRequestClient - - return PullRequestClient(self) - @cached_property def reference(self): from .reference_client import ReferenceClient diff --git a/otterdog/providers/github/rest/pull_request_client.py b/otterdog/providers/github/rest/pull_request_client.py deleted file mode 100644 index 50aa31a2..00000000 --- a/otterdog/providers/github/rest/pull_request_client.py +++ /dev/null @@ -1,222 +0,0 @@ -# ******************************************************************************* -# Copyright (c) 2024-2025 Eclipse Foundation and others. -# This program and the accompanying materials are made available -# under the terms of the Eclipse Public License 2.0 -# which is available at http://www.eclipse.org/legal/epl-v20.html -# SPDX-License-Identifier: EPL-2.0 -# ******************************************************************************* -import json -from typing import Any - -from otterdog.logging import get_logger -from otterdog.providers.github.exception import GitHubException - -from . import RestApi, RestClient - -_logger = get_logger(__name__) - - -class PullRequestClient(RestClient): - def __init__(self, rest_api: RestApi): - super().__init__(rest_api) - - async def get_pull_request( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - ) -> dict[str, Any]: - _logger.debug("getting pull request with number '%s' from repo '%s/%s'", pull_request_number, org_id, repo_name) - - try: - return await self.requester.request_json("GET", f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}") - except GitHubException as ex: - raise RuntimeError(f"failed retrieving pull request:\n{ex}") from ex - - async def create_pull_request( - self, - org_id: str, - repo_name: str, - title: str, - head: str, - base: str, - body: str | None = None, - ) -> dict[str, Any]: - _logger.debug("creating pull request for repo '%s/%s'", org_id, repo_name) - - try: - data = { - "title": title, - "head": head, - "base": base, - } - - if body is not None: - data["body"] = body - - return await self.requester.request_json("POST", f"/repos/{org_id}/{repo_name}/pulls", data=data) - except GitHubException as ex: - raise RuntimeError(f"failed creating pull request:\n{ex}") from ex - - async def get_pull_requests( - self, - org_id: str, - repo_name: str, - state: str = "all", - base_ref: str | None = None, - ) -> list[dict[str, Any]]: - _logger.debug("getting pull requests from repo '%s/%s'", org_id, repo_name) - - try: - params = {"state": state} - - if base_ref is not None: - params.update({"base": base_ref}) - - return await self.requester.request_paged_json("GET", f"/repos/{org_id}/{repo_name}/pulls", params=params) - except GitHubException as ex: - raise RuntimeError(f"failed retrieving pull requests:\n{ex}") from ex - - async def merge_pull_request( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - method: str, - ) -> dict[str, Any]: - _logger.debug("merging pull request #%s for repo '%s/%s'", pull_request_number, org_id, repo_name) - - try: - data = {"merge_method": method} - return await self.requester.request_json( - "PUT", f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}/merge", data=data - ) - except GitHubException as ex: - raise RuntimeError(f"failed merging pull request:\n{ex}") from ex - - async def get_commits( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - ) -> list[dict[str, Any]]: - _logger.debug("getting commits for pull request #%s from repo '%s/%s'", pull_request_number, org_id, repo_name) - - try: - return await self.requester.request_paged_json( - "GET", f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}/commits" - ) - except GitHubException as ex: - raise RuntimeError(f"failed retrieving pull request commits:\n{ex}") from ex - - async def get_reviews( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - ) -> list[dict[str, Any]]: - _logger.debug("getting reviews for pull request #%s from repo '%s/%s'", pull_request_number, org_id, repo_name) - - try: - return await self.requester.request_paged_json( - "GET", f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}/reviews" - ) - except GitHubException as ex: - raise RuntimeError(f"failed retrieving pull request reviews:\n{ex}") from ex - - async def request_reviews( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - reviewers: list[str] | None = None, - team_reviewers: list[str] | None = None, - ) -> bool: - _logger.debug( - "requesting reviews for pull request #%s in repo '%s/%s': %s, %s", - pull_request_number, - org_id, - repo_name, - reviewers, - team_reviewers, - ) - - if (reviewers is None or len(reviewers) == 0) and (team_reviewers is None or len(team_reviewers) == 0): - _logger.error( - "requesting reviews for pull request #%s in repo '%s/%s' without any reviewer specified", - pull_request_number, - org_id, - repo_name, - ) - return False - - try: - data = {} - - if reviewers is not None: - data["reviewers"] = reviewers - - if team_reviewers is not None: - data["team_reviewers"] = team_reviewers - - status, body = await self.requester.request_raw( - "POST", - f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}/requested_reviewers", - data=json.dumps(data), - ) - - if status == 201: - return True - elif status == 422: - _logger.warning("failed to request reviews for reviewers (%s, %s): %s", reviewers, team_reviewers, body) - return False - else: - raise RuntimeError( - f"failed requesting reviews for pull request #{pull_request_number} in repo " - f"'{org_id}/{repo_name}'\n{status}: {body}" - ) - - except GitHubException as ex: - raise RuntimeError(f"failed requesting pull request reviews:\n{ex}") from ex - - async def get_files( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - ) -> list[dict[str, Any]]: - _logger.debug("getting files for pull request #%s from repo '%s/%s'", pull_request_number, org_id, repo_name) - - try: - return await self.requester.request_paged_json( - "GET", f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}/files" - ) - except GitHubException as ex: - raise RuntimeError(f"failed retrieving pull request files:\n{ex}") from ex - - async def merge( - self, - org_id: str, - repo_name: str, - pull_request_number: str, - commit_message: str | None = None, - merge_method: str = "squash", - ) -> bool: - _logger.debug("merging pull request #%s from repo '%s/%s'", pull_request_number, org_id, repo_name) - - try: - data = { - "merge_method": merge_method, - } - - if commit_message is not None: - data.update({"commit_message": commit_message}) - - response = await self.requester.request_json( - "PUT", - f"/repos/{org_id}/{repo_name}/pulls/{pull_request_number}/merge", - data=data, - ) - return response["merged"] - except GitHubException as ex: - raise RuntimeError(f"failed merging pull request:\n{ex}") from ex diff --git a/otterdog/providers/github/rest/requester.py b/otterdog/providers/github/rest/requester.py index e90f21ed..a71ec636 100644 --- a/otterdog/providers/github/rest/requester.py +++ b/otterdog/providers/github/rest/requester.py @@ -29,6 +29,8 @@ class Requester: + """Low-level access to the GitHub REST API, handling authentication, caching, retries, and statistics.""" + def __init__( self, auth_strategy: AuthStrategy | None, diff --git a/otterdog/webapp/db/models.py b/otterdog/webapp/db/models.py index 61d62929..27b00ad3 100644 --- a/otterdog/webapp/db/models.py +++ b/otterdog/webapp/db/models.py @@ -109,16 +109,26 @@ class PullRequestModel(Model): closed_at: datetime | None = None merged_at: datetime | None = Field(index=True, default=None) + def automerge_problems(self) -> list[str]: + problems = [] + if self.status != PullRequestStatus.OPEN: + problems.append("pull request is not open") + if self.valid is False: + problems.append("pull request is not valid") + if self.supports_auto_merge is False: + problems.append( + "pull request cannot be automatically merged " + "(contains secrets, requires web UI changes, includes deletions or touches non-configuration files)" + ) + if self.author_can_auto_merge is False and self.has_required_approvals is False: + problems.append( + "pull request does not have the required approvals, " + "and the author is not eligible for merge without approvals" + ) + return problems + def can_be_automerged(self) -> bool: - return ( - self.valid is True - # do not explicitly require that the config is in sync - # only changes to the latest checked in config are applied - # this allows projects to update the config even if its out-of-sync. - # and self.in_sync is True - and self.supports_auto_merge is True - and (self.author_can_auto_merge is True or self.has_required_approvals is True) - ) + return not self.automerge_problems() class StatisticsModel(Model): diff --git a/otterdog/webapp/policies/macos_large_runners.py b/otterdog/webapp/policies/macos_large_runners.py index 4c47f460..c2ec7ac9 100644 --- a/otterdog/webapp/policies/macos_large_runners.py +++ b/otterdog/webapp/policies/macos_large_runners.py @@ -42,12 +42,12 @@ async def evaluate( uses_restricted_runner, permitted = self._is_workflow_job_permitted(payload.labels) if not permitted: - from otterdog.webapp.utils import get_rest_api_for_installation + from otterdog.webapp.utils import get_github_provider_for_installation run_id = payload.run_id - rest_api = await get_rest_api_for_installation(installation_id) - cancelled = await rest_api.action.cancel_workflow_run(github_id, repo_name, run_id) + github_provider = await get_github_provider_for_installation(installation_id) + cancelled = await github_provider.rest_api.action.cancel_workflow_run(github_id, repo_name, run_id) logger.info(f"cancelled workflow run #{run_id} in repo '{github_id}/{repo_name}': success={cancelled}") await self._update_status(github_id, uses_restricted_runner, permitted) diff --git a/otterdog/webapp/tasks/__init__.py b/otterdog/webapp/tasks/__init__.py index 7a210955..9561c64a 100644 --- a/otterdog/webapp/tasks/__init__.py +++ b/otterdog/webapp/tasks/__init__.py @@ -28,8 +28,7 @@ schedule_task, ) from otterdog.webapp.utils import ( - get_graphql_api_for_installation, - get_rest_api_for_installation, + get_github_provider_for_installation, get_temporary_base_directory, ) @@ -112,17 +111,21 @@ def __repr__(self) -> str: class InstallationBasedTask(Protocol): installation_id: int - __rest_api: RestApi | None = None - __graphql_api: GraphQLClient | None = None + __github_provider: GitHubProvider | None = None __rest_statistics: RequestStatistics | None = None __graphql_statistics: RequestStatistics | None = None + @property + async def github_provider(self) -> GitHubProvider: + if self.__github_provider is None: + self.__github_provider = await get_github_provider_for_installation(self.installation_id) + return self.__github_provider + @property async def rest_api(self) -> RestApi: - if self.__rest_api is None: - self.__rest_api = await get_rest_api_for_installation(self.installation_id) - return self.__rest_api + github_provider = await self.github_provider + return github_provider.rest_api @property def rest_statistics(self) -> RequestStatistics: @@ -138,9 +141,8 @@ def graphql_statistics(self) -> RequestStatistics: @property async def graphql_api(self) -> GraphQLClient: - if self.__graphql_api is None: - self.__graphql_api = await get_graphql_api_for_installation(self.installation_id) - return self.__graphql_api + github_provider = await self.github_provider + return github_provider.graphql_client def _merge_rest_statistics(self, other: RequestStatistics) -> None: self.rest_statistics.merge(other) @@ -153,11 +155,9 @@ def merge_statistics_from_provider(self, provider: GitHubProvider) -> None: self._merge_graphql_statistics(provider.graphql_client.statistics) def _update_task_model(self, task: TaskModel) -> None: - if self.__rest_api is not None: - self._merge_rest_statistics(self.__rest_api.statistics) - - if self.__graphql_api is not None: - self._merge_graphql_statistics(self.__graphql_api.statistics) + if self.__github_provider is not None: + self._merge_rest_statistics(self.__github_provider.rest_api.statistics) + self._merge_graphql_statistics(self.__github_provider.graphql_client.statistics) if self.rest_statistics.total_requests == 0: cache_stats = "rest: no requests" @@ -255,11 +255,8 @@ def schedule_automerge_task(self, org_id: str, repo_name: str, pull_request_numb ) async def _cleanup(self) -> None: - if self.__rest_api is not None: - await self.__rest_api.close() - - if self.__graphql_api is not None: - await self.__graphql_api.close() + if self.__github_provider is not None: + await self.__github_provider.close() async def get_organization_config( diff --git a/otterdog/webapp/tasks/apply_changes.py b/otterdog/webapp/tasks/apply_changes.py index f7bfd84d..9ab22a3b 100644 --- a/otterdog/webapp/tasks/apply_changes.py +++ b/otterdog/webapp/tasks/apply_changes.py @@ -68,10 +68,9 @@ async def _pre_execute(self) -> bool: ) if isinstance(self.pull_request_or_number, int): - rest_api = await self.rest_api - response = await rest_api.pull_request.get_pull_request( - self.org_id, self.repo_name, str(self.pull_request_or_number) - ) + github = await self.github_provider + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_or_number) + response = await pr.get_data() self._pull_request = PullRequest.model_validate(response) else: self._pull_request = self.pull_request_or_number @@ -118,7 +117,10 @@ async def _pre_execute(self) -> bool: comment = await render_template( "comment/wrong_team_apply_comment.txt", admin_teams=get_full_admin_team_slugs(self.org_id) ) - await rest_api.issue.create_comment(self.org_id, self.repo_name, str(self.pull_request_number), comment) + provider = await self.github_provider + await provider.rest_api.issue.create_comment( + self.org_id, self.repo_name, str(self.pull_request_number), comment + ) self.logger.error( f"apply for pull request #{self.pull_request_number} triggered by user '{self.author}' " @@ -222,7 +224,10 @@ async def _execute(self) -> ApplyResult: admin_teams=get_full_admin_team_slugs(self.org_id), ) - await rest_api.issue.create_comment(self.org_id, org_config.config_repo, pull_request_number, result) + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( + self.org_id, org_config.config_repo, pull_request_number, result + ) return apply_result diff --git a/otterdog/webapp/tasks/auto_merge_comment.py b/otterdog/webapp/tasks/auto_merge_comment.py index f15c56fa..06e57e35 100644 --- a/otterdog/webapp/tasks/auto_merge_comment.py +++ b/otterdog/webapp/tasks/auto_merge_comment.py @@ -45,8 +45,8 @@ async def _execute(self) -> None: ): comment = await render_template("comment/auto_merge_comment.txt") - rest_api = await self.rest_api - await rest_api.issue.create_comment( + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( self.org_id, self.repo_name, str(self.pull_request_number), diff --git a/otterdog/webapp/tasks/blueprints/__init__.py b/otterdog/webapp/tasks/blueprints/__init__.py index 188c18b8..cdeeea27 100644 --- a/otterdog/webapp/tasks/blueprints/__init__.py +++ b/otterdog/webapp/tasks/blueprints/__init__.py @@ -122,10 +122,8 @@ async def _create_branch_if_needed(self, default_branch: str) -> bool: return False async def _find_existing_pull_request(self, default_branch: str) -> int | None: - rest_api = await self.rest_api - open_pull_requests = await rest_api.pull_request.get_pull_requests( - self.org_id, self.repo_name, "open", default_branch - ) + github = await self.github_provider + open_pull_requests = await github.get_pull_requests(self.org_id, self.repo_name, "open", default_branch) for pr in open_pull_requests: if pr["head"]["ref"] == self.branch_name: @@ -159,8 +157,8 @@ async def _create_pull_request( dashboard_url=dashboard_url, ) - rest_api = await self.rest_api - created_pr = await rest_api.pull_request.create_pull_request( + github = await self.github_provider + created_pr = await github.create_pull_request( self.org_id, self.repo_name, pr_title, @@ -169,11 +167,10 @@ async def _create_pull_request( pr_body, ) - pull_request_number = created_pr["number"] + pull_request_number = created_pr.pr_number if team_reviewers is not None and len(team_reviewers) > 0: - await rest_api.pull_request.request_reviews( - self.org_id, self.repo_name, pull_request_number, team_reviewers=team_reviewers - ) + pr = github.pull_request(self.org_id, self.repo_name, pull_request_number) + await pr.request_reviews(team_reviewers=team_reviewers) return pull_request_number diff --git a/otterdog/webapp/tasks/check_sync.py b/otterdog/webapp/tasks/check_sync.py index 410779c8..4959266f 100644 --- a/otterdog/webapp/tasks/check_sync.py +++ b/otterdog/webapp/tasks/check_sync.py @@ -79,12 +79,12 @@ async def _pre_execute(self) -> bool: self.repo_name, ) - rest_api = await self.rest_api + github = await self.github_provider + rest_api = github.rest_api if isinstance(self.pull_request_or_number, int): - response = await rest_api.pull_request.get_pull_request( - self.org_id, self.repo_name, str(self.pull_request_number) - ) + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_or_number) + response = await pr.get_data() self._pull_request = PullRequest.model_validate(response) else: self._pull_request = self.pull_request_or_number @@ -92,11 +92,8 @@ async def _pre_execute(self) -> bool: # do not perform checks every time a pull request is synchronized # if the last check was done within an hour if self.is_triggered_from_comment is False: - commits = await rest_api.pull_request.get_commits( - self.org_id, - self.repo_name, - str(self.pull_request_number), - ) + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_number) + commits = await pr.get_commits() if len(commits) > 1: previous_commit = commits[-2] @@ -199,8 +196,8 @@ def sync_callback( ) if comment is not None: - rest_api = await self.rest_api - await rest_api.issue.create_comment( + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( self.org_id, org_config.config_repo, self.pull_request_number, diff --git a/otterdog/webapp/tasks/complete_pull_request.py b/otterdog/webapp/tasks/complete_pull_request.py index 73024eb7..6ee82127 100644 --- a/otterdog/webapp/tasks/complete_pull_request.py +++ b/otterdog/webapp/tasks/complete_pull_request.py @@ -75,7 +75,10 @@ async def _pre_execute(self) -> bool: comment = await render_template( "comment/wrong_team_done_comment.txt", admin_teams=get_full_admin_team_slugs(self.org_id) ) - await rest_api.issue.create_comment(self.org_id, self.repo_name, str(self.pull_request_number), comment) + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( + self.org_id, self.repo_name, self.pull_request_number, body=comment + ) self.logger.error( f"apply for pull request #{self.pull_request_number} triggered by user '{self.author}' " @@ -91,8 +94,10 @@ async def _execute(self) -> None: await update_pull_request(self._pr_model) comment = await render_template("comment/done_comment.txt") - rest_api = await self.rest_api - await rest_api.issue.create_comment(self.org_id, self.repo_name, str(self.pull_request_number), comment) + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( + self.org_id, self.repo_name, self.pull_request_number, body=comment + ) def __repr__(self) -> str: return f"CompletePullRequestTask(repo={self.org_id}/{self.repo_name}, pull_request=#{self.pull_request_number})" diff --git a/otterdog/webapp/tasks/fetch_all_pull_requests.py b/otterdog/webapp/tasks/fetch_all_pull_requests.py index abce38e6..02991b38 100644 --- a/otterdog/webapp/tasks/fetch_all_pull_requests.py +++ b/otterdog/webapp/tasks/fetch_all_pull_requests.py @@ -34,11 +34,9 @@ async def _execute(self) -> None: self.repo_name, ) - rest_api = await self.rest_api + github = await self.github_provider - all_pull_requests = await rest_api.pull_request.get_pull_requests( - self.org_id, self.repo_name, state="all", base_ref="main" - ) + all_pull_requests = await github.get_pull_requests(self.org_id, self.repo_name, state="all", base_ref="main") for pr in all_pull_requests: pr_from_github = PullRequest.model_validate(pr) diff --git a/otterdog/webapp/tasks/help_comment.py b/otterdog/webapp/tasks/help_comment.py index 45482763..39164061 100644 --- a/otterdog/webapp/tasks/help_comment.py +++ b/otterdog/webapp/tasks/help_comment.py @@ -54,7 +54,6 @@ async def _execute(self) -> None: self.repo_name, ) - rest_api = await self.rest_api comment = await render_template("comment/help_comment.txt") await self.minimize_outdated_comments( @@ -64,7 +63,8 @@ async def _execute(self) -> None: "", ) - await rest_api.issue.create_comment( + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( self.org_id, self.repo_name, str(self.pull_request_number), diff --git a/otterdog/webapp/tasks/merge_pull_request.py b/otterdog/webapp/tasks/merge_pull_request.py index cabb6534..16142a45 100644 --- a/otterdog/webapp/tasks/merge_pull_request.py +++ b/otterdog/webapp/tasks/merge_pull_request.py @@ -10,7 +10,7 @@ from quart import render_template -from otterdog.webapp.db.models import PullRequestStatus, TaskModel +from otterdog.webapp.db.models import TaskModel from otterdog.webapp.db.service import find_pull_request from otterdog.webapp.tasks import ( InstallationBasedTask, @@ -36,14 +36,7 @@ def create_task_model(self): pull_request=self.pull_request_number, ) - async def _pre_execute(self) -> bool: - self.logger.info( - "auto-merging pull request #%d on behalf of user '%s' for repo '%s/%s'", - self.pull_request_number, - self.author, - self.org_id, - self.repo_name, - ) + async def _pre_execute_and_return_problems(self) -> list[str]: pr_model = await find_pull_request(self.org_id, self.repo_name, self.pull_request_number) if pr_model is None: @@ -53,24 +46,12 @@ async def _pre_execute(self) -> bool: else: self._pr_model = pr_model - if pr_model.status != PullRequestStatus.OPEN: - self.logger.info( - f"pull request #{self.pull_request_number} for repo '{self.org_id}/{self.repo_name}' " - "is not open, skipping" - ) - return False - - if pr_model.can_be_automerged() is False: - self.logger.info( - f"pull request #{self.pull_request_number} for repo '{self.org_id}/{self.repo_name}' " - "is not eligible for auto-merge, skipping" - ) - return False + if problems := pr_model.automerge_problems(): + return problems - rest_api = await self.rest_api - response = await rest_api.pull_request.get_pull_request( - self.org_id, self.repo_name, str(self.pull_request_number) - ) + github = await self.github_provider + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_number) + response = await pr.get_data() pull_request = PullRequest.model_validate(response) if self.author != pull_request.user.login: @@ -82,21 +63,44 @@ async def _pre_execute(self) -> bool: team_membership = [team["name"] for team in team_data] if not contains_eligible_team_for_auto_merge(team_membership): - comment = await render_template("comment/wrong_user_merge_comment.txt") - await rest_api.issue.create_comment(self.org_id, self.repo_name, str(self.pull_request_number), comment) + return [ + "Only the author of the pull request, a project-lead or a member of the admin teams " + "is allowed to auto-merge." + ] + + return [] - self.logger.error( - f"merge for pull request #{self.pull_request_number} triggered by user '{self.author}' " - "who is not the creator of the PR and not eligible for auto-merge, skipping" - ) + async def _pre_execute(self) -> bool: + self.logger.info( + "auto-merging pull request #%d on behalf of user '%s' for repo '%s/%s'", + self.pull_request_number, + self.author, + self.org_id, + self.repo_name, + ) - return False + problems = await self._pre_execute_and_return_problems() + if problems: + self.logger.info( + f"pull request #{self.pull_request_number} merge in repo '{self.org_id}/{self.repo_name}' triggered by user '{self.author}' " + f"is not eligible for auto-merge, skipping. Problems: {', '.join(problems)}" + ) + comment = await render_template("comment/automerge_problems.txt", problems=problems) + github_provider = await self.github_provider + await github_provider.rest_api.issue.create_comment( + self.org_id, + self.repo_name, + str(self.pull_request_number), + body=comment, + ) + return False return True async def _execute(self) -> None: - rest_api = await self.rest_api - merged = await rest_api.pull_request.merge(self.org_id, self.repo_name, str(self.pull_request_number)) + github = await self.github_provider + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_number) + merged = await pr.merge() if merged is True: self.logger.info(f"Pull Request #{self.pull_request_number} auto-merged") diff --git a/otterdog/webapp/tasks/retrieve_team_membership.py b/otterdog/webapp/tasks/retrieve_team_membership.py index 8e0e95d2..9ff633f5 100644 --- a/otterdog/webapp/tasks/retrieve_team_membership.py +++ b/otterdog/webapp/tasks/retrieve_team_membership.py @@ -45,10 +45,9 @@ def create_task_model(self): async def _pre_execute(self) -> bool: if isinstance(self.pull_request_or_number, int): - rest_api = await self.rest_api - response = await rest_api.pull_request.get_pull_request( - self.org_id, self.repo_name, str(self.pull_request_number) - ) + github = await self.github_provider + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_number) + response = await pr.get_data() self._pull_request = PullRequest.model_validate(response) else: self._pull_request = self.pull_request_or_number diff --git a/otterdog/webapp/tasks/update_pull_request.py b/otterdog/webapp/tasks/update_pull_request.py index 385005bb..f2bb3250 100644 --- a/otterdog/webapp/tasks/update_pull_request.py +++ b/otterdog/webapp/tasks/update_pull_request.py @@ -47,12 +47,10 @@ async def _execute(self) -> None: ) if self.review is not None: - rest_api = await self.rest_api - # check if the PR was approved by a team eligible for auto-merge - reviews = await rest_api.pull_request.get_reviews( - self.org_id, self.repo_name, str(self.pull_request_number) - ) + github = await self.github_provider + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_number) + reviews = await pr.get_reviews() approved_by_users = [x["user"]["login"] for x in filter(lambda x: x["state"] == "APPROVED", reviews)] diff --git a/otterdog/webapp/tasks/validate_pull_request.py b/otterdog/webapp/tasks/validate_pull_request.py index 8874fe8e..fc38eed4 100644 --- a/otterdog/webapp/tasks/validate_pull_request.py +++ b/otterdog/webapp/tasks/validate_pull_request.py @@ -32,7 +32,7 @@ if TYPE_CHECKING: from otterdog.operations.diff_operation import DiffStatus from otterdog.operations.validate import ValidationStatus - from otterdog.providers.github.rest import RestApi + from otterdog.providers.github import GitHubProvider @dataclass @@ -77,10 +77,9 @@ def create_task_model(self): async def _pre_execute(self) -> bool: if isinstance(self.pull_request_or_number, int): - rest_api = await self.rest_api - response = await rest_api.pull_request.get_pull_request( - self.org_id, self.repo_name, str(self.pull_request_or_number) - ) + github = await self.github_provider + pr = github.pull_request(self.org_id, self.repo_name, self.pull_request_or_number) + response = await pr.get_data() self._pull_request = PullRequest.model_validate(response) else: self._pull_request = self.pull_request_or_number @@ -105,14 +104,14 @@ async def _execute(self) -> ValidationResult: ) async with self.get_organization_config() as org_config: - rest_api = await self.rest_api + github = await self.github_provider org_config_file = org_config.jsonnet_config.org_config_file # get BASE config base_file = org_config_file + "-BASE" await fetch_config_from_github( - rest_api, + github.rest_api, self.org_id, self.org_id, org_config.config_repo, @@ -126,7 +125,7 @@ async def _execute(self) -> ValidationResult: # get HEAD config from PR head_file = org_config_file await fetch_config_from_github( - rest_api, + github.rest_api, self.org_id, head_repo.owner.login, head_repo.name, @@ -136,7 +135,7 @@ async def _execute(self) -> ValidationResult: validation_result = ValidationResult() - for file in await self._get_pull_request_files(rest_api): + for file in await self._get_pull_request_files(github): self.logger.debug(f"touched file: {file}") if file != f"otterdog/{self.org_id}.jsonnet": validation_result.touches_non_configuration = True @@ -267,12 +266,12 @@ async def _update_final_status(self, validation_result: ValidationResult) -> Non if pull_request_model.can_be_automerged(): self.schedule_automerge_task(self.org_id, self.repo_name, self.pull_request_number) - async def _get_pull_request_files(self, rest_api: RestApi) -> list[str]: - pull_request_data = await rest_api.pull_request.get_files( + async def _get_pull_request_files(self, github: GitHubProvider) -> list[str]: + pull_request_data = await github.pull_request( self.org_id, self.repo_name, - str(self.pull_request_number), - ) + self.pull_request_number, + ).get_files() return [x["filename"] for x in pull_request_data] def __repr__(self) -> str: diff --git a/otterdog/webapp/templates/comment/automerge_problems.txt b/otterdog/webapp/templates/comment/automerge_problems.txt new file mode 100644 index 00000000..4f92c524 --- /dev/null +++ b/otterdog/webapp/templates/comment/automerge_problems.txt @@ -0,0 +1,6 @@ +> [!WARNING] +> This pull request cannot be merged by the author + +{% for problem in problems %} +- problem +{% endfor %} diff --git a/otterdog/webapp/templates/comment/wrong_user_merge_comment.txt b/otterdog/webapp/templates/comment/wrong_user_merge_comment.txt deleted file mode 100644 index 51ac8b85..00000000 --- a/otterdog/webapp/templates/comment/wrong_user_merge_comment.txt +++ /dev/null @@ -1,2 +0,0 @@ -> [!WARNING] -> Only the author of the pull request, a project-lead or a member of the admin teams is allowed to auto-merge it. diff --git a/otterdog/webapp/utils.py b/otterdog/webapp/utils.py index 60a49899..482e2606 100644 --- a/otterdog/webapp/utils.py +++ b/otterdog/webapp/utils.py @@ -21,10 +21,11 @@ from otterdog.cache import get_github_cache from otterdog.config import OtterdogConfig +from otterdog.credentials import Credentials +from otterdog.providers.github import GitHubProvider from otterdog.providers.github.auth import app_auth, token_auth from otterdog.providers.github.cache.ghproxy import ghproxy_cache from otterdog.providers.github.cache.redis import redis_cache -from otterdog.providers.github.graphql import GraphQLClient from otterdog.providers.github.rest import RestApi from otterdog.webapp.blueprints import Blueprint, read_blueprint from otterdog.webapp.policies import Policy, read_policy @@ -106,14 +107,17 @@ def decode_bytes_dict(data: dict[bytes, bytes]) -> dict[str, str]: return {k.decode("utf-8"): v.decode("utf-8") for k, v in data.items()} -async def get_rest_api_for_installation(installation_id: int) -> RestApi: +async def get_github_provider_for_installation(installation_id: int) -> GitHubProvider: token, _ = await get_token_for_installation(installation_id) - return RestApi(token_auth(token), get_github_cache()) - - -async def get_graphql_api_for_installation(installation_id: int) -> GraphQLClient: - token, _ = await get_token_for_installation(installation_id) - return GraphQLClient(token_auth(token), get_github_cache()) + return GitHubProvider( + Credentials( + _username=None, + _password=None, + _totp_secret=None, + _last_totp=None, + _github_token=token, + ) + ) def get_app_root_directory(app: Quart | None = None) -> str: diff --git a/tests/providers/github/integration/test_pull_request.py b/tests/providers/github/integration/test_pull_request.py new file mode 100644 index 00000000..542055f4 --- /dev/null +++ b/tests/providers/github/integration/test_pull_request.py @@ -0,0 +1,82 @@ +# ******************************************************************************* +# Copyright (c) 2026 Eclipse Foundation and others. +# This program and the accompanying materials are made available +# under the terms of the Eclipse Public License 2.0 +# which is available at http://www.eclipse.org/legal/epl-v20.html +# SPDX-License-Identifier: EPL-2.0 +# ******************************************************************************* + +from .conftest import GitHubProviderTestKit + +ORG_ID = "test-org" +REPO_NAME = "test-repo" +PR_NUMBER = 123 + + +def pull_request(github: GitHubProviderTestKit): + return github.provider.pull_request(ORG_ID, REPO_NAME, PR_NUMBER) + + +async def test_get_data_returns_pull_request_payload(github: GitHubProviderTestKit): + github.http.expect( + "GET", + f"/repos/{ORG_ID}/{REPO_NAME}/pulls/{PR_NUMBER}", + response_json={"number": PR_NUMBER, "state": "open"}, + ) + + data = await pull_request(github).get_data() + + assert data == {"number": PR_NUMBER, "state": "open"} + + +async def test_request_reviews(github: GitHubProviderTestKit): + github.http.expect( + "POST", + f"/repos/{ORG_ID}/{REPO_NAME}/pulls/{PR_NUMBER}/requested_reviewers", + request_json={"reviewers": ["alice"], "team_reviewers": ["core"]}, + response_status=201, + response_text="created", + ) + + result = await pull_request(github).request_reviews(reviewers=["alice"], team_reviewers=["core"]) + + assert result is True + + +async def test_merge_pull_request_returns_api_response(github: GitHubProviderTestKit): + github.http.expect( + "PUT", + f"/repos/{ORG_ID}/{REPO_NAME}/pulls/{PR_NUMBER}/merge", + request_json={"merge_method": "squash"}, + response_json={"merged": True, "sha": "1234"}, + ) + + response = await pull_request(github).merge_pull_request("squash") + + assert response == {"merged": True, "sha": "1234"} + + +async def test_merge_rebase(github: GitHubProviderTestKit): + github.http.expect( + "PUT", + f"/repos/{ORG_ID}/{REPO_NAME}/pulls/{PR_NUMBER}/merge", + request_json={"merge_method": "rebase"}, + response_json={"merged": True}, + ) + + merged = await pull_request(github).merge(merge_method="rebase") + + assert merged is True + + +async def test_merge_squash(github: GitHubProviderTestKit): + github.http.expect( + "PUT", + f"/repos/{ORG_ID}/{REPO_NAME}/pulls/{PR_NUMBER}/merge", + request_json={"merge_method": "squash", "commit_message": "chore: merge"}, + response_json={"merged": True}, + ) + + merged = await pull_request(github).merge(commit_message="chore: merge", merge_method="squash") + + assert merged is True