-
Notifications
You must be signed in to change notification settings - Fork 156
add missing support for proxy mode in proxy runner #1813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1813 +/- ##
==========================================
+ Coverage 42.03% 42.07% +0.03%
==========================================
Files 184 184
Lines 21649 21662 +13
==========================================
+ Hits 9101 9115 +14
+ Misses 11860 11859 -1
Partials 688 688 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
495d807 to
6092c38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds missing support for proxy mode flag in the proxy runner command, enabling users to specify proxy mode directly via command line arguments. This feature was already supported in the run config but not exposed as a command-line flag in the proxy runner itself.
- Adds
--proxy-modeflag to the proxy runner command with default value "sse" - Updates operator to handle proxy mode from MCPServer spec with backward compatibility
- Adds comprehensive tests to verify proxy mode flag functionality and validation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/thv-proxyrunner/app/run.go | Adds proxy-mode flag definition, validation, and usage in runner configuration |
| cmd/thv-proxyrunner/app/run_test.go | Adds comprehensive tests for proxy mode flag parsing, validation, and integration |
| cmd/thv-operator/api/v1alpha1/mcpserver_types.go | Adds ProxyMode field to MCPServerSpec with validation constraints |
| cmd/thv-operator/controllers/mcpserver_runconfig.go | Implements proxy mode handling in operator with default fallback |
| test/e2e/chainsaw/operator/single-tenancy/test-scenarios/configmap-mode/mcpserver.yaml | Adds test configuration using streamable-http proxy mode |
| test/e2e/chainsaw/operator/single-tenancy/test-scenarios/configmap-mode/chainsaw-test.yaml | Adds verification tests for proxy mode functionality in logs and ConfigMap |
Comments suppressed due to low confidence (1)
cmd/thv-proxyrunner/app/run_test.go:1
- The test case sets expectValid to false for invalid proxy mode, but the test doesn't actually validate the proxy mode value - it only tests flag parsing. The validation logic happens in runCmdFunc, not during flag parsing.
package app
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
e098e8a to
e0c068e
Compare
It is covered as part of the runconfig but it was not supported as flag on the proxyrunner itself, while it was supported in run command Related-to: #1638
e0c068e to
964649b
Compare
It is covered as part of the runconfig but it was not supported as flag on the proxyrunner itself, while it was supported in run command
Related-to: #1638