Conversation
WalkthroughAdds runtime validation for SubgraphType.GRPC_SERVICE routing URLs via a new exported utility Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Comment |
…ng-url-of-grpc-services
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2473 +/- ##
===========================================
+ Coverage 28.34% 63.31% +34.96%
===========================================
Files 127 296 +169
Lines 11144 41686 +30542
Branches 243 4391 +4148
===========================================
+ Hits 3159 26394 +23235
- Misses 7981 15270 +7289
- Partials 4 22 +18
🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controlplane/src/core/util.ts`:
- Around line 769-805: The DNS naming handling currently accepts
"dns://authority" with no host:port; update isValidGrpcNamingScheme to require a
host:port path when the scheme is "dns" and rest starts with "//": after
computing scheme and rest, if scheme === 'dns' and rest.startsWith('//') then
slice off the '//' (const remainder = rest.slice(2)), find const slashIndex =
remainder.indexOf('/'), return false if slashIndex === -1 or the endpoint after
the slash is empty, otherwise validate the endpoint with
isValidHostPort(endpoint); keep existing fallback behavior for other schemes and
for cases where rest does not start with "//". Ensure you reference
isValidGrpcNamingScheme, isValidDnsTarget, and isValidHostPort when making this
change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controlplane/src/core/util.test.ts`:
- Around line 300-305: The test case asserting
isValidGrpcNamingScheme('dns://example.com:8080') contradicts the validator
which treats '//' as an authority indicator and expects an authority/path form;
update the test in controlplane/src/core/util.test.ts to use the non-authority
form (e.g., 'dns:example.com:8080') so the expectation remains true when calling
isValidGrpcNamingScheme.
…ng-url-of-grpc-services
We need to check the routing url sent for grpc service: grpc services have a different and stricter requirement than other urls. If not checking on the controlplane, the users will only see the issue at runtime in the router at the first request toward the gRPC service ... not a good time to see an error!
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist