Update service mapping URLs in preparation for the dist-git migration - #996
Update service mapping URLs in preparation for the dist-git migration#996betulependule wants to merge 10 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 11:38 AM UTC · Completed 11:52 AM UTC |
|
✔️ pre-commit SUCCESS in 3m 21s |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Labels: PR significantly modifies Pagure service implementation (default URL, decorators, probing) alongside Forgejo migration work. Next steps:
Previous run (3)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (4)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (5)ReviewFindingsHigh
Medium
Low
Labels: PR modifies Pagure service mapping and adds Forgejo dist-git support Next steps:
Previous run (6)ReviewFindingsCritical
High
Medium
Low
Next steps:
Previous run (7)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsHigh
Medium
Low
Labels: PR modifies Forgejo service mapping and adds Fedora dist-git domain registrations |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
area/forgejo
2258023 to
3bf6904
Compare
|
🤖 Review · ❌ Terminated · Started 11:59 AM UTC · Ended 12:14 PM UTC |
|
✔️ pre-commit SUCCESS in 3m 29s |
Superseded by updated review
fullsend-ai-review
Bot
added
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 11:59 AM UTC · Completed 12:14 PM UTC |
could we make it work with both at the same time? I assume that would ease up testing and transition period. |
I don’t think both should be a problem, though… I we cannot have both Forgejo and Pagure under one domain, and also I’m not sure how would the syncing on Fedora Infra work… afaik the changes flow only from prod to stage on Fedora Infra, not the other way around… yet… we would submit production builds and changes, so… this would be a mess |
I think it should be possible to support both? Packit uses the get_service_class_or_none to get the appropriate service class, which would (in the current implementation) only return an instance of PagureService or ForgejoService (whichever it got to first in the for loop). This function could be modified so that ogr checks which one should be used, or we could use a new function, which would return all candidate service classes and packit would handle the checks. If the migration of all repos happens at once, we can check the backend at runtime: Or if the migration is gradual per-repo, we would need |
|
I asked the team behind the migration and I was told they plan for the migration of all repos to happen all at once. They will migrate all repos to a new deployment and once that is done, make It would still be problematic for us if we simply edit I suppose we could:
I can't say I like this solution and it would be very clunky with a lot of unnecessary API calls, so I hope there could be a better solution. |
3bf6904 to
3295e11
Compare
|
🤖 Review · |
3295e11 to
98d1358
Compare
|
🤖 Review · ❌ Terminated · Started 8:31 AM UTC · Ended 8:51 AM UTC |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 27s |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
🤖 Finished Review · ✅ Success · Started 8:31 AM UTC · Completed 8:51 AM UTC |
|
If my understanding is correct, the requre tests are failing because of the API call to https://src.fedoraproject.org/api/0/version (a lacking recording of a response to this GET request). Multiple files in the |
98d1358 to
440e8c1
Compare
|
🤖 Review · ❌ Terminated · Started 11:30 AM UTC · Ended 12:10 PM UTC |
|
✔️ pre-commit SUCCESS in 3m 24s |
95ef178 to
0d6cbd8
Compare
|
🤖 Review · ❌ Terminated · Started 7:09 AM UTC · Ended 7:29 AM UTC |
|
✔️ pre-commit SUCCESS in 3m 51s |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 7:09 AM UTC · Completed 7:28 AM UTC |
"https://src.fedoraproject.org" will no longer be associated with Pagure after the migration. The value of `instance_url` has been changed to "https://pagure.io".
The two hostnames will no longer be associated with Pagure after the migration of dist-git and should be removed. The edit to the test recording is more of a hot fix. It would be probably better to add a new recording, but not sure if it's worth it.
0d6cbd8 to
6a76adb
Compare
|
🤖 Review · ❌ Terminated · Started 7:53 AM UTC · Ended 8:31 AM UTC |
|
Build succeeded. ✔️ pre-commit SUCCESS in 4m 33s |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 7:53 AM UTC · Completed 8:31 AM UTC |
|
|
||
| parsed_url = parse_git_repo(url) | ||
|
|
||
| # [XXX] remove once the migration of dist-git is finished |
There was a problem hiding this comment.
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
| api_endpoint = "https://src.fedoraproject.org/api/0/version" | ||
| api_endpoint_stg = "https://src.stg.fedoraproject.org/api/0/version" |
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…
There was a problem hiding this comment.
Are you sure? I can get to "https://src.stg.fedoraproject.org/api/0/version" just fine.
| 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: |
There was a problem hiding this comment.
#sarcastic 50x could also mean it’s a Pagure…
| self, | ||
| token: Optional[str] = None, | ||
| instance_url: str = "https://src.fedoraproject.org", | ||
| instance_url: str = "https://pagure.io", |
There was a problem hiding this comment.
this is going to die too tbf, not sure if before or after dist-git
There was a problem hiding this comment.
Yep, as you pointed out, "pagure.io" is read-only now. I could change instance_url to "git.centos.org", but if that's going to die as well, then I might as well keep it set to "src.fedoraproject.org".
There was a problem hiding this comment.
(best would be no default, but… that is breaking change… idk, the pagure.io is probably the most sane default)
There was a problem hiding this comment.
Well, changing instance_url from "https://src.fedoraproject.org" to anything else is a breaking change regardless. I'm thinking that once this is released before the migration, whoever is using PagureService without explicitly setting instance_url to "https://src.fedoraproject.org" will run into an error, despite dist-git still being on Pagure at that point. Maybe it's safer to keep the default value set to "https://src.fedoraproject.org" and only change it in a follow-up PR after the migration to make sure no users are affected before the migration.
There was a problem hiding this comment.
As fullsend pointed out, another breaking change similar to this one is in pagure/project.py where removing the hardcoded dist-git URLs would break the is_private method whenever called pre-migration.
I think it would be best to drop 0506e3d7c9a45c16a2f9ef2fcce5097bd5b2bf1e and d044e7c6932f22179a4e2bf08ef599ac1f7d0276 from this PR, then create a follow-up PR with these two commits and merge it after migration, not before.
Co-authored-by: Matej Focko <mfocko@users.noreply.github.com>
|
🤖 Review · ❌ Terminated · Started 2:26 PM UTC · Ended 2:48 PM UTC Commit: |
|
✔️ pre-commit SUCCESS in 3m 28s |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 2:26 PM UTC · Completed 2:48 PM UTC Commit: |
Related to packit/ogr#996 Signed-off-by: Matej Focko <mfocko@packit.dev>
Fix `get_project_from_url` repo parsing in `ForgejoService` The duplication of two existing test cases and editting them to test code in relation to ForgejoService revealed that the parsing of repo from a given url was incomplete. The get_project_from_url method would extract python-dockerpty.git instead of the expected string python-dockerpty. That is because the stripping of the .git extension was omitted in the previous implementation. It is now fixed. This bug was discovered in: #996. I've separated the fix to this separate PR as it's not directly related to #996. RELEASE NOTES BEGIN The ForgejoService.get_project_from_url() method has been updated to ensure repository names are parsed accurately. It now correctly removes the .git extension from the repository name, addressing an issue where the extension was previously retained. RELEASE NOTES END Reviewed-by: fullsend-ai-review[bot] Reviewed-by: Alžběta Kučerová Reviewed-by: Tomas Tomecek <tomas@tomecek.net> Reviewed-by: Matej Focko
Co-authored-by: Matej Focko <mfocko@users.noreply.github.com>
|
🤖 Review · ❌ Terminated · Started 9:26 AM UTC · Ended 10:06 AM UTC Commit: |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 35s |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
ogr/factory.py:127: [high] behavioral-contract-change
get_service_class_or_none() was a pure, side-effect-free lookup function. It now makes HTTP network calls for dist-git URLs, introducing latency (up to 5s timeout), mutable cache state, and a new OgrNetworkError exception. While intentional and temporary (marked [XXX]), this fundamentally changes the function's contract for all callers.
Suggested fix: Consider making probing opt-in via a parameter (e.g. probe_dist_git=True), or catch OgrNetworkError internally and fall back to a default.
ogr/factory.py:155: [high] logic-error
The substring check if dgit_url in parsed_url.hostname causes 'src.fedoraproject.org' to match 'src.stg.fedoraproject.org' (and pkgs vs pkgs.stg). Since DGIT_URLS lists shorter entries first, staging URLs are matched by non-staging entries, causing: (1) the probe hits production instead of staging, (2) the cache is keyed incorrectly, and (3) user overrides via service_mapping_update for staging URLs are bypassed.
Suggested fix: Use equality check (dgit_url == parsed_url.hostname) instead of substring containment.
ogr/factory.py:176: [medium] fail-open
The condition response.status_code != 404 treats any non-404 response (including 500, 502, 503) as evidence that Pagure is still running. A transient server error would cause wrong service class selection.
Suggested fix: Use response.ok to confirm Pagure is alive; treat non-2xx, non-404 responses as inconclusive or default to ForgejoService.
ogr/factory.py:148: [medium] error-handling-gap
The probing code only catches ConnectionError and ReadTimeout, but HTTP requests can also raise TooManyRedirects, ChunkedEncodingError, and other RequestException subclasses that propagate unhandled.
Suggested fix: Catch requests.exceptions.RequestException as the base class instead.
ogr/factory.py:176: [medium] new-exception-type
get_service_class_or_none() now raises OgrNetworkError, which was never part of this function's contract. Functions that call it (get_service_class(), get_project(), get_instances_from_dict()) inherit this new exception path. Downstream consumers are unlikely to handle it.
Suggested fix: Catch network errors internally and fall back gracefully, or coordinate with downstream consumers to add error handling.
ogr/services/pagure/service.py:35: [medium] default-parameter-change
PagureService.init default instance_url changed from 'https://src.fedoraproject.org' to 'https://pagure.io'. Any code constructing PagureService() without an explicit instance_url will silently connect to a different server.
Suggested fix: Announce in release notes as a breaking change.
ogr/services/pagure/service.py(file-level): Line 39 · [medium] scope-creep
Changing PagureService's default instance_url is beyond what issue #997 authorizes (which asks to update SERVICE_MAPPING and add a transition mechanism, not change constructor defaults).
Suggested fix: Explicitly call out in the PR description as an intentional breaking change and confirm with maintainers.
ogr/factory.py:127: [medium] return-type-change
get_service_class_or_none() now returns ForgejoService instead of PagureService for dist-git URLs. ForgejoService.init has different parameters than PagureService.init (no read_only, insecure, max_retries, user_agent). Code passing PagureService-specific kwargs will fail.
Suggested fix: Verify ForgejoService.init accepts the same kwargs that downstream callers pass for dist-git URLs. Document the return type change.
tests/unit/conftest.py:13: [low] test-adequacy
The autouse fixture only mocks the 404 response (post-migration). No test covers the pre-migration path (non-404 returning PagureService) or error paths (ConnectionError/ReadTimeout raising OgrNetworkError). The _DGIT_FORGE_CACHE is never cleared between tests.
ogr/services/pagure/project.py:382: [low] scope-creep
Removing src.fedoraproject.org and src.stg.fedoraproject.org from is_private() allowlist means during pre-migration (when probing returns PagureService), is_private() will raise OperationNotSupported instead of returning False.
ogr/factory.py:185: [low] information-disclosure
OgrNetworkError message includes the user's project URL (url parameter) rather than the actual probe endpoint that failed (request_url), making debugging confusing.
ogr/factory.py:162: [low] race-condition
_DGIT_FORGE_CACHE uses a non-atomic check-then-act pattern with no thread-safety controls. Worst case: redundant probe requests in multi-threaded environments.
ogr/constant.py:7: [low] naming-convention
DGIT_URLS uses an abbreviation ('DGIT') not found elsewhere in the codebase. Existing constants use full descriptive names (CLONE_TIMEOUT, DEFAULT_RO_PREFIX_STRING).
ogr/factory.py:155: [low] code-organization
Lazy imports of ForgejoService and PagureService inside the for loop body. While justified to avoid circular imports, this deviates from the codebase's top-level import pattern.
ogr/factory.py:160: [low] probing-hardcoded-urls
For pkgs.fedoraproject.org URLs, the probe hits src.fedoraproject.org, assuming pkgs and src domains migrate atomically. This assumption is undocumented.
ogr/factory.py:20: [low] cache-module-state
No public API to clear or inspect _DGIT_FORGE_CACHE. In long-running processes, cache could serve stale results for up to 2 minutes after migration.
ogr/factory.py:155: [low] architectural-coherence
Dist-git URLs are registered on both ForgejoService (via @use_for_service decorators) and handled by probing code. The static registrations are effectively dead code while probing exists, but serve as the post-migration fallback when probing is removed.
ogr/services/pagure/service.py:249: [low] api-shape-pattern
New timeout parameter has Optional[float] type annotation while existing sibling parameters (params, data, header) in the same method lack type annotations.
|
🤖 Finished Review · ✅ Success · Started 9:26 AM UTC · Completed 10:06 AM UTC Commit: |
3 participants
Service mapping needs to be updated after the migration so that
Config.load_authentication()in packit is able to authenticate against Forgejo as dist-git.Fixes #997
Related to #2681
Note: Ignore the name of the branch that this PR was created from. This PR is unrelated to Fedora Forge. I would rename the branch, but don't want to risk breaking the PR / references to the branch.
Merge right before / during the migration of dist-git from Pagure to Forgejo. This mustn't be merged now. I don't think it would break anything if this were merged now, but it would lead to (currently) unnecessary API calls to see whether
PagureServiceis supposed to be used when working with dist-git. I'll keep this PR a draft until then to make sure this doesn't get accidentally merged.