-
Notifications
You must be signed in to change notification settings - Fork 156
Implement CompositeToolRefs resolution in VirtualMCPServer converter #2885
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2885 +/- ##
==========================================
- Coverage 56.41% 56.39% -0.02%
==========================================
Files 322 322
Lines 31638 31758 +120
==========================================
+ Hits 17848 17911 +63
- Misses 12252 12311 +59
+ Partials 1538 1536 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
This PR implements resolution of CompositeToolRefs in the VirtualMCPServer converter, enabling users to reference shared VirtualMCPCompositeToolDefinition resources instead of only defining composite tools inline. The implementation adds Kubernetes resource fetching capabilities to the converter and properly merges inline and referenced tools while validating for duplicate names.
Key changes:
- Added Kubernetes client to the
Converterstruct to enable fetching referenced resources - Implemented orchestration logic to merge inline
CompositeToolswith referencedCompositeToolRefs - Added comprehensive validation to detect duplicate tool names across inline and referenced tools
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/thv-operator/pkg/vmcpconfig/converter.go | Added k8sClient field, new methods for fetching/converting referenced tools, and validation logic |
| cmd/thv-operator/pkg/vmcpconfig/converter_test.go | Added comprehensive unit tests covering successful resolution, merging, error cases, and nil client handling |
| cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go | Updated converter instantiation to pass r.Client for resource fetching |
| cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go | Added end-to-end integration tests verifying ConfigMap generation with referenced tools |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
will address copilot feedback and fix ci |
jhrozek
left a comment
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.
good work, some nits inline
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
|
@amirejaz there is a conflict now |
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
jhrozek
left a comment
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.
Two comments for later, but overall LGTM
…tacklok#2885) * Implement CompositeToolRefs resolution in VirtualMCPServer converter * fix tests by passing workload names * refactor and address feedback * fix linting * fix operator tests integration * update mocks * fix operator tests integration * remove unnecessary comment
Summary
Implements resolution of
CompositeToolRefsin the VirtualMCPServer converter. The converter now fetches referencedVirtualMCPCompositeToolDefinitionresources from Kubernetes, converts them toCompositeToolConfigentries, and merges them with inlineCompositeTools.Fixes #2774
Changes
k8sClientfield toConverterstruct to enable fetching Kubernetes resourcesk8sClientas required parameter (non-variadic)VirtualMCPCompositeToolDefinitionresourcesVirtualMCPCompositeToolDefinitionCRD toCompositeToolConfigr.Clientto converter for resource fetchingTesting
kodeploymentVerification
Tested manually on Kind cluster:
Large PR Justification
This PR implements CompositeToolRefs support for VirtualMCPServer, which requires coordinated changes across API definitions, controller validation, converter logic, watch mechanisms, and integration tests. The changes follow the established GroupRef validation pattern and must be atomic to maintain consistency across all layers.