Skip to content

Update service mapping URLs in preparation for the dist-git migration #996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
betulependule wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Update service mapping URLs in preparation for the dist-git migration #996

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5fa02da
Update Forgejo and Pagure service mapping
betulependule Jul 23, 2026
008ea99
Add temporary dist-git forge detection
betulependule Aug 4, 2026
0597de1
Add timeout when calling Pagure's API
betulependule Aug 4, 2026
1c481d2
Add two-minute caching of dist-git forge service class
betulependule Aug 5, 2026
0506e3d
Update default value of `instance_url` in `PagureService`
betulependule Aug 5, 2026
d044e7c
Remove hardcoded dist-git hostnames from `PagureProject`
betulependule Aug 6, 2026
ad671e3
Update docstrings to match the updated mapping
betulependule Aug 5, 2026
6a76adb
Update doc links in `CONTRIBUTING.md`
betulependule Aug 5, 2026
ef885fc
Refactor caching logic
betulependule Aug 10, 2026
3e3a496
Fix `UnboundLocalError`
betulependule Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ogr/constant.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,10 @@

CLONE_TIMEOUT = 60
DEFAULT_RO_PREFIX_STRING = "READ ONLY: "

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

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.

DGIT_URLS = (
"src.fedoraproject.org",
"src.stg.fedoraproject.org",
"pkgs.fedoraproject.org",
"pkgs.stg.fedoraproject.org",
)
49 changes: 48 additions & 1 deletion ogr/factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,21 @@
# SPDX-License-Identifier: MIT

import functools
import logging
from collections.abc import Iterable
from typing import Optional

from requests.exceptions import ConnectionError

from ogr.abstract import GitProject, GitService
from ogr.exceptions import OgrException
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]] = {}

logger = logging.getLogger(__name__)


def use_for_service(service: str, _func=None):
"""
Expand Down Expand Up @@ -116,6 +122,9 @@ def get_service_class_or_none(
) -> Optional[type[GitService]]:

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Args:
url: URL of the project, e.g. `"https://github.com/packit/ogr"`.
Expand All @@ -126,13 +135,51 @@ def get_service_class_or_none(

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 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

@mfocko mfocko Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an FYI:

  • you need to keep in mind that this will probably get also released, so even if we remove it after the migration is finished, it might live somewhere for some time…
  • also, it would be ideal to have this in some release before the migration

for dgit_url in non_overridden_dgit_urls:

# if dealing with dist-git, we need to check whether we need to use

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high] logic-error

The dist-git URL check uses dgit_url in parsed_url.hostname (substring match) instead of exact equality. Since DGIT_URLS iterates 'src.fedoraproject.org' before 'src.stg.fedoraproject.org', a staging hostname matches the production entry first, causing the probe to hit the production API endpoint for staging URLs.

Suggested fix: Use if dgit_url == parsed_url.hostname: for exact match.

# `PagureService` or `ForgejoService`
if dgit_url in parsed_url.hostname:

from ogr.services.forgejo import ForgejoService
from ogr.services.pagure import PagureService

# API call to the Pagure backend
api_endpoint = "https://src.fedoraproject.org/api/0/version"

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

@mfocko mfocko Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

@betulependule betulependule Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? I can get to "https://src.stg.fedoraproject.org/api/0/version" just fine.

@mfocko mfocko Aug 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pagure.io, not staging dist-git

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)

# if not found, then dist-git is no longer hosted on Pagure
if response.status_code != 404:

@mfocko mfocko Aug 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#sarcastic 50x could also mean it’s a Pagure…

return PagureService
return ForgejoService

except ConnectionError 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
Expand Down
4 changes: 4 additions & 0 deletions ogr/services/forgejo/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@

@use_for_service("forgejo")
@use_for_service("codeberg.org")
@use_for_service("src.fedoraproject.org")

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] intent-contradiction

PR registers dist-git hostnames to ForgejoService via @use_for_service AND adds runtime probing that can return PagureService. The two mechanisms contradict each other.

Suggested fix: Choose one mechanism: decorators (merge when migration complete) or runtime probing (remove dist-git decorators from ForgejoService).

@betulependule betulependule Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters whether the decorators are added to ForgejoService. If dealing with dist-git URLs, probing is always used to determine whether ForgejoService or PagureService should be used. The decorators should have no effect until the probing is removed.

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] service-mapping-break

Both static @use_for_service decorators and runtime probing can return different service classes for the same dist-git URL. The dual registration creates confusing redundancy.

Suggested fix: Document the interaction or remove the static registration since probing handles the decision dynamically.

@use_for_service("src.stg.fedoraproject.org")
@use_for_service("pkgs.fedoraproject.org")
@use_for_service("pkgs.stg.fedoraproject.org")
class ForgejoService(BaseGitService):
version = "/api/v1"

Expand Down
4 changes: 0 additions & 4 deletions ogr/services/pagure/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,6 @@


@use_for_service("pagure")
@use_for_service("src.fedoraproject.org")
@use_for_service("src.stg.fedoraproject.org")
@use_for_service("pkgs.fedoraproject.org")
@use_for_service("pkgs.stg.fedoraproject.org")
@use_for_service("git.centos.org")
@use_for_service("git.stg.centos.org")
class PagureService(BaseGitService):
Expand Down
27 changes: 27 additions & 0 deletions tests/unit/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# 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():

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@betulependule betulependule Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I don't think it matters.

@fullsend-ai-review fullsend-ai-review Bot Aug 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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",
).and_return(
response,
)
flexmock(PagureService).should_receive("get_raw_request").with_args(
url="https://src.stg.fedoraproject.org/api/0/version",
).and_return(
response,
)

return
32 changes: 24 additions & 8 deletions tests/unit/test_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@
from flexmock import Mock, flexmock
from urllib3.util import Retry

from ogr import GithubService, GitlabService, PagureService
from ogr import ForgejoService, GithubService, GitlabService, PagureService
from ogr.exceptions import OgrException
from ogr.factory import get_instances_from_dict, get_project, get_service_class
from ogr.services.forgejo import ForgejoProject
from ogr.services.github import GithubProject
from ogr.services.gitlab import GitlabProject
from ogr.services.pagure import PagureProject
Expand All @@ -35,7 +36,7 @@
{"github.com": PagureService},
PagureService,
),
("https://src.fedoraproject.org/rpms/python-ogr", None, PagureService),
("https://src.fedoraproject.org/rpms/python-ogr", None, ForgejoService),
("https://pagure.io/ogr", None, PagureService),
("https://pagure.something.com/ogr", None, PagureService),
("https://gitlab.com/someone/project", None, GitlabService),
Expand All @@ -55,18 +56,18 @@
(
"https://src.fedoraproject.org/rpms/golang-gitlab-flimzy-testy",
None,
PagureService,
ForgejoService,
),
(
"https://src.stg.fedoraproject.org/rpms/golang-gitlab-flimzy-testy",
None,
PagureService,
ForgejoService,
),
("https://src.fedoraproject.org/rpms/python-gitlab", None, PagureService),
("https://src.fedoraproject.org/rpms/python-gitlab", None, ForgejoService),
(
"https://src.fedoraproject.org/rpms/golang-gitlab-yawning-utls",
None,
PagureService,
ForgejoService,
),
],
)
Expand Down Expand Up @@ -165,10 +166,10 @@ def test_get_service_class_not_found(url, mapping):
None,
None,
True,
PagureProject(
ForgejoProject(
namespace="rpms",
repo="python-ogr",
service=PagureService(instance_url="https://src.fedoraproject.org"),
service=ForgejoService(instance_url="https://src.fedoraproject.org"),
),
),
(
Expand Down Expand Up @@ -350,6 +351,7 @@ def test_get_project_not_found(url, mapping, instances, exc_str):
({"github.com": {"token": "abcd"}}, {GithubService(token="abcd")}),
({"gitlab": {"token": "abcd"}}, {GitlabService(token="abcd")}),
({"pagure": {"token": "abcd"}}, {PagureService(token="abcd")}),
({"forgejo": {"token": "abcd"}}, {ForgejoService(token="abcd")}),
(
{
"pagure": {
Expand All @@ -359,6 +361,20 @@ def test_get_project_not_found(url, mapping, instances, exc_str):
},
{PagureService(token="abcd", instance_url="https://src.fedoraproject.org")},
),
(
{
"forgejo": {
"token": "abcd",
"instance_url": "https://src.fedoraproject.org",
},
},
{
ForgejoService(
token="abcd",
instance_url="https://src.fedoraproject.org",
),
},
),
(
{"github.com": {"token": "abcd"}, "gitlab": {"token": "abcd"}},
{GithubService(token="abcd"), GitlabService(token="abcd")},
Expand Down