feat: add spec.logLevel field to MCPGatewayExtension - #1153
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a Changesspec.logLevel for broker-router
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
coderabbitai
Bot
added
high-risk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/deployment_test.go (1)
622-691: ⚡ Quick winAdd explicit coverage for the unset
spec.logLevelfallback path.This table verifies mapped spec values and precedence, but it no longer asserts behavior when
spec.logLevelis unset and onlyBrokerRouterLogLevelis provided.test case additions
{ name: "spec.logLevel overrides operator-wide BrokerRouterLogLevel", brokerLogLevel: "-4", specLogLevel: mcpv1alpha1.LogLevelError, want: "--log-level=8", }, + { + name: "unset spec.logLevel falls back to operator-wide BrokerRouterLogLevel", + brokerLogLevel: "4", + specLogLevel: "", + want: "--log-level=4", + }, + { + name: "no log-level flag when both spec.logLevel and operator fallback are unset", + brokerLogLevel: "", + specLogLevel: "", + want: "", + },🤖 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 `@internal/controller/deployment_test.go` around lines 622 - 691, The TestBuildBrokerRouterDeployment_SpecLogLevel test table is missing coverage for the scenario where spec.logLevel is unset and the deployment should fall back to using the operator-wide BrokerRouterLogLevel. Add a new test case to the tests slice in this function that sets brokerLogLevel to a specific value but leaves specLogLevel unset (use the zero value of mcpv1alpha1.LogLevel), then verify that the resulting deployment command includes the expected log-level flag mapped from the operator-wide BrokerRouterLogLevel value.
🤖 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 `@internal/controller/broker_router.go`:
- Around line 74-82: The logLevelFlagValues map maps warn to "4", but the
broker-router's logger switch statement in cmd/mcp-broker-router/main.go only
explicitly handles "-4", "0", and "8", causing warn level to fall back to debug.
Update the logLevelFlagValues map entries to use numeric string values that
match what the logger switch statement in the broker-router actually handles for
each log level (debug, info, warn, and error). Verify the mapping by checking
the switch statement logic in cmd/mcp-broker-router/main.go to ensure all
LogLevel values are correctly handled.
---
Nitpick comments:
In `@internal/controller/deployment_test.go`:
- Around line 622-691: The TestBuildBrokerRouterDeployment_SpecLogLevel test
table is missing coverage for the scenario where spec.logLevel is unset and the
deployment should fall back to using the operator-wide BrokerRouterLogLevel. Add
a new test case to the tests slice in this function that sets brokerLogLevel to
a specific value but leaves specLogLevel unset (use the zero value of
mcpv1alpha1.LogLevel), then verify that the resulting deployment command
includes the expected log-level flag mapped from the operator-wide
BrokerRouterLogLevel value.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01abd1cc-b46c-432a-b6fb-ef98f674cc65
⛔ Files ignored due to path filters (2)
charts/mcp-gateway/crds/mcp.kuadrant.io_mcpgatewayextensions.yamlis excluded by!charts/mcp-gateway/crds/**config/crd/mcp.kuadrant.io_mcpgatewayextensions.yamlis excluded by!config/crd/mcp.kuadrant.io_*.yaml
📒 Files selected for processing (5)
api/v1alpha1/mcpgatewayextension_types.gobundle/manifests/mcp-gateway.clusterserviceversion.yamlbundle/manifests/mcp.kuadrant.io_mcpgatewayextensions.yamlinternal/controller/broker_router.gointernal/controller/deployment_test.go
|
Thanks for the contributions! One thing to flag: our contributing guidelines ask contributors to limit themselves to one open PR at a time. The idea is that getting one change reviewed and merged is more valuable than having several open in parallel. You currently have four open PRs. Could you pick whichever one you think is most ready for review, and we'll focus on getting that one through first? |
|
@david-martin sorry for that i had commented on a few issues so i though i should make them ; the most complete pr is #1137 should i make the other prs draft i understand that multiple prs increase the review burden sorry for that would make sure to keep one pr open at a time |
No worries @rogueslasher We're figuring out and evolving the process to ensure it sets expectations and works well for both contributors and reviewers. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@maleck13 please review this one thank you |
|
Needs work before merge:
The rewritten test (TestBuildBrokerRouterDeployment_SpecLogLevel) dropped two cases from the original test:
Did the tests get removed for a good reason? |
Signed-off-by: rogueslasher <aniketpandey25092005@gmail.com>
98e4000 to
83895b3
Compare
…k test cases Signed-off-by: Aniket Pandey <aniketpandey25092005@gmail.com>
83895b3 to
2e1cb95
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@maleck13 sorry for dropping the tests they were accidentally replaced instead of kept alongside the new one. Fixed now along with the rebase and docs update. |
|
Going to approve, but ideally we would exercise this new property via one of the e2e tests |
3 participants
Closes #1099
Adds a
logLevelfield toMCPGatewayExtensionSpecso the operator propagateslog verbosity to the broker-router deployment declaratively instead of requiringmanual patching of--log-levelargs.Maps enum values to the broker's
--log-levelflag:debug=-4,info=0,warn=4,error=8.spec.logLeveltakes precedence over the operator-wideBROKER_ROUTER_LOG_LEVELfallback when set.Changed:
api/v1alpha1/mcpgatewayextension_types.gonewLogLevelenum type and fieldinternal/controller/broker_router.goenum to numeric flag mapping, precedence over operator-wide fallbackinternal/controller/deployment_test.gonewTestBuildBrokerRouterDeployment_SpecLogLevelconfig/crd/...,charts/...,bundle/...regenerated manifestsSummary by CodeRabbit
New Features
debug,info,warn, orerror.Bug Fixes
Documentation