feat: migrate RPC surface to Connect-RPC#118
Merged
Merged
Conversation
- plugin.go RPC() returns (string, http.Handler) via informerV1connect.NewInformerServiceHandler from api-go v6.0.0-beta.9 - rpc.go reshaped to the Connect handler interface with 5 methods: - ListPlugins, GetWorkers, GetJobs (NO_SIDE_EFFECTS, GET-eligible) - AddWorker, RemoveWorker - pool/v2/state/process.State and api-plugins/v6/jobs.State are converted per-call to informerV1.ProcessState / informerV1.JobState (int32 pid, float32 cpu_percent to match service.v1.Status widths) - tests/informer_test.go: 4 goridge client.Call sites switched to a single newInformerClient helper using h2c Connect client - new tests/informer_api_test.go covers Connect, plain HTTP+protojson, plain gRPC against the same handler, plus HTTP-GET idempotency for the three read-only methods - new tests/configs/.rr-informer-api.yaml minimal config for the API tests - bump api-go to v6.0.0-beta.9, goridge to v4.0.0-beta.2, rpc/v6 to v6.0.0-beta.4
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR migrates the informer plugin from legacy ChangesConnectRPC Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR migrates the informer plugin’s RPC surface from the legacy net/rpc + Goridge codec to Connect-RPC handlers, making the service reachable via Connect, plain HTTP+protojson, and gRPC on the same rpc.listen port.
Changes:
- Replaced the informer RPC implementation with Connect-RPC methods (
ListPlugins,GetWorkers,GetJobs,AddWorker,RemoveWorker) and updatedPlugin.RPC()to return a Connect handler mount. - Updated existing informer tests to use a Connect client instead of
net/rpc. - Added a new integration-style API test suite that exercises Connect, plain HTTP+protojson, gRPC, and HTTP GET idempotency behavior.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
rpc.go |
Replaces legacy net/rpc service methods with Connect-RPC method implementations and converts internal state types to wire types. |
plugin.go |
Changes RPC() to return the Connect handler mount ((string, http.Handler)) for informer service. |
tests/informer_test.go |
Updates existing tests to call the new Connect-RPC endpoints via a helper client. |
tests/informer_api_test.go |
Adds new tests covering Connect, HTTP+protojson, gRPC, and GET idempotency behavior. |
tests/configs/.rr-informer-api.yaml |
Adds a dedicated config for the new informer API tests. |
go.mod / go.sum |
Adds Connect-RPC + api-go v6.0.0-beta.9 and related dependency updates for the main module. |
tests/go.mod / tests/go.sum |
Updates test module deps to match Connect-RPC usage and bumped RR deps used by tests. |
go.work.sum |
Updates workspace dependency checksums. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (2)
tests/informer_api_test.go (1)
183-189: 💤 Low valueConsider using method-specific request messages.
The test constructs a single
GetWorkersRequestmessage (lines 183-189) and reuses it for all five methods via GET query parameters. While this might work if the server ignores message content for GET requests that don't match, it creates confusion:
ListPluginsexpectsListPluginsRequest(empty message)GetJobsexpectsGetJobsRequest{Plugin: ...}AddWorker/RemoveWorkerexpectAddWorkerRequest/RemoveWorkerRequestIf the intent is to verify HTTP method semantics (405 for mutating methods via GET) regardless of message content, consider either:
- Using empty or minimal messages that are valid for all methods, or
- Adding a comment explaining why a
GetWorkersRequestis being reused♻️ Optional clarification
If the message content is intentionally ignored for this test, add a comment:
body, err := protojson.Marshal(&informerV1.GetWorkersRequest{Plugin: "probe"}) require.NoError(t, err) + // Note: We reuse the same encoded message for all methods since this test + // verifies HTTP method (GET) support, not message validation. q := url.Values{}🤖 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 `@tests/informer_api_test.go` around lines 183 - 189, The test currently reuses a GetWorkersRequest (proto message named GetWorkersRequest) encoded into the GET query (variable body / message param) for multiple endpoints (ListPlugins, GetJobs, AddWorker, RemoveWorker), which is confusing; update the test to encode and send method-specific request messages (e.g., ListPluginsRequest for ListPlugins, GetJobsRequest{Plugin: "probe"} for GetJobs, AddWorkerRequest/RemoveWorkerRequest for the mutating endpoints) or, if you intentionally want to ignore message content to only assert HTTP method semantics, replace the encoded payload with a minimal/empty message valid for all endpoints and add a clear comment above the test explaining that choice so readers understand why GetWorkersRequest is not used across the board.rpc.go (1)
14-20: ⚡ Quick winStabilize
ListPluginsresponse ordering.
pluginsis populated from map iteration, so response order is nondeterministic. Sort before returning to avoid flaky comparisons/caching behavior.Suggested patch
import ( "context" + "sort" "connectrpc.com/connect" informerV1 "github.com/roadrunner-server/api-go/v6/informer/v1" ) @@ for name := range r.plugin.withWorkers { plugins = append(plugins, name) } + sort.Strings(plugins) return connect.NewResponse(&informerV1.PluginsList{Plugins: plugins}), nil }🤖 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 `@rpc.go` around lines 14 - 20, ListPlugins builds the plugins slice by iterating r.plugin.withWorkers which yields nondeterministic map order; to stabilize responses, sort the plugins slice before returning. Modify the ListPlugins function to call sort.Strings(plugins) (importing "sort" if missing) after the loop and before creating the connect.NewResponse so the returned informerV1.PluginsList.Plugins is consistently ordered.
🤖 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 `@rpc.go`:
- Around line 59-62: The AddWorker and RemoveWorker RPCs currently map all
errors from Plugin.AddWorker/RemoveWorker to connect.CodeInternal; change them
to return a 4xx Connect code for unsupported plugins by distinguishing that
specific error: either (A) have the Plugin implementation return a sentinel
error (e.g., plugin.ErrUnsupportedPlugin) or a typed error and in
rpc.AddWorker/rpc.RemoveWorker use errors.Is / type assertion to detect it and
return connect.NewError(connect.CodeInvalidArgument, err), or (B) add a public
validation method on the Plugin interface (e.g.,
SupportsWorkerManagement(pluginName string) bool) and call it from
rpc.AddWorker/rpc.RemoveWorker before invoking mutation methods and return
connect.CodeInvalidArgument when it returns false; keep all other errors mapped
to connect.CodeInternal. Ensure you reference Plugin.AddWorker,
Plugin.RemoveWorker, rpc.AddWorker and rpc.RemoveWorker when applying the
change.
In `@tests/informer_api_test.go`:
- Around line 63-71: The test incorrectly calls wg.Go(...) on a sync.WaitGroup;
replace that with wg.Add(1) immediately before starting a goroutine and launch
the goroutine with go func() { defer wg.Done(); ... } so the anonymous function
uses defer wg.Done() and preserves the select logic reading from ch and stop;
ensure any later synchronization uses wg.Wait() to block until the goroutine
completes.
---
Nitpick comments:
In `@rpc.go`:
- Around line 14-20: ListPlugins builds the plugins slice by iterating
r.plugin.withWorkers which yields nondeterministic map order; to stabilize
responses, sort the plugins slice before returning. Modify the ListPlugins
function to call sort.Strings(plugins) (importing "sort" if missing) after the
loop and before creating the connect.NewResponse so the returned
informerV1.PluginsList.Plugins is consistently ordered.
In `@tests/informer_api_test.go`:
- Around line 183-189: The test currently reuses a GetWorkersRequest (proto
message named GetWorkersRequest) encoded into the GET query (variable body /
message param) for multiple endpoints (ListPlugins, GetJobs, AddWorker,
RemoveWorker), which is confusing; update the test to encode and send
method-specific request messages (e.g., ListPluginsRequest for ListPlugins,
GetJobsRequest{Plugin: "probe"} for GetJobs,
AddWorkerRequest/RemoveWorkerRequest for the mutating endpoints) or, if you
intentionally want to ignore message content to only assert HTTP method
semantics, replace the encoded payload with a minimal/empty message valid for
all endpoints and add a clear comment above the test explaining that choice so
readers understand why GetWorkersRequest is not used across the board.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdd77e9e-74bc-42ea-a765-d58ab38ba19c
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modplugin.gorpc.gotests/configs/.rr-informer-api.yamltests/go.modtests/informer_api_test.gotests/informer_test.go
- handler-side map lookups (mirrors service.loadProcesses / resetter pattern):
GetWorkers / GetJobs surface unknown plugin as CodeNotFound;
AddWorker / RemoveWorker surface "plugin does not support workers management"
as CodeFailedPrecondition (caller-input class, not internal)
- GetJobs maps context.Canceled / DeadlineExceeded to typed Connect codes
- sentinels (errNoSuchPlugin, errNoWorkerManagement) live alongside the handler
- Plugin.{Workers,Jobs,AddWorker,RemoveWorker} thin wrappers removed — no
external callers, handler owns the logic now
- informerPluginWOWorkersRPCTest now expects CodeNotFound (was the very silent-
fallback this fixup removes)
- TestInformerConnectAPI extended to cover RemoveWorker + unknown-plugin
negative paths
- end-of-migration go get -u all sweep: http/v6 beta.6→beta.7 (typecheck-fix
upstream), server/v6 beta.5→beta.6, status/v6 beta.3→beta.5,
resetter/v6 beta.2→beta.3, quic-go, sysconf/numcpus
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #118 +/- ##
=============================
=============================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the informer plugin's RPC surface from net/rpc + goridge codec to Connect-RPC over HTTP/2 (h2c), using
informerV1connect.NewInformerServiceHandlerfrom api-go v6.0.0-beta.9.ListPluginsNO_SIDE_EFFECTSGetWorkersNO_SIDE_EFFECTSGetJobsNO_SIDE_EFFECTSAddWorkerAddWorkerRequestRemoveWorkerRemoveWorkerRequestThe handler converts internal Go types (
pool/v2/state/process.State,api-plugins/v6/jobs.State) to the wire types (informerV1.ProcessState,informerV1.JobState) per call. Field widths match the existingservice.v1.Statusconvention:int32 pid,float32 cpu_percent.All three wire protocols (Connect / plain HTTP+protojson / plain gRPC) reachable on the same
rpc.listenport. Newtests/informer_api_test.gocovers all three plus an HTTP-GET idempotency table.Breaking changes
Wire protocol change: callers using
net/rpc+ goridge codec (client.Call("informer.Workers", name, &out),client.Call("informer.List", true, &out)) must switch to a Connect / HTTP / gRPC client targeting/informer.v1.InformerService/{ListPlugins,GetWorkers,GetJobs,AddWorker,RemoveWorker}. Method name changes:Workers→GetWorkers,Jobs→GetJobs,List→ListPlugins.Dep bumps:
github.com/roadrunner-server/api-go/v6→v6.0.0-beta.9github.com/roadrunner-server/goridge/v4→v4.0.0-beta.2github.com/roadrunner-server/rpc/v6(tests) →v6.0.0-beta.4Summary by CodeRabbit
New Features
Tests