Update service mapping URLs in preparation for the dist-git migration #996
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,26 @@ | |
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import functools | ||
| import logging | ||
| import time | ||
| from collections.abc import Iterable | ||
| from typing import Optional | ||
|
|
||
| from requests.exceptions import ConnectionError, ReadTimeout | ||
|
|
||
| from ogr.abstract import GitProject, GitService | ||
| from ogr.constant import DGIT_URLS | ||
| from ogr.exceptions import OgrException, OgrNetworkError | ||
| from ogr.parsing import parse_git_repo | ||
|
|
||
| _SERVICE_MAPPING: dict[str, type[GitService]] = {} | ||
|
|
||
| # cache dist-git forge service class for two minutes | ||
| _DGIT_FORGE_CACHE: dict[str, tuple[float, type[GitService]]] = {} | ||
| _DGIT_FORGE_CACHE_TTL = 120 | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def use_for_service(service: str, _func=None): | ||
| """ | ||
|
|
@@ -116,6 +127,10 @@ def get_service_class_or_none( | |
| ) -> Optional[type[GitService]]: | ||
|
There was a problem hiding this comment. [critical] behavioral-contract-change get_service_class_or_none() now raises OgrNetworkError for dist-git URLs when the Pagure probe request fails with ConnectionError or ReadTimeout. Previously this function returned Optional[type[GitService]] and never raised exceptions. Downstream callers (get_service_class, get_instances_from_dict, get_project, and packit's Config.load_authentication()) do not handle this new exception type. Suggested fix: Either catch the error inside get_service_class_or_none and return a fallback (e.g., ForgejoService from the static mapping) or None, or update all callers to handle OgrNetworkError. |
||
| """ | ||
| Get the matching service class from the URL. | ||
| When attempting to get the matching service class for dist-git, probing | ||
| is used to determine whether `PagureService` or `ForgejoService` | ||
| should be returned. This information is cached for two minutes. The | ||
| probing request is set to timeout after 5 seconds. | ||
|
|
||
| Args: | ||
| url: URL of the project, e.g. `"https://github.com/packit/ogr"`. | ||
|
|
@@ -126,13 +141,60 @@ def get_service_class_or_none( | |
|
|
||
|
There was a problem hiding this comment. [high] behavioral-contract-break get_service_class_or_none() changes from a pure in-memory dict lookup to a function that makes HTTP requests for dist-git URLs and can now raise OgrNetworkError. This is a new failure mode for all callers including get_service_class() and get_instances_from_dict(). Suggested fix: Consider catching OgrNetworkError internally and falling back to ForgejoService, preserving the function's no-raise contract. |
||
| Returns: | ||
| Matched class (subclass of `GitService`) or `None`. | ||
|
|
||
| Raises: | ||
| OgrNetworkError, in case a ConnectionError or ReadTimeout error | ||
| is encountered when attempting to probe Pagure dist-git. | ||
| """ | ||
| mapping = {} | ||
| mapping.update(_SERVICE_MAPPING) | ||
| non_overridden_dgit_urls: Iterable[str] = DGIT_URLS | ||
|
|
||
| if service_mapping_update: | ||
| mapping.update(service_mapping_update) | ||
| non_overridden_dgit_urls = ( | ||
| set(non_overridden_dgit_urls) - service_mapping_update.keys() | ||
| ) | ||
|
|
||
| parsed_url = parse_git_repo(url) | ||
|
|
||
| # [XXX] remove once the migration of dist-git is finished | ||
|
Member
There was a problem hiding this comment. Just an FYI:
|
||
| for dgit_url in non_overridden_dgit_urls: | ||
|
|
||
| # if dealing with dist-git, we need to check whether we need to use | ||
|
There was a problem hiding this comment. [high] logic-error The dist-git URL check uses Suggested fix: Use |
||
| # `PagureService` or `ForgejoService` | ||
| if dgit_url in parsed_url.hostname: | ||
|
|
||
| from ogr.services.forgejo import ForgejoService | ||
| from ogr.services.pagure import PagureService | ||
|
|
||
| cache = _DGIT_FORGE_CACHE.get(dgit_url) | ||
|
|
||
|
There was a problem hiding this comment. [medium] probing-hardcoded-urls Probe logic uses two hardcoded endpoints (src.fedoraproject.org and src.stg.fedoraproject.org) for all four DGIT_URLS entries, implicitly assuming all instances migrate in lockstep. Suggested fix: Add a code comment documenting this assumption, or probe the actual hostname. |
||
| now = time.monotonic() | ||
| if cache and ((now - cache[0]) < _DGIT_FORGE_CACHE_TTL): | ||
| return cache[1] | ||
|
|
||
|
betulependule marked this conversation as resolved.
Outdated
|
||
| # API call to the Pagure backend | ||
| api_endpoint = "https://src.fedoraproject.org/api/0/version" | ||
|
There was a problem hiding this comment. [low] trust-boundary Forge detection relies on HTTP response from Fedora infrastructure. Document the trust assumption. |
||
| api_endpoint_stg = "https://src.stg.fedoraproject.org/api/0/version" | ||
|
Comment on lines
+177
to
+178
Member
There was a problem hiding this comment. I was going to say that it doesn’t work for pagure.io, so it’s some dist-git specific patch, but… now that I checked the forge itself, I cannot seem to get there altogether…
Contributor
Author
There was a problem hiding this comment. Are you sure? I can get to "https://src.stg.fedoraproject.org/api/0/version" just fine. |
||
| request_url = api_endpoint_stg if ".stg." in dgit_url else api_endpoint | ||
|
|
||
| try: | ||
| pagure_service = PagureService() | ||
| response = pagure_service.get_raw_request(url=request_url, timeout=5) | ||
|
|
||
| # if not found, then dist-git is no longer hosted on Pagure | ||
| dgit_service_kls = ( | ||
| PagureService if response.status_code != 404 else ForgejoService | ||
| ) | ||
|
|
||
| _DGIT_FORGE_CACHE[dgit_url] = (now, dgit_service_kls) | ||
| return dgit_service_kls | ||
|
|
||
| except (ConnectionError, ReadTimeout) as er: | ||
| logger.error(er) | ||
| raise OgrNetworkError(f"Cannot connect to url: '{url}'.") from er | ||
|
|
||
| for service, service_kls in mapping.items(): | ||
| if parse_git_repo(service).hostname in parsed_url.hostname: | ||
| return service_kls | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Copyright Contributors to the Packit project. | ||
| # SPDX-License-Identifier: MIT | ||
|
|
||
| import pytest | ||
| from flexmock import flexmock | ||
|
|
||
| from ogr import PagureService | ||
|
|
||
|
|
||
| # mocks API calls to Pagure dist-git made to determine whether dist-git | ||
| # is still hosted on Pagure and returns the status code expected after | ||
| # the migration of dist-git to Forgejo | ||
| @pytest.fixture(autouse=True) | ||
| def setup_api_request_mock(): | ||
|
There was a problem hiding this comment. [low] test-isolation _DGIT_FORGE_CACHE is module-level state never cleared between tests. First test populates cache; subsequent tests within TTL use cached values instead of the mock.
Contributor
Author
There was a problem hiding this comment. Sure, but I don't think it matters. There was a problem hiding this comment. [medium] missing-test-coverage The autouse fixture always returns a 404 response (post-migration state). No test verifies the pre-migration path where Pagure returns a non-404 status and PagureService should be returned. |
||
| response = flexmock(status_code=404) | ||
| flexmock(PagureService).should_receive("get_raw_request").with_args( | ||
| url="https://src.fedoraproject.org/api/0/version", | ||
| timeout=5, | ||
| ).and_return( | ||
| response, | ||
| ) | ||
| flexmock(PagureService).should_receive("get_raw_request").with_args( | ||
| url="https://src.stg.fedoraproject.org/api/0/version", | ||
| timeout=5, | ||
| ).and_return( | ||
| response, | ||
| ) | ||
|
|
||
| return | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] naming-convention
DGIT abbreviation is non-standard in the codebase. Consider DISTGIT_URLS for clarity.