Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds operator CLI flags and builds an operatorArgs slice propagated through SetupPlan and setup steps; injects operatorArgs into the manager deployment YAML when present; adds a test-mode path to skip operator build/use preloaded image; and hardens MCPServer status updates to re-fetch resourceVersion and handle NotFound/Conflict. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "mcp-runtime setup (CLI)"
participant Builder as "BuildSetupPlan"
participant Plan as "SetupPlan"
participant Steps as "setup step runner"
participant Deployer as "DeployOperatorManifests"
participant YAML as "manager.yaml injector"
User->>CLI: run setup with operator flags / --test-mode
CLI->>CLI: buildOperatorArgs() -> operatorArgs
CLI->>Builder: BuildSetupPlan(TestMode, OperatorArgs)
Builder->>Plan: return SetupPlan (contains TestMode & OperatorArgs)
CLI->>Steps: run deployOperatorStep(ctx.Plan)
Steps->>Deployer: call DeployOperatorManifests(operatorImage, operatorArgs)
Deployer->>YAML: injectOperatorArgs(manager.yaml, operatorArgs)
YAML-->>Deployer: modified manager.yaml
Deployer->>Deployer: apply manifests (operator starts with injected args)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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🧪 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]](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/cli/setup_helpers_test.go (1)
498-499: Tests updated for new signature, but consider adding operatorArgs coverage.The test call sites correctly pass
nilfor the newoperatorArgsparameter to accommodate the updated signature.Consider adding a test case that verifies
deployOperatorManifestsWithKubectlcorrectly injects operator arguments into the manager YAML whenoperatorArgsis non-nil.Also applies to: 553-553, 575-575, 612-612
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/cli/setup.gointernal/cli/setup_helpers_test.gointernal/cli/setup_plan.gointernal/cli/setup_plan_test.gointernal/cli/setup_steps.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cli/setup.go (3)
internal/cli/registry.go (1)
ExternalRegistryConfig(201-205)internal/cli/printer.go (1)
Info(257-257)internal/cli/kubectl_runner.go (1)
KubectlRunner(6-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit + Integration Tests
🔇 Additional comments (7)
internal/cli/setup_plan.go (1)
13-14: LGTM!The
OperatorArgsfield is correctly added to both input and plan structs, and properly propagated throughBuildSetupPlan. Clean implementation for passing operator arguments through the setup flow.Also applies to: 24-25, 54-55
internal/cli/setup_steps.go (1)
100-110: LGTM!The
deployOperatorStepCmd.Runcorrectly forwardsctx.Plan.OperatorArgsto thedeployOperatorStepfunction, maintaining consistency with the updated function signature.internal/cli/setup_plan_test.go (1)
152-152: LGTM!The mock function signatures for
DeployOperatorManifestsare correctly updated to accept the new[]stringparameter for operator arguments, maintaining consistency with the updated interface.Also applies to: 222-222, 302-302
internal/cli/setup.go (4)
126-128: LGTM!The operator configuration flags are well-named with the
operator-prefix for clarity, and using empty string defaults allows the manager.yaml defaults to apply when flags aren't explicitly set.Also applies to: 168-170
343-348: LGTM!The function signatures for
deployOperatorStep,deployOperatorManifests, anddeployOperatorManifestsWithKubectlare consistently updated to accept the newoperatorArgsparameter, and the arguments are correctly passed through the call chain.Also applies to: 643-649
681-684: LGTM!The conditional check before calling
injectOperatorArgscorrectly avoids unnecessary processing when no operator arguments are provided.
175-191: No issues found with the buildOperatorArgs function.The flag names (
--metrics-bind-address,--health-probe-bind-address,--leader-elect) are correct and standard for controller-runtime operators. The function logic properly handles conditional argument inclusion based on non-empty and non-default values, preserving manager.yaml defaults.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
+ Coverage 77.54% 78.04% +0.50%
==========================================
Files 28 28
Lines 3758 3867 +109
==========================================
+ Hits 2914 3018 +104
Misses 686 686
- Partials 158 163 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/operator/controller.go (2)
640-642: Consider skipping status update on fetch errors instead of fallback.When fetching the latest MCPServer fails (non-NotFound errors), the code falls back to using the original
mcpServerobject. However, this object is likely stale and will cause a conflict error during the Status().Update() call, creating unnecessary retry overhead.Consider returning early on fetch errors (similar to the NotFound case) and letting the next reconcile retry with fresh data.
🔎 Suggested refactor
if err := r.Get(ctx, types.NamespacedName{Name: mcpServer.Name, Namespace: mcpServer.Namespace}, latest); err != nil { // If object not found, it may have been deleted - skip status update if errors.IsNotFound(err) { log.FromContext(ctx).V(1).Info("MCPServer not found, skipping status update (may have been deleted)") return } - // For other errors, log but try to update with the original object as fallback - log.FromContext(ctx).Error(err, "Failed to fetch MCPServer for status update, using original object") - latest = mcpServer + // For other errors, skip status update and let next reconcile retry + log.FromContext(ctx).Error(err, "Failed to fetch MCPServer for status update, skipping update") + return }
659-662: Fallback Update() may inadvertently modify spec fields.When Status().Update() fails, the code falls back to a full Update() on the latest object. However, this
latestobject was fetched via Get(), which includes both spec and status. If there's a mismatch between the spec inlatestand the actual desired spec, this fallback could inadvertently revert spec changes made elsewhere.Consider explicitly setting only status fields before the fallback Update(), or document this limitation.
🔎 Proposed refinement
} else { // Some environments (like the fake client) may not support status subresources. + // Note: This performs a full object update, not just status. + // In rare cases, this could revert concurrent spec changes. if updateErr := r.Update(ctx, latest); updateErr != nil { log.FromContext(ctx).Error(updateErr, "Failed to update MCPServer after status update failure", "resourceVersion", latest.ResourceVersion) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/operator/controller.gointernal/operator/controller_test.gotest/golden/cli/testdata/mcp-runtime_setup_help.golden
🧰 Additional context used
🧬 Code graph analysis (2)
internal/operator/controller_test.go (1)
api/v1alpha1/mcpserver_types.go (1)
MCPServer(120-126)
internal/operator/controller.go (1)
api/v1alpha1/mcpserver_types.go (1)
MCPServer(120-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit + Integration Tests
🔇 Additional comments (3)
internal/operator/controller_test.go (2)
274-280: LGTM: Test updated to support status subresource.The test now correctly initializes the fake client with status subresource support and explicitly creates the MCPServer object. This aligns with the controller's refactored
updateStatusmethod that uses Status().Update().
474-482: LGTM: Test correctly verifies status update via API.The test now fetches the updated MCPServer object from the client and verifies the status fields, which correctly reflects the behavior of the refactored
updateStatusmethod that persists status via the API.test/golden/cli/testdata/mcp-runtime_setup_help.golden (1)
18-20: Verify default values match operator implementation.The help text references default values from
manager.yamlfor the new operator flags. Ensure these defaults (:8080 for metrics, :8081 for probes, false for leader election) match the actual defaults in the operator manager deployment manifest.Run the following script to verify the default values in the operator configuration:
#!/bin/bash # Description: Check default values in operator manager YAML files # Search for manager deployment manifests fd -e yaml -e yml manager config/ # Search for default flag values in Go code rg -nP '\bmetrics.*addr\b|\bprobe.*addr\b|\bleader.*elect\b' --type=go -A2 -B2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/setup.go (1)
408-423: Critical: MissingoperatorArgsparameter breaks the feature.The
deployOperatorStepfunction receivesoperatorArgsas a parameter (line 407) but fails to pass it todeps.DeployOperatorManifestsat line 409. This means the operator command-line arguments will never be applied to the deployment, completely breaking the new feature.🔎 Proposed fix
Info("Deploying operator manifests") - if err := deps.DeployOperatorManifests(logger, operatorImage); err != nil { + if err := deps.DeployOperatorManifests(logger, operatorImage, operatorArgs); err != nil { wrappedErr := wrapWithSentinelAndContext(
♻️ Duplicate comments (1)
internal/cli/setup.go (1)
921-959: Critical: Duplicate "args:" header in YAML injection.This issue was already identified in a previous review. The function builds
argsYAMLwith an"args:\n"prefix (line 930), but then the replacement at line 940 also adds"args:\n", resulting in duplicate headers and malformed YAML output.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cli/setup.gointernal/cli/setup_steps.gointernal/operator/controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cli/setup_steps.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cli/setup.go (2)
internal/cli/registry.go (1)
ExternalRegistryConfig(229-233)internal/cli/kubectl_runner.go (1)
KubectlRunner(6-10)
internal/operator/controller.go (1)
api/v1alpha1/mcpserver_types.go (1)
MCPServer(120-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit + Integration Tests
🔇 Additional comments (9)
internal/operator/controller.go (1)
663-686: LGTM: Robust status update preparation.The re-fetch logic properly handles the latest resourceVersion and avoids optimistic locking conflicts. The NotFound case is handled gracefully, and the fallback to the original object on fetch errors is reasonable.
internal/cli/setup.go (8)
127-129: LGTM: Flag variable declarations are appropriate.The three operator flag variables are correctly typed and positioned.
143-144: LGTM: Operator args construction.The call to
buildOperatorArgscorrectly assembles operator arguments from the CLI flags.
176-192: LGTM: Clean flag-to-args conversion.The function correctly constructs operator arguments, including only explicitly set flags. The logic appropriately handles empty strings and boolean defaults.
407-407: LGTM: Function signature updated correctly.The
operatorArgs []stringparameter is appropriately added to the function signature.
793-799: LGTM: Function signatures consistently updated.Both
deployOperatorManifestsanddeployOperatorManifestsWithKubectlsignatures are correctly updated to acceptoperatorArgs []string. The comment at line 798 appropriately reflects the new behavior.
856-859: LGTM: Conditional injection preserves backward compatibility.The logic correctly injects operator args only when provided, ensuring existing behavior is unchanged when no flags are specified.
45-45: LGTM: Interface signature updated correctly.The
DeployOperatorManifestsfunction type inSetupDepsis appropriately updated to include theoperatorArgs []stringparameter.
169-171: The code is correctly designed. Theoperator-prefixed flags at the CLI level (lines 169-171) are intentional aliases that get translated to standard controller-runtime flag names (--metrics-bind-address,--health-probe-bind-address,--leader-elect) by thebuildOperatorArgs()function (lines 182, 185, 188) before being passed to the operator. This layering avoids flag name conflicts with other setup flags and maintains compliance with controller-runtime conventions. No changes needed.
Resolve setup command conflicts and fix the Trivy and gosec CI failures.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/operator/controller.go (1)
695-698:⚠️ Potential issue | 🟠 MajorRemove the full-object fallback from the status path.
Lines 695-698 switch this code back from a status-only write to a normal object update. In this repo the CRD already exposes
subresources.status(config/crd/bases/mcpruntime.org_mcpservers.yaml:188-189) and the controller already hasmcpservers/statusupdate permission (config/rbac/role.yaml:80-87), so a non-conflict failure here should be treated as a real status-update failure and retried on the next reconcile, not routed throughr.Update. If fake-client compatibility is still needed, keep that behind test-only code instead of the production reconcile path.🛠️ Proposed fix
if err := r.Status().Update(ctx, latest); err != nil { // If it's a conflict error, that's expected in concurrent scenarios - log at debug level if errors.IsConflict(err) { log.FromContext(ctx).V(1).Info("Status update conflict (expected in concurrent reconciles), will retry on next reconcile", "resourceVersion", latest.ResourceVersion) } else { - // Some environments (like the fake client) may not support status subresources. - if updateErr := r.Update(ctx, latest); updateErr != nil { - log.FromContext(ctx).Error(updateErr, "Failed to update MCPServer after status update failure", "resourceVersion", latest.ResourceVersion) - } // Log other errors but don't fail the reconcile - status update failures are non-fatal // The next reconcile will retry the status update log.FromContext(ctx).Error(err, "Failed to update MCPServer status", "resourceVersion", latest.ResourceVersion) } }#!/bin/bash set -u echo "== CRD exposes the status subresource ==" rg -n -C2 'subresources:|status:\s*\{\}' config/crd/bases/mcpruntime.org_mcpservers.yaml || true echo echo "== RBAC allows MCPServer status updates ==" rg -n -C2 'mcpservers/status|verbs:' config/rbac/role.yaml || true echo echo "== Controller tests referencing status-subresource behavior ==" rg -n -C3 'WithStatusSubresource|Status\(\)\.Update|updateStatus' internal/operator/controller_test.go || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operator/controller.go` around lines 695 - 698, The fallback that performs a full-object write via r.Update(ctx, latest) after a status update failure must be removed; instead treat the error from the status update as a real failure and return it (so the controller will retry the status update on the next reconcile). Locate the block containing the call r.Update(ctx, latest) in the reconcile path and delete that fallback branch; ensure the reconcile returns the original status-update error (or wraps it) rather than attempting an object update. If fake-client compatibility is required, move any special-case logic into test-only code or test helpers rather than keeping a production-path r.Update fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/operator/controller.go`:
- Around line 695-698: The fallback that performs a full-object write via
r.Update(ctx, latest) after a status update failure must be removed; instead
treat the error from the status update as a real failure and return it (so the
controller will retry the status update on the next reconcile). Locate the block
containing the call r.Update(ctx, latest) in the reconcile path and delete that
fallback branch; ensure the reconcile returns the original status-update error
(or wraps it) rather than attempting an object update. If fake-client
compatibility is required, move any special-case logic into test-only code or
test helpers rather than keeping a production-path r.Update fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 630a8a21-31db-49b6-b37b-6524f6b9a16f
📒 Files selected for processing (10)
.github/workflows/security-trivy.yamlinternal/cli/printer.gointernal/cli/registry.gointernal/cli/setup.gointernal/cli/setup_helpers_test.gointernal/cli/setup_plan.gointernal/cli/setup_plan_test.gointernal/cli/setup_steps.gointernal/operator/controller.gotest/golden/cli/testdata/mcp-runtime_setup_help.golden
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/security-trivy.yaml
- internal/cli/registry.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/setup_steps.go
- test/golden/cli/testdata/mcp-runtime_setup_help.golden
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/setup_plan_test.go (1)
191-201: Consider adding test coverage for OperatorArgs propagation.The existing
setupPlatformWithDepstests update theDeployOperatorManifestssignature but don't verify thatplan.OperatorArgsis actually passed through. You could enhance the call recorder to capture the arguments, or add a dedicated test that verifies the operatorArgs slice reaches the deployment function.This is optional since the production code appears to be a straightforward pass-through, but it would strengthen confidence in the new feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/setup_plan_test.go` around lines 191 - 201, Add a unit test to assert that SetupPlan.OperatorArgs is passed to DeployOperatorManifests: extend the existing setupPlatformWithDeps test harness (or its call recorder) to capture the arguments passed into DeployOperatorManifests and add a case where plan := SetupPlan{..., OperatorArgs: []string{"--foo","bar"}} is supplied, then assert the recorded call contains that same slice; update the test to fail if the OperatorArgs are not propagated so the signature change is covered by tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cli/setup_plan_test.go`:
- Around line 191-201: Add a unit test to assert that SetupPlan.OperatorArgs is
passed to DeployOperatorManifests: extend the existing setupPlatformWithDeps
test harness (or its call recorder) to capture the arguments passed into
DeployOperatorManifests and add a case where plan := SetupPlan{...,
OperatorArgs: []string{"--foo","bar"}} is supplied, then assert the recorded
call contains that same slice; update the test to fail if the OperatorArgs are
not propagated so the signature change is covered by tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 191c7795-d6f7-4c95-8625-8c29f5536f1b
📒 Files selected for processing (7)
internal/cli/printer_test.gointernal/cli/setup.gointernal/cli/setup_plan.gointernal/cli/setup_plan_test.gointernal/cli/setup_steps.gointernal/cli/setup_steps_test.gotest/golden/cli/testdata/mcp-runtime_setup_help.golden
✅ Files skipped from review due to trivial changes (2)
- internal/cli/setup_plan.go
- internal/cli/setup_steps_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/setup_steps.go
- test/golden/cli/testdata/mcp-runtime_setup_help.golden
- internal/cli/setup.go
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores