fix(e2e): resolve Forgejo fork conflict by enumerating existing forks - #1699
fix(e2e): resolve Forgejo fork conflict by enumerating existing forks#1699kasemAlem wants to merge 1 commit into
Conversation
PR Summary by QodoHandle Forgejo fork conflicts by discovering existing organization forks
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 5:17 PM UTC · Completed 5:29 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.62 |
Code Review by Qodo
1. Tests still target a missing repository
|
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #1699 +/- ##
==========================================
- Coverage 73.97% 68.90% -5.07%
==========================================
Files 74 74
Lines 10617 10617
==========================================
- Hits 7854 7316 -538
- Misses 2006 2534 +528
- Partials 757 767 +10
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Risk Assessment: low (1/5) DetailsSingle-file XS fix to e2e test utility code with minimal churn, single known author, no protected paths, no dependency or CI changes, and a stable git history. Previous runRisk Assessment: low (1/5) DetailsSingle-file XS fix to e2e test utility code with minimal churn, single known author, no protected paths, no dependency or CI changes, and a stable git history; the only elevated signal is the test_file_ratio of 0.00 (the file lacks a _test.go suffix despite living in e2e-tests/), which inflates Tier 1 but does not represent a real production coverage gap. Previous run (2)Risk Assessment: low (1/5) DetailsSingle XS file change (1 file, 19 lines) in a test utility package with a focused bug fix for Forgejo fork conflict resolution; low churn, single author, no protected paths, no dependency or CI changes. |
ReviewFindingsLow
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsLow
Next steps:
|
![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.
52f887b to
1e6243d
Compare
|
🤖 Review · Commit: |
1e6243d to
a86cbda
Compare
|
🤖 Finished Review · ✅ Success · Started 7:03 PM UTC · Completed 7:17 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.55 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Scenario: konflux-e2e
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/konflux-team/integration-service:konflux-e2e-qcbl4Test results analysis🚨 Error occurred while running the E2E tests, list of failed Spec(s): ➡️ [ Click to view logsTimed out after 600.005s.
timed out when waiting for the PipelineRun to start for the component stat-rep-2sup/test-component-pac-7n3oub
Expected success, but got an error:
<*errors.errorString | 0xc0007547b0>:
no pipelinerun found for component test-component-pac-7n3oub
{
s: "no pipelinerun found for component test-component-pac-7n3oub",
}➡️ [ Click to view logsExpected success, but got an error:
<*errors.errorString | 0xc000068e90>:
PipelineRun cannot be created for the Component group-ho3z/konflux-test-integration-clone-ponipn
{
s: "PipelineRun cannot be created for the Component group-ho3z/konflux-test-integration-clone-ponipn",
}OCI Artifact Browser URL |
On 409 Conflict from CreateFork, Forgejo enforces one fork per org per source repo. The previous handler called GetRepo with the *requested* target name, which fails when a stale fork from a prior test run exists under a *different* name — producing the consistent CI failure: fork of X to Y already exists but failed to fetch: The target couldn't be found. Replace the GetRepo lookup with ListForks so the 409 path finds whichever fork already exists in targetOwner's namespace, regardless of its name. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Kasem Alem <kalem@redhat.com>
a86cbda to
1b13923
Compare
|
🤖 Finished Review · ✅ Success · Started 2:00 AM UTC · Completed 2:13 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.16 |
| for _, f := range forks { | ||
| if strings.HasPrefix(f.FullName, prefix) { | ||
| staleName := strings.TrimPrefix(f.FullName, prefix) | ||
| if _, delErr := fc.client.DeleteRepo(targetOwner, staleName); delErr != nil { |
There was a problem hiding this comment.
[low] error-handling
When DeleteRepo fails on the stale fork, a 404 Not Found error (e.g., from a concurrent deletion) is treated as fatal. The separate DeleteRepository method (line 296-307) already swallows 404 errors, establishing a pattern that could be followed here.
Suggested fix: Check whether the delete error indicates a 404 Not Found and, if so, treat it as a successful deletion (set deleted = true and continue to the retry).
| ListOptions: forgejo.ListOptions{Page: page, PageSize: 100}, | ||
| }) | ||
| if listErr != nil { | ||
| return nil, fmt.Errorf("fork of %s already exists in %s but failed to list forks: %w", sourceProjectID, targetOwner, listErr) |
There was a problem hiding this comment.
[low] error-handling-idiom
The error message 'fork of %s already exists in %s but failed to list forks: %w' deviates from the file's consistent 'failed to : %w' pattern (seen at lines 26, 100, 267, etc.).
Suggested fix: Rephrase to follow the prevailing pattern, e.g.: 'failed to list forks of %s in %s: %w'.
| // the name being correct for branch creation and cleanup. | ||
| prefix := targetOwner + "/" | ||
| deleted := false | ||
| for page := 1; !deleted; page++ { |
There was a problem hiding this comment.
[low] scope-coherence
The 409 conflict path now deletes a repository via fc.client.DeleteRepo, which is a destructive side effect for a function named ForkRepository. The inline comment explains the rationale, but the function's godoc (lines 233-236) does not mention that it may delete a stale fork on conflict.
| return nil, fmt.Errorf("fork of %s conflicts in %s but no matching fork found in fork list", sourceProjectID, targetOwner) | ||
| } | ||
| // Retry now that the stale fork is gone. | ||
| forkedRepo, _, retryErr := fc.client.CreateFork(sourceOwner, sourceRepo, forgejo.CreateForkOption{ |
There was a problem hiding this comment.
[low] edge-case
The retry CreateFork does not handle a second 409 Conflict. If another concurrent process creates a fork between the stale-fork deletion and the retry, the error returned would not attempt another list-and-delete cycle.
3 participants
On 409 Conflict from CreateFork, Forgejo enforces one fork per org per source repo. The previous handler called GetRepo with the requested target name, which fails when a stale fork from a prior test run exists under a different name — producing the consistent CI failure:
fork of X to Y already exists but failed to fetch: The target couldn't be found.
Replace the GetRepo lookup with ListForks so the 409 path finds whichever fork already exists in targetOwner's namespace, regardless of its name.
Maintainers will complete the following section