refactor(scheduler): replace goto in Bind with fail closure#2089
Conversation
Replace 3 'goto ReleaseNodeLocks' jumps and the trailing labeled block in Bind() with an inline 'fail' closure. The closure preserves the exact cleanup sequence (releaseAllDevices + recordScheduleBindingResult event) and the (result, nil) return contract used by the kube-scheduler extender protocol. Why: - goto across nested select/switch is a Go code-review anti-pattern; a named closure makes the failure path an explicit, reusable concept. - Keeps the early-exit branches (pod/node lookup) untouched, since they must not invoke releaseAllDevices (no lock acquired yet). - Future error branches can reuse 'return fail(err)'. Verified: go build, go vet, and 'go test -run Bind --race ./pkg/scheduler/...' all pass. Signed-off-by: yxxhero <aiopsclub@163.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesScheduler binding flow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bind method in pkg/scheduler/scheduler.go by replacing goto statements with a local helper function fail to handle binding failures. The review feedback suggests adding a nil check for the error parameter within this helper function to prevent a potential nil pointer dereference when calling e.Error().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Defensive nil check for the 'e' parameter in the Bind() fail closure to prevent potential nil-pointer dereference when calling e.Error(). All current call sites guarantee non-nil err (inside if-err-fail branches), but the closure is a reusable unit and the guard keeps it self-contained for future call sites. Addresses PR Project-HAMi#2089 review comment from gemini-code-assist. Signed-off-by: yxxhero <aiopsclub@163.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, yxxhero 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 |
Summary
Replace 3
goto ReleaseNodeLocksjumps and the trailing labeled cleanup block inScheduler.Bind()with an inlinefailclosure. The closure preserves the exact cleanup sequence and return contract.Motivation
gotoacross nested select/switch is flagged as a Go code-review anti-pattern. A named closure:return fail(err))releaseAllDevices(no lock acquired yet)Changes
pkg/scheduler/scheduler.go—Bind():fail func(e error) (*extenderv1.ExtenderBindingResult, error)closuregoto ReleaseNodeLockswithreturn fail(err)err = ...+if err != nilintoif err = ...; err != nil(idiomatic Go)Behavioral equivalence
return fail(err)→ sameThe extender contract (
return (res, nil)for bind failures vs. scheduler-level errors) is preserved —failalways returnsnilas the second value.AI Assistance Disclosure
This PR was authored with AI assistance (Claude Code) and is disclosed per CONTRIBUTING.md.
Test Plan
go build ./pkg/scheduler/...go vet ./pkg/scheduler/...go test -run Bind -short --race -count=1 ./pkg/scheduler/...— all passSummary by CodeRabbit