Add new endpoint to get the MCPServer to prefil the MCP server deploy modal#6902
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (15)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6902 +/- ##
==========================================
+ Coverage 64.09% 64.49% +0.39%
==========================================
Files 2530 2504 -26
Lines 76695 77255 +560
Branches 19202 19181 -21
==========================================
+ Hits 49161 49825 +664
+ Misses 27534 27430 -104
... and 120 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Signed-off-by: ppadti <ppadti@redhat.com>
ppadti
changed the title
…erver with remote server url Signed-off-by: ppadti <ppadti@redhat.com>
openshift-ci
bot
added
area/frontend
and removed
needs-rebase
manaswinidas
left a comment
manaswinidas
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left inline comments on the high/medium items.
TL;DR of the main concerns:
- Bug in
ExtractContainerImage— always returns on the first loop iteration, skipping subsequent artifacts - Dead code — conversion result fields (env comments, optional env vars, prereq comments) are computed but discarded by the handler;
IncludeCommentsoption is unused; the new hook + service are never consumed - Breaking behavior — deploy button is now completely hidden (instead of shown disabled) for servers without artifacts
- Missing validation — empty container image produces an invalid CR instead of an error response
- Test gaps — multi-artifact case for
ExtractContainerImage, empty-string URI edge case, loading state
| func ExtractContainerImage(artifacts []models.McpArtifact) string { | ||
| for _, a := range artifacts { | ||
| uri := a.URI | ||
| if strings.HasPrefix(uri, "oci://") { | ||
| return strings.TrimPrefix(uri, "oci://") | ||
| } | ||
| return uri | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Bug: This loop always returns on the first iteration regardless of prefix. If the first artifact isn't oci://-prefixed, it's returned immediately without checking the rest. The return uri should be moved after the loop as a fallback, or the intent needs clarifying.
| func ExtractContainerImage(artifacts []models.McpArtifact) string { | |
| for _, a := range artifacts { | |
| uri := a.URI | |
| if strings.HasPrefix(uri, "oci://") { | |
| return strings.TrimPrefix(uri, "oci://") | |
| } | |
| return uri | |
| } | |
| return "" | |
| } | |
| func ExtractContainerImage(artifacts []models.McpArtifact) string { | |
| for _, a := range artifacts { | |
| uri := a.URI | |
| if strings.HasPrefix(uri, "oci://") { | |
| return strings.TrimPrefix(uri, "oci://") | |
| } | |
| } | |
| if len(artifacts) > 0 { | |
| return artifacts[0].URI | |
| } | |
| return "" | |
| } |
There was a problem hiding this comment.
I believe we will have one artifacts fields in data right? This is from the util - maybe will double check with backend team about the same
| type ConversionOptions struct { | ||
| Name string | ||
| ContainerImage string | ||
| IncludeComments bool |
There was a problem hiding this comment.
Nit: IncludeComments is declared but never read anywhere in this file or the handler. Remove it or wire it up.
There was a problem hiding this comment.
This is was from the util we have - let me double check.
packages/model-registry/upstream/bff/internal/redhat/handlers/mcp_server_converter.go
Show resolved
Hide resolved
| return | ||
| } | ||
|
|
||
| containerImage := helper.ExtractContainerImage(server.Artifacts) |
There was a problem hiding this comment.
If server.Artifacts is empty, containerImage will be "" and the resulting CR will have an empty containerImage.ref. Consider returning a 400/422 here instead of producing an invalid CR.
There was a problem hiding this comment.
I think we will not hit this case as frontend doesn't show a deploy button if the server.artifacts is not present. But I can update this to be handling it from bff
...model-registry/upstream/frontend/src/app/hooks/mcpCatalogDeployment/useMcpServerConverter.ts
Show resolved
Hide resolved
...s/model-registry/upstream/frontend/src/app/pages/mcpCatalog/screens/McpServerDetailsPage.tsx
Show resolved
Hide resolved
| {Name: "API_KEY", Description: "API key for auth", Example: ptr("sk-xxx")}, | ||
| {Name: "ENDPOINT", Description: "Endpoint URL"}, | ||
| }, | ||
| } | ||
|
|
||
| result := ConvertToMCPServer(metadata, ConversionOptions{Name: "env-test", ContainerImage: "img:v1"}) | ||
|
|
||
| assert.Len(t, result.MCPServer.Spec.Config.Env, 2) | ||
| assert.Equal(t, "API_KEY", result.MCPServer.Spec.Config.Env[0].Name) | ||
| assert.Equal(t, "<API_KEY>", result.MCPServer.Spec.Config.Env[0].Value) | ||
|
|
||
| assert.Contains(t, result.EnvComments, "Required environment variables:") | ||
| assert.Contains(t, result.EnvComments, " - API_KEY: API key for auth") | ||
| assert.Contains(t, result.EnvComments, " Example: sk-xxx") | ||
| } | ||
|
|
||
| func TestConvertToMCPServer_OptionalEnvVars(t *testing.T) { | ||
| metadata := &models.McpRuntimeMetadata{ | ||
| OptionalEnvironmentVariables: []models.McpEnvVarMetadata{ | ||
| {Name: "LOG_LEVEL", Description: "Log level", DefaultValue: ptr("info")}, | ||
| {Name: "TIMEOUT", Description: "Timeout in seconds"}, | ||
| }, |
There was a problem hiding this comment.
Missing coverage for the multi-artifact case in ExtractContainerImage — e.g., []McpArtifact{{URI: "docker.io/foo"}, {URI: "oci://quay.io/bar"}}. Adding this test would expose the early-return bug in the implementation.
There was a problem hiding this comment.
will check with backend team before updating this. If this merges, we can follow up
...stream/frontend/src/__tests__/cypress/cypress/tests/mocked/mcpCatalog/mcpServerDetails.cy.ts
Show resolved
Hide resolved
packages/model-registry/upstream/frontend/src/app/api/mcpCatalogDeployment/service.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: ppadti <ppadti@redhat.com>
manaswinidas
left a comment
manaswinidas
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Remote MCP servers like "github" don't have Deploy button while others do - I noticed "Github" MCP server isn't visible in "All MCP servers" but that's maybe a different issue and should be a follow up.
The endpoint works as described:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: manaswinidas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The BFF util converter is added as is from the MCP lifecycle operator backend team |
openshift-merge-bot
bot
merged commit 3ed63a5
into
opendatahub-io:main
That is because on All server section we only show 4 models and the rest will only be visible in its specific section |
…ServerCR types PR opendatahub-io#6910 accidentally deleted the MCPServerCR type and its supporting types from mcpDeploymentTypes.ts while adding McpDeploymentCreateRequest and McpDeploymentUpdateRequest. This caused type errors in service.ts and useMcpServerConverter.ts which both import MCPServerCR. On investigation, the entire MCP deployment feature (types, API service, hooks, and extension) is downstream-only — it doesn't exist in the upstream kubeflow/model-registry repo. These files were incorrectly placed in the upstream/ directory by PRs opendatahub-io#6902 and opendatahub-io#6910. Changes: - Move mcpDeploymentTypes.ts, api/mcpCatalogDeployment/service.ts, hooks/mcpCatalogDeployment/useMcpServerConverter.ts, and hooks/mcpCatalogDeployment/useMcpServerDeployAvailable.ts from upstream/frontend/src/app/ to src/mcpCatalogDeployment/ - Restore MCPServerCR and supporting types (MCPServerMetadata, MCPServerSpec, MCPSourceSpec, etc.) that were deleted in opendatahub-io#6910 - Update imports in moved files to use relative paths instead of ~/ - Move the mcp-catalog.mcp-server/deploy-modal extension from upstream extensions.ts to downstream extensions.ts - Add mod-arch-core dependency to packages/model-registry/package.json so the downstream tsconfig can resolve it Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu>
2 participants
Closes: https://redhat.atlassian.net/browse/RHOAIENG-54986
Description
The PR aims to use the Utility to convert runtimeMetadata into MCPServer kind to prefil the deploy modal for MCP server.
Added a new endpoint which frontend could call to get the MCPServer kind.
This PR now conditionally renders the deploy button based on the server artifacts. if present we will show the button otherwise we hide it.
How Has This Been Tested?
I have used these MCP server data - https://redhat-internal.slack.com/archives/C08G31WCV16/p1774039108953109?thread_ts=1773931929.864539&cid=C08G31WCV16
Its present in both psi-16 and green-3
Run the application federated mode and try this curl
TOKEN=$(oc whoami -t) && curl -s -H "x-forwarded-access-token: $TOKEN" "http://localhost:4000/api/v1/mcp_catalog/mcp_servers/85/mcpserver?namespace=rhoai-model-registries" | jq .Test Impact
Added test for handlers
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main