-
Notifications
You must be signed in to change notification settings - Fork 153
Remove redundant ToolType field from Workload #2932
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
base: main
Are you sure you want to change the base?
Conversation
The ToolType field in the Workload struct mapped to either "mcp" (container workloads) or "remote" (remote workloads), but since MCP is the only alternative type in practice, the field has become unnecessary and adds no value. This change eliminates the redundant field from the codebase, simplifying the business logic that previously checked both ToolType and Remote fields. The condition "ToolType != 'mcp' && !Remote" now simplifies to just checking the Remote field directly. Changes: - Remove ToolType field from core.Workload struct - Delete LabelToolType constant and GetToolType function from labels package - Simplify shouldSkipWorkload logic in client manager - Remove ToolType from CLI list command output - Remove tool_type from backend metadata (workloads and vMCP) - Update all affected tests - Update API documentation (swagger.yaml) and regenerate docs Fixes stacklok#2923 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: carlos <carlosgn@protonmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2932 +/- ##
==========================================
- Coverage 56.36% 56.36% -0.01%
==========================================
Files 323 323
Lines 31763 31753 -10
==========================================
- Hits 17904 17898 -6
+ Misses 12330 12326 -4
Partials 1529 1529 ☔ 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 removes the redundant ToolType field from the Workload struct throughout the codebase. The field previously distinguished between "mcp" (container workloads) and "remote" (remote workloads), but since MCP is the only alternative type in practice and the Remote boolean field provides the same information, ToolType became unnecessary.
Key changes:
- Removed
ToolTypefield fromcore.Workloadstruct and all JSON serialization - Deleted
LabelToolTypeconstant andGetToolType()helper function from labels package - Simplified
shouldSkipWorkload()logic fromworkload.ToolType != mcpToolType && !workload.Remoteto just!workload.Remote
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/core/workload.go |
Removed ToolType field from Workload struct definition |
pkg/core/workload_test.go |
Removed ToolType field from test workload instances and assertions |
pkg/labels/labels.go |
Removed LabelToolType constant, GetToolType() function, and label assignment logic |
pkg/labels/labels_test.go |
Removed LabelToolType from expected label maps and deleted TestGetToolType function |
pkg/workloads/types/types.go |
Removed GetToolType() call and ToolType field assignment in WorkloadFromContainerInfo() |
pkg/workloads/manager.go |
Removed ToolType from backend metadata in two locations (workload conversion and remote workload creation) |
pkg/vmcp/workloads/k8s.go |
Removed ToolType from backend metadata and modernized code with maps.Copy() |
pkg/vmcp/workloads/k8s_test.go |
Removed ToolType assertion from backend metadata test |
pkg/vmcp/aggregator/discoverer_test.go |
Removed ToolType from test backend metadata definitions and assertions |
pkg/client/manager.go |
Simplified shouldSkipWorkload() to check only !workload.Remote instead of compound condition |
pkg/runner/config_test.go |
Removed toolhive-tool-type label from expected container labels in multiple test cases |
cmd/thv/app/list.go |
Removed TOOL TYPE column from CLI list command output |
test/e2e/remote_mcp_server_test.go |
Removed ToolType field from WorkloadInfo struct and test assertion |
docs/server/swagger.yaml |
Removed tool_type field definition from core.Workload schema |
docs/server/swagger.json |
Removed tool_type from JSON API specification (regenerated from swagger.yaml) |
docs/server/docs.go |
Removed tool_type from Go docs template (regenerated from swagger.yaml) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
👀 checking this |
Summary
This PR removes the redundant
ToolTypefield from theWorkloadstruct. The field mapped to either "mcp" (container workloads) or "remote" (remote workloads), but since MCP is the only alternative type in practice, the field has become unnecessary.Changes
ToolTypefield fromcore.WorkloadstructLabelToolTypeconstant andGetToolType()function from labels packageshouldSkipWorkload()logic in client managertool_typefrom backend metadata (both workloads and vMCP)Impact
The business logic that previously checked
ToolType != 'mcp' && !Remotenow simplifies to just checking theRemotefield directly. This makes the code cleaner and easier to understand.Stats: 16 files changed, 9 insertions(+), 96 deletions(-)
Fixes #2923
Signed-off-by: carlos carlosgn@protonmail.com
🤖 Generated with Claude Code
Co-authored-by: Claude noreply@anthropic.com