Skip to content

fix: use the mcp user CM for the its tests#1295

Merged
fege merged 3 commits intoopendatahub-io:mainfrom
fege:mcp_cm
Mar 25, 2026
Merged

fix: use the mcp user CM for the its tests#1295
fege merged 3 commits intoopendatahub-io:mainfrom
fege:mcp_cm

Conversation

@fege
Copy link
Copy Markdown
Contributor

@fege fege commented Mar 25, 2026

Pull Request

Summary

Related Issues

  • Fixes:
  • JIRA:

How it has been tested

  • Locally
  • Jenkins

Additional Requirements

  • If this PR introduces a new test image, did you create a PR to mirror it in disconnected environment?
  • If this PR introduces new marker(s)/adds a new component, was relevant ticket created to update relevant Jenkins job?

Summary by CodeRabbit

  • Tests
    • Updated test infrastructure for MCP catalog sources. Changes include ConfigMap naming updates, YAML catalog path adjustments to new directory structures, enhanced test fixture documentation, and new configuration constants. These updates improve organization and consistency of test fixtures supporting MCP server catalog configuration and management.

Signed-off-by: fege <fmosca@redhat.com>
@github-actions
Copy link
Copy Markdown

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/hold', '/verified', '/build-push-pr-image', '/cherry-pick', '/wip', '/lgtm'}

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 25, 2026

📝 Walkthrough

Walkthrough

Refactoring updates test infrastructure to use a new MCP-specific ConfigMap constant (DEFAULT_MCP_CATALOG_CM = "mcp-catalog-sources") in place of the generic model catalog constant, and adjusts MCP YAML catalog path prefixes from /data/user-sources/ to /data/user-mcp-sources/.

Changes

Cohort / File(s) Summary
MCP ConfigMap Constant Definition & Imports
tests/model_registry/constants.py, tests/model_registry/conftest.py, tests/model_registry/mcp_servers/config/utils.py
Introduced new constant DEFAULT_MCP_CATALOG_CM = "mcp-catalog-sources" and replaced all references to DEFAULT_CUSTOM_MODEL_CATALOG with the new constant across fixture and utility code.
MCP YAML Catalog Path Updates
tests/model_registry/mcp_servers/constants.py
Updated path prefix for three YAML catalog path constants from /data/user-sources/ to /data/user-mcp-sources/: MCP_SERVERS_YAML_CATALOG_PATH, MCP_SERVERS_YAML2_CATALOG_PATH, and MCP_SERVERS_YAML_INVALID_CATALOG_PATH.
Fixture & Documentation Updates
tests/model_registry/conftest.py, tests/model_registry/mcp_servers/config/conftest.py
Updated ConfigMap fixture docstrings from "model-catalog-sources ConfigMap" to "mcp-catalog-sources ConfigMap"; no behavioral changes to patch logic or sequencing.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning PR description is entirely empty template boilerplate with no actual summary, issue links, testing details, or implementation notes filled in. Complete the Summary section with why the ConfigMap name changed; add Related Issues if applicable; check both Locally and Jenkins boxes or explain testing approach.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed Title references the main change—updating MCP-related test fixtures and constants to use a dedicated ConfigMap—which aligns with the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/model_registry/mcp_servers/constants.py (1)

191-191: ⚠️ Potential issue | 🟠 Major

Inconsistent path for MCP_SERVERS_YAML3_CATALOG_PATH.

This path still uses /data/user-sources/ while lines 6, 89, and 139 were updated to /data/user-mcp-sources/. The mcp_source_label_configmap_patch fixture (in tests/model_registry/mcp_servers/config/conftest.py lines 87-93) writes mcp-servers-3.yaml to the ConfigMap, but the catalog source at line 213 references the old mount path. This will cause source 3 to fail YAML resolution.

Proposed fix
-MCP_SERVERS_YAML3_CATALOG_PATH: str = "/data/user-sources/mcp-servers-3.yaml"
+MCP_SERVERS_YAML3_CATALOG_PATH: str = "/data/user-mcp-sources/mcp-servers-3.yaml"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/model_registry/mcp_servers/constants.py` at line 191, The constant
MCP_SERVERS_YAML3_CATALOG_PATH currently points to
"/data/user-sources/mcp-servers-3.yaml" which is inconsistent with the other
constants and the mcp_source_label_configmap_patch fixture; update
MCP_SERVERS_YAML3_CATALOG_PATH to use
"/data/user-mcp-sources/mcp-servers-3.yaml" so the catalog source (the YAML
referenced for source 3) matches the ConfigMap mount and resolves correctly.
🧹 Nitpick comments (1)
tests/model_registry/mcp_servers/config/conftest.py (1)

76-78: Docstring inconsistency: still references "model-catalog-sources".

The fixture actually patches the mcp-catalog-sources ConfigMap via get_mcp_catalog_sources(), which uses DEFAULT_MCP_CATALOG_CM. Update docstring for consistency with line 38.

Proposed fix
     """
-    Class-scoped fixture that patches the model-catalog-sources ConfigMap
+    Class-scoped fixture that patches the mcp-catalog-sources ConfigMap
     with three MCP catalog sources: two labeled and one unlabeled.
     Used for sourceLabel filtering tests (TC-API-036 to TC-API-039).
     """
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/model_registry/mcp_servers/config/conftest.py` around lines 76 - 78,
Update the fixture docstring to correctly reference "mcp-catalog-sources"
instead of "model-catalog-sources" to match the behavior of
get_mcp_catalog_sources() which patches DEFAULT_MCP_CATALOG_CM; locate the
class-scoped fixture in conftest.py (the one that calls
get_mcp_catalog_sources()) and change its descriptive text to mention
mcp-catalog-sources and that it patches three MCP catalog sources (two labeled
and one unlabeled) used for sourceLabel filtering tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/model_registry/mcp_servers/constants.py`:
- Line 191: The constant MCP_SERVERS_YAML3_CATALOG_PATH currently points to
"/data/user-sources/mcp-servers-3.yaml" which is inconsistent with the other
constants and the mcp_source_label_configmap_patch fixture; update
MCP_SERVERS_YAML3_CATALOG_PATH to use
"/data/user-mcp-sources/mcp-servers-3.yaml" so the catalog source (the YAML
referenced for source 3) matches the ConfigMap mount and resolves correctly.

---

Nitpick comments:
In `@tests/model_registry/mcp_servers/config/conftest.py`:
- Around line 76-78: Update the fixture docstring to correctly reference
"mcp-catalog-sources" instead of "model-catalog-sources" to match the behavior
of get_mcp_catalog_sources() which patches DEFAULT_MCP_CATALOG_CM; locate the
class-scoped fixture in conftest.py (the one that calls
get_mcp_catalog_sources()) and change its descriptive text to mention
mcp-catalog-sources and that it patches three MCP catalog sources (two labeled
and one unlabeled) used for sourceLabel filtering tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 97a3999f-4a0c-4190-8992-9f6577e8c6b6

📥 Commits

Reviewing files that changed from the base of the PR and between 850de4b and 8ab3202.

📒 Files selected for processing (5)
  • tests/model_registry/conftest.py
  • tests/model_registry/constants.py
  • tests/model_registry/mcp_servers/config/conftest.py
  • tests/model_registry/mcp_servers/config/utils.py
  • tests/model_registry/mcp_servers/constants.py

@fege fege enabled auto-merge (squash) March 25, 2026 13:05
@fege fege merged commit 68bb285 into opendatahub-io:main Mar 25, 2026
11 checks passed
@github-actions
Copy link
Copy Markdown

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

@fege fege deleted the mcp_cm branch March 25, 2026 13:11
fege added a commit that referenced this pull request Mar 25, 2026
dbasunag pushed a commit that referenced this pull request Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants