improve error handling around disk partitioning#430
Conversation
WalkthroughThis PR adds retry and polling logic to the Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/windows/disk/partitioner_test.go`:
- Line 283: The failing line uses goimports-incompatible formatting; run
goimports on platform/windows/disk/partitioner_test.go (or the whole repo) and
commit the formatted file so the line with cmdRunner.AddCmdResult(getKey,
fakes.FakeCmdResult{Stdout: "E\n"}) conforms to goimports rules—this will fix
import grouping/formatting and unblock CI; locate the call by searching for
AddCmdResult, getKey, or fakes.FakeCmdResult to confirm the change.
In `@platform/windows/disk/partitioner.go`:
- Around line 174-193: The retry loop in GetDriveLetter (the for attempt := 0;
attempt < 10; attempt++ block using p.ps with driveScript) always calls
time.Sleep(pollInterval) even after the final attempt; modify the loop so it
only sleeps between attempts (e.g., compute maxAttempts := 10 or use attempt <
9) and call time.Sleep(pollInterval) only when attempt < maxAttempts-1, leaving
the error return path (which uses partitionNumber and diskNumber) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9a1c89b-c08a-454b-b674-b14e6c7a4136
📒 Files selected for processing (4)
integration/windows/environment_test.gointegration/windows/ephemeral_disk_test.goplatform/windows/disk/partitioner.goplatform/windows/disk/partitioner_test.go
|
|
||
| cmdRunner.AddCmdResult(addKey, fakes.FakeCmdResult{}) | ||
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "\x00\n"}) // null char — unassigned | ||
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll |
There was a problem hiding this comment.
Fix goimports formatting at this line to unblock lint.
CI is currently failing goimports on this line; please run goimports on the file and commit the formatted result.
Likely formatting-only adjustment
- cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll
+ cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll | |
| cmdRunner.AddCmdResult(getKey, fakes.FakeCmdResult{Stdout: "E\n"}) // assigned on second poll |
🧰 Tools
🪛 GitHub Check: lint (macos-latest)
[failure] 283-283:
File is not properly formatted (goimports)
🪛 GitHub Check: lint (ubuntu-latest)
[failure] 283-283:
File is not properly formatted (goimports)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/windows/disk/partitioner_test.go` at line 283, The failing line uses
goimports-incompatible formatting; run goimports on
platform/windows/disk/partitioner_test.go (or the whole repo) and commit the
formatted file so the line with cmdRunner.AddCmdResult(getKey,
fakes.FakeCmdResult{Stdout: "E\n"}) conforms to goimports rules—this will fix
import grouping/formatting and unblock CI; locate the call by searching for
AddCmdResult, getKey, or fakes.FakeCmdResult to confirm the change.
| for attempt := 0; attempt < 10; attempt++ { | ||
| stdout, _, _, err := p.ps(driveScript) | ||
| if err != nil { | ||
| return "", fmt.Errorf( | ||
| "failed to find drive letter for partition %s on disk %s: %s", | ||
| partitionNumber, | ||
| diskNumber, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| // The DriveLetter property is a char; an unassigned partition returns the null | ||
| // character (\x00). Strip it along with whitespace before checking. | ||
| letter := strings.Trim(strings.TrimSpace(stdout), "\x00") | ||
| if letter != "" { | ||
| return letter, nil | ||
| } | ||
|
|
||
| time.Sleep(pollInterval) | ||
| } |
There was a problem hiding this comment.
Avoid sleeping after the last retry attempt.
The loop sleeps even on the final failed attempt, adding an unnecessary delay before returning the error.
Proposed fix
for attempt := 0; attempt < 10; attempt++ {
stdout, _, _, err := p.ps(driveScript)
if err != nil {
return "", fmt.Errorf(
"failed to find drive letter for partition %s on disk %s: %s",
partitionNumber,
diskNumber,
err,
)
}
// The DriveLetter property is a char; an unassigned partition returns the null
// character (\x00). Strip it along with whitespace before checking.
letter := strings.Trim(strings.TrimSpace(stdout), "\x00")
if letter != "" {
return letter, nil
}
- time.Sleep(pollInterval)
+ if attempt < 9 {
+ time.Sleep(pollInterval)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for attempt := 0; attempt < 10; attempt++ { | |
| stdout, _, _, err := p.ps(driveScript) | |
| if err != nil { | |
| return "", fmt.Errorf( | |
| "failed to find drive letter for partition %s on disk %s: %s", | |
| partitionNumber, | |
| diskNumber, | |
| err, | |
| ) | |
| } | |
| // The DriveLetter property is a char; an unassigned partition returns the null | |
| // character (\x00). Strip it along with whitespace before checking. | |
| letter := strings.Trim(strings.TrimSpace(stdout), "\x00") | |
| if letter != "" { | |
| return letter, nil | |
| } | |
| time.Sleep(pollInterval) | |
| } | |
| for attempt := 0; attempt < 10; attempt++ { | |
| stdout, _, _, err := p.ps(driveScript) | |
| if err != nil { | |
| return "", fmt.Errorf( | |
| "failed to find drive letter for partition %s on disk %s: %s", | |
| partitionNumber, | |
| diskNumber, | |
| err, | |
| ) | |
| } | |
| // The DriveLetter property is a char; an unassigned partition returns the null | |
| // character (\x00). Strip it along with whitespace before checking. | |
| letter := strings.Trim(strings.TrimSpace(stdout), "\x00") | |
| if letter != "" { | |
| return letter, nil | |
| } | |
| if attempt < 9 { | |
| time.Sleep(pollInterval) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/windows/disk/partitioner.go` around lines 174 - 193, The retry loop
in GetDriveLetter (the for attempt := 0; attempt < 10; attempt++ block using
p.ps with driveScript) always calls time.Sleep(pollInterval) even after the
final attempt; modify the loop so it only sleeps between attempts (e.g., compute
maxAttempts := 10 or use attempt < 9) and call time.Sleep(pollInterval) only
when attempt < maxAttempts-1, leaving the error return path (which uses
partitionNumber and diskNumber) unchanged.
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
1 participant
The test cleanup code was trying to unmount c:, this should catch the actual failure instead of falling through to the cleanup code.