Add project clone retries functionality#1613
Conversation
Signed-off-by: David Kwon <dakwon@redhat.com> Assisted-by: Claude Code Opus 4.6
dkwon17
requested review from
akurinnoy,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkwon17 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe changes introduce retry logic with exponential backoff for git clone operations. Retry configuration is externalized through an environment variable with sensible defaults and validated during initialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
project-clone/internal/global.go (1)
68-76: Consider adding an upper bound forCloneRetries.The validation rejects negative values but allows arbitrarily large retry counts. With exponential backoff, high values like
CloneRetries=30would result in a delay of ~17 years before the final retry attempt (1s * 2^29).Consider capping at a reasonable maximum (e.g., 10) to prevent configuration mistakes from causing excessively long waits.
Suggested change
+const maxCloneRetries = 10 + CloneRetries = defaultCloneRetries if val := os.Getenv(cloneRetriesEnvVar); val != "" { parsed, err := strconv.Atoi(val) - if err != nil || parsed < 0 { - log.Printf("Invalid value for %s: %q, using default (%d)", cloneRetriesEnvVar, val, defaultCloneRetries) + if err != nil || parsed < 0 || parsed > maxCloneRetries { + log.Printf("Invalid value for %s: %q (must be 0-%d), using default (%d)", cloneRetriesEnvVar, val, maxCloneRetries, defaultCloneRetries) } else { CloneRetries = parsed } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@project-clone/internal/global.go` around lines 68 - 76, The env parsing for CloneRetries (using cloneRetriesEnvVar and defaultCloneRetries) only rejects negatives but allows huge values; add an upper bound (e.g., maxCloneRetries = 10) and when parsed > maxCloneRetries log a warning and set CloneRetries = maxCloneRetries, otherwise set CloneRetries = parsed; keep the existing invalid-number fallback behavior and use the same log.Printf pattern to report out-of-range values.project-clone/internal/git/setup.go (1)
66-68: Use%wfor proper error wrapping.Using
%sformats the error as a string but loses the error chain, preventing callers from usingerrors.Is()orerrors.Unwrap()to inspect the underlying cause.Suggested change
if cloneErr != nil { - return fmt.Errorf("failed to clone project: %s", cloneErr) + return fmt.Errorf("failed to clone project: %w", cloneErr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@project-clone/internal/git/setup.go` around lines 66 - 68, The error return currently formats cloneErr with "%s" which discards the error chain; update the return in the clone error path to wrap the original error using "%w" so callers can use errors.Is/errors.Unwrap (i.e., change the return that references cloneErr to use "%w" to wrap the error).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@project-clone/internal/git/setup.go`:
- Around line 66-68: The error return currently formats cloneErr with "%s" which
discards the error chain; update the return in the clone error path to wrap the
original error using "%w" so callers can use errors.Is/errors.Unwrap (i.e.,
change the return that references cloneErr to use "%w" to wrap the error).
In `@project-clone/internal/global.go`:
- Around line 68-76: The env parsing for CloneRetries (using cloneRetriesEnvVar
and defaultCloneRetries) only rejects negatives but allows huge values; add an
upper bound (e.g., maxCloneRetries = 10) and when parsed > maxCloneRetries log a
warning and set CloneRetries = maxCloneRetries, otherwise set CloneRetries =
parsed; keep the existing invalid-number fallback behavior and use the same
log.Printf pattern to report out-of-range values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62363f36-541d-466e-bc69-a145673dc401
📒 Files selected for processing (2)
project-clone/internal/git/setup.goproject-clone/internal/global.go
|
@dkwon17: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Assisted-by: Claude Code Opus 4.6
What does this PR do?
Adds project clone retries functionality.
For the time being, by default, there will be up to 3 retries (4 attempts in total) with an exponential backoff. In a later PR, it would be nice to add a field in the DWOC to configure the number of retry attempts.
What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-9779
Is it tested? How?
After building the image, install DWO:
I have tested two cases:
Verify that regular cloning is working as intended
Create the following DW and verify that project-clone was successful:
Successful project-clone init container:
Verify that retries are working as intended.
Create the following DW. Note that in this DW, the
devworkspace-operatorgit project has typos in the remote urls, which will cause failures during git clone.Verify in the project-clone init container logs, that retries have been logged:
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
PROJECT_CLONE_RETRIESenvironment variable.