CNV-83516: Enable kubevirt toolset in default MCP server config - #1833
CNV-83516: Enable kubevirt toolset in default MCP server config#1833lyarwood wants to merge 1 commit into
Conversation
|
@lyarwood: This pull request references CNV-83516 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
📝 WalkthroughWalkthroughThe standalone OpenShift MCP server configuration now enables the ChangesOpenShift MCP configuration
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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
🤖 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/utils/mcp_server_config_test.go`:
- Around line 27-28: Convert the test around the TOML toolsets assertion from
testing.T and strings.Contains to the repository’s Ginkgo v2/Gomega style, using
the established suite and matcher patterns. Preserve the assertion that the
config explicitly contains the toolsets list ending with kubevirt, and remove
the direct testing and strings-based assertion usage.
🪄 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: Enterprise
Run ID: c48462c1-7fa3-4426-84ac-fddabc79387f
📒 Files selected for processing (2)
internal/controller/utils/mcp_server_config.gointernal/controller/utils/mcp_server_config_test.go
| if !strings.Contains(config, `toolsets = ["core", "config", "helm", "metrics", "kubevirt"]`) { | ||
| t.Error("TOML config should pin toolsets explicitly") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required Ginkgo v2/Gomega test style.
This updated test still uses testing.T and strings.Contains. Convert it to the repository’s Ginkgo/Gomega pattern while preserving the kubevirt assertion.
As per coding guidelines and path instructions, *_test.go files must use Ginkgo v2/Gomega and shared repository test patterns.
🤖 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/utils/mcp_server_config_test.go` around lines 27 - 28,
Convert the test around the TOML toolsets assertion from testing.T and
strings.Contains to the repository’s Ginkgo v2/Gomega style, using the
established suite and matcher patterns. Preserve the assertion that the config
explicitly contains the toolsets list ending with kubevirt, and remove the
direct testing and strings-based assertion usage.
Sources: Coding guidelines, Path instructions
|
/retest-required |
edef312 to
5e0a3b6
Compare
The kubevirt toolset handles the absence of CNV gracefully by returning tool-level errors rather than crashing the MCP server, making it safe to enable by default. Signed-off-by: Lee Yarwood <lyarwood@redhat.com> Assisted-By: Claude <noreply@anthropic.com>
5e0a3b6 to
2feb36a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/ocpmcp/assets.go (2)
47-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
utils.DefaultLabels()for asset labels.
selectorLabels()is the shared label source for every generated asset, but it hand-rolls the map. Base it onutils.DefaultLabels()and layer only component-specific selector labels on top.As per coding guidelines: “Use
utils.DefaultLabels()for consistent labeling in asset generation functions.”🤖 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/ocpmcp/assets.go` around lines 47 - 55, Update selectorLabels() to initialize its label map from utils.DefaultLabels(), then overlay only the component-specific selector labels required by the assets. Remove the duplicated hand-rolled default labels while preserving the existing selector values and shared-label consistency.Source: Coding guidelines
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required unexported asset-generator names.
These
assets.gohelpers should be namedgenerate<Resource>rather than exportedGenerate<Resource>functions. Rename the five helpers and update their call sites ininternal/controller/ocpmcp/assets_test.go.As per coding guidelines: “Use naming conventions: functions
reconcile<Resource>andgenerate<Resource>...”
As per path instructions: “Use naming conventions: functionsreconcile<Resource>andgenerate<Resource>...”Also applies to: 67-83, 85-102, 104-133, 135-174
🤖 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/ocpmcp/assets.go` around lines 57 - 64, Rename the five asset helpers in assets.go from exported Generate<Resource> to unexported generate<Resource>, following the required naming convention. Update every corresponding call in internal/controller/ocpmcp/assets_test.go and preserve each helper’s behavior and signatures otherwise.Sources: Coding guidelines, Path instructions
🤖 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.
Nitpick comments:
In `@internal/controller/ocpmcp/assets.go`:
- Around line 47-55: Update selectorLabels() to initialize its label map from
utils.DefaultLabels(), then overlay only the component-specific selector labels
required by the assets. Remove the duplicated hand-rolled default labels while
preserving the existing selector values and shared-label consistency.
- Around line 57-64: Rename the five asset helpers in assets.go from exported
Generate<Resource> to unexported generate<Resource>, following the required
naming convention. Update every corresponding call in
internal/controller/ocpmcp/assets_test.go and preserve each helper’s behavior
and signatures otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de7f0450-0705-49ee-ba87-8cc10237e7de
📒 Files selected for processing (2)
internal/controller/ocpmcp/assets.gointernal/controller/ocpmcp/assets_test.go
|
@lyarwood: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Description
Enable the kubevirt toolset in the default MCP server config deployed by the operator. The kubevirt toolset is safe to enable unconditionally thanks to two layers of protection:
TargetCompatibilityFilters (kubernetes-mcp-server#1298) — kubevirt tools now declare GVK-based filters that check for
kubevirt.io/v1 VirtualMachineon connected clusters. When KubeVirt is not installed, the tools are completely hidden from the MCP client — they never appear in the tool list.Graceful error handling — as a fallback, if a kubevirt tool is somehow invoked on a cluster without CNV, it returns a tool-level error (
IsError: true) to the LLM rather than crashing the MCP server.This replaces the previous justification that relied solely on error handling. With target compatibility filtering, there is zero cost to enabling the toolset on clusters without KubeVirt — the tools simply don't appear.
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
internal/controller/ocpmcp/assets_test.goupdated to validate the kubevirt toolset andexperimental_enable_target_compatibility_tool_filtersin the generated TOML ConfigMap.IsError: true) to the LLM — the MCP server process continues running normally.Summary by CodeRabbit