Conversation
📝 WalkthroughWalkthroughAdds three operator CLI flags collected into an operatorArgs slice, propagates operatorArgs through SetupPlan and deploy steps, injects operatorArgs into the operator manager deployment YAML, and hardens operator status updates to handle resourceVersion conflicts and NotFound cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as "mcp-runtime setup (flags)"
participant Builder as "BuildSetupPlan"
participant Plan as "SetupPlan"
participant Step as "deployOperatorStep"
participant Deployer as "DeployOperatorManifests"
participant YAML as "injectOperatorArgs (manager YAML)"
User->>CLI: run setup with operator flags
CLI->>CLI: buildOperatorArgs() -> operatorArgs
CLI->>Builder: BuildSetupPlan(OperatorArgs)
Builder->>Plan: return SetupPlan (OperatorArgs)
CLI->>Step: invoke deployOperatorStep(ctx.Plan.OperatorArgs)
Step->>Deployer: call DeployOperatorManifests(operatorImage, operatorArgs)
Deployer->>YAML: injectOperatorArgs(manager.yaml, operatorArgs)
YAML-->>Deployer: manager.yaml with args injected
Deployer->>Deployer: apply manifests (operator starts with args)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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]](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.
| // injectOperatorArgs injects operator command-line arguments into the manager deployment YAML. | ||
| // It replaces the existing args section or adds one if it doesn't exist. | ||
| func injectOperatorArgs(yamlContent string, args []string) string { | ||
| if len(args) == 0 { | ||
| return yamlContent | ||
| } | ||
|
|
||
| // Build the new args section | ||
| indent := " " // 8 spaces (standard YAML indentation for args in containers) | ||
| argsYAML := indent + "args:\n" | ||
| for _, arg := range args { | ||
| argsYAML += fmt.Sprintf("%s- %s\n", indent, arg) | ||
| } | ||
|
|
||
| // Pattern to match the args section - find "args:" line and everything after it until next key | ||
| // Match: "args:" followed by any number of indented list items (using [0-9] instead of backreference) | ||
| argsPattern := regexp.MustCompile(`(?m)^(\s*)args:\s*$\n((?:\s+-\s+[^\n]+\n?)*)`) | ||
| if argsPattern.MatchString(yamlContent) { | ||
| // Replace existing args section - capture the indentation from group 1 | ||
| return argsPattern.ReplaceAllString(yamlContent, "${1}args:\n"+argsYAML) | ||
| } | ||
|
|
||
| // If no args section exists, find the command line and add args after it | ||
| // Pattern: matches "command:" followed by command array items | ||
| commandPattern := regexp.MustCompile(`(?m)^(\s*)command:\s*$\n((?:\s+-\s+[^\n]+\n?)+)`) | ||
| if commandPattern.MatchString(yamlContent) { | ||
| // Add args after command | ||
| return commandPattern.ReplaceAllString(yamlContent, "${0}"+argsYAML) | ||
| } | ||
|
|
||
| // Fallback: if we can't find command, try to add after image | ||
| imagePattern := regexp.MustCompile(`(?m)^(\s*)image:\s*\S+$`) | ||
| if imagePattern.MatchString(yamlContent) { | ||
| return imagePattern.ReplaceAllString(yamlContent, "${0}\n"+argsYAML) | ||
| } | ||
|
|
||
| // If all else fails, return original (shouldn't happen with valid manager.yaml) | ||
| return yamlContent | ||
| } |
There was a problem hiding this comment.
Critical: Duplicate "args:" header in YAML injection causes malformed output.
The injectOperatorArgs function has a bug that produces invalid YAML. At line 724-728, argsYAML is built to include "args:\n" as a prefix, but then:
-
Line 735: When replacing an existing args section, the replacement is
"${1}args:\n"+argsYAML, which produces:args: args: - --metrics-bind-address=:9090
-
Lines 743 and 749: The fallback patterns append
argsYAMLwhich already contains theargs:header, which is correct for adding a new section but will have wrong indentation.
🔎 Proposed fix
func injectOperatorArgs(yamlContent string, args []string) string {
if len(args) == 0 {
return yamlContent
}
- // Build the new args section
- indent := " " // 8 spaces (standard YAML indentation for args in containers)
- argsYAML := indent + "args:\n"
+ // Build the args items (without the "args:" header)
+ indent := " " // 8 spaces (standard YAML indentation for args in containers)
+ var argsItems string
for _, arg := range args {
- argsYAML += fmt.Sprintf("%s- %s\n", indent, arg)
+ argsItems += fmt.Sprintf("%s- %s\n", indent, arg)
}
// Pattern to match the args section - find "args:" line and everything after it until next key
// Match: "args:" followed by any number of indented list items (using [0-9] instead of backreference)
argsPattern := regexp.MustCompile(`(?m)^(\s*)args:\s*$\n((?:\s+-\s+[^\n]+\n?)*)`)
if argsPattern.MatchString(yamlContent) {
// Replace existing args section - capture the indentation from group 1
- return argsPattern.ReplaceAllString(yamlContent, "${1}args:\n"+argsYAML)
+ return argsPattern.ReplaceAllString(yamlContent, "${1}args:\n"+argsItems)
}
+ // Full args section for adding new (includes header)
+ argsSection := indent + "args:\n" + argsItems
+
// If no args section exists, find the command line and add args after it
// Pattern: matches "command:" followed by command array items
commandPattern := regexp.MustCompile(`(?m)^(\s*)command:\s*$\n((?:\s+-\s+[^\n]+\n?)+)`)
if commandPattern.MatchString(yamlContent) {
// Add args after command
- return commandPattern.ReplaceAllString(yamlContent, "${0}"+argsYAML)
+ return commandPattern.ReplaceAllString(yamlContent, "${0}"+argsSection)
}
// Fallback: if we can't find command, try to add after image
imagePattern := regexp.MustCompile(`(?m)^(\s*)image:\s*\S+$`)
if imagePattern.MatchString(yamlContent) {
- return imagePattern.ReplaceAllString(yamlContent, "${0}\n"+argsYAML)
+ return imagePattern.ReplaceAllString(yamlContent, "${0}\n"+argsSection)
}
// If all else fails, return original (shouldn't happen with valid manager.yaml)
return yamlContent
}🤖 Prompt for AI Agents
internal/cli/setup.go lines 716-754: The function currently builds argsYAML
including an "args:\n" header and then inserts it in places that also add their
own "args:" header (causing duplicate headers) and uses a fixed indent; fix by
separating the header from the list items: build a string of arg list items only
(e.g., lines like "- <arg>\n") and when replacing or inserting, prepend a single
"args:\n" with the correct captured indentation from the regex match (use the
capture group indentation for list items as well) — for the existing-args
replacement use "${1}args:\n"+argItems, for the command insertion prepend the
match's indentation + "args:\n"+argItems, and for the image fallback append
"\n"+capturedIndent+"args:\n"+argItems; remove the duplicate hardcoded header
and ensure indentation is derived from regex group 1.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
- Coverage 81.96% 80.60% -1.36%
==========================================
Files 22 22
Lines 2744 2794 +50
==========================================
+ Hits 2249 2252 +3
- Misses 346 388 +42
- Partials 149 154 +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.
| // Use Status().Update() which only updates the status subresource | ||
| // This is safer than Update() which would update the entire object | ||
| 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
Reconsider the fallback to full Update().
The fallback to r.Update(ctx, latest) on line 696 updates the entire object, potentially overwriting concurrent spec changes. While the comment mentions fake client compatibility, in production Kubernetes, if Status().Update() fails for reasons other than unsupported status subresource (e.g., validation errors, admission webhook failures), falling back to a full update could cause unintended side effects.
Consider either:
- Removing the fallback entirely and just logging the error (status updates will retry on next reconcile).
- Detecting the fake client environment explicitly and only using the fallback in tests.
- Checking the specific error type to determine if fallback is safe.
🔎 Option 1: Remove the fallback (recommended)
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)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use Status().Update() which only updates the status subresource | |
| // This is safer than Update() which would update the entire object | |
| 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) | |
| } | |
| } | |
| 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 { | |
| // 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) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/operator/controller.go around lines 688 to 703, remove the fallback
call to r.Update(ctx, latest) inside the Status().Update() error handling
because a full Update can overwrite concurrent spec changes; instead, only
handle conflict errors at debug level and log other Status() update errors
without attempting a full Update. If tests require a fallback, restrict that
behavior to test environments only (e.g., detect the fake client or use
build-tags/test helpers) rather than performing a full Update in production.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.