refactor(orchestrator): extract run() into pkg/factories for reuse#2219
refactor(orchestrator): extract run() into pkg/factories for reuse#2219
Conversation
Move the orchestrator lifecycle logic into an exported factories.Run() function with an Options struct that accepts Version, CommitSHA, and an EgressFactory callback. This allows orchestrator-ee to reuse the full init/shutdown flow while plugging in its own egress proxy implementation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Guard against nil EgressFactory in Run() with a clear fatal message instead of a cryptic panic. Also guard against EgressFactory returning (nil, nil) before accessing EgressSetup fields.
PR SummaryMedium Risk Overview Written by Cursor Bugbot for commit 2c3938e. This will update automatically on new commits. Configure here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c3938eedb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !success { | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Run is now exported from pkg/factories for reuse, but it still calls os.Exit(1) internally on failure. That makes the bool return value effectively unusable for error handling and prevents callers (e.g., other binaries/tests reusing this package) from running their own cleanup/defer logic when startup/shutdown fails. Returning the failure and letting the top-level main decide whether to exit preserves reuse without forcing process termination from a library entrypoint.
Useful? React with 👍 / 👎.
| slotStorage, err := newStorage(ctx, nodeID, config.NetworkConfig, egressSetup.Proxy) | ||
| if err != nil { |
There was a problem hiding this comment.
The new callback path accepts EgressFactory output and only checks whether egressSetup itself is nil, then passes egressSetup.Proxy into network storage creation without validation. If a custom factory accidentally returns a nil proxy, slot lifecycle code later calls s.egressProxy.OnSlotCreate/OnSlotDelete (packages/orchestrator/pkg/sandbox/network/network.go), which will panic at runtime when a slot is created or removed. Add an explicit nil-proxy guard (or default to network.NoopEgressProxy) at construction time.
Useful? React with 👍 / 👎.
2 participants
Move the orchestrator lifecycle logic into an exported factories.Run() function with an Options struct that accepts Version, CommitSHA, and an EgressFactory callback. This allows orchestrator-ee to reuse the full init/shutdown flow while plugging in its own egress proxy implementation.