Skip to content

Add test for oauth deployment of MR#321

Merged
lugi0 merged 7 commits intoopendatahub-io:mainfrom
lugi0:feat/mr-oauth-proxy
Jun 3, 2025
Merged

Add test for oauth deployment of MR#321
lugi0 merged 7 commits intoopendatahub-io:mainfrom
lugi0:feat/mr-oauth-proxy

Conversation

@lugi0
Copy link
Copy Markdown
Contributor

@lugi0 lugi0 commented May 29, 2025

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features

    • Added support for testing model registry creation and registration with OAuth proxy enabled.
    • Introduced a new test for registering and validating models using OAuth authentication.
  • Tests

    • Improved test setup by modularizing model registry instance creation and adding reusable fixtures.
    • Added constants for OAuth proxy configuration to streamline test configuration.
    • Updated import paths for better consistency and clarity in test files.
    • Enhanced flexibility in test client fixture to support dynamic service selection.
  • Chores

    • Updated a dependency version requirement for improved compatibility.

Signed-off-by: lugi0 <lgiorgi@redhat.com>
@lugi0 lugi0 requested review from dbasunag and fege May 29, 2025 15:19
@lugi0 lugi0 self-assigned this May 29, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented May 29, 2025

Walkthrough

The changes update the dependency version for "openshift-python-wrapper," refactor test fixtures for modular ModelRegistry instance creation, introduce OAuth proxy configuration support, and add a new test for OAuth-protected model registry creation. Several import statements for the ModelRegistry class are updated to use a more specific module path, and a new OAuth proxy configuration constant is added.

Changes

File(s) Change Summary
pyproject.toml Updated "openshift-python-wrapper" dependency version requirement.
tests/model_registry/conftest.py Refactored ModelRegistry fixture creation, added factory function, and introduced OAuth proxy fixture.
tests/model_registry/constants.py Added new constant dictionary for OAuth proxy configuration and standard labels.
tests/model_registry/python_client/test_model_registry_creation_oauth.py Added new test class for model registry creation and registration with OAuth proxy.
tests/model_registry/image_validation/test_verify_rhoai_images.py
tests/model_registry/negative_tests/test_model_registry_creation_negative.py
tests/model_registry/rbac/test_mr_rbac.py
tests/model_registry/rbac/utils.py
tests/model_registry/utils.py
Updated import statements for ModelRegistry class to use a more specific module path; added factory function in utils.py.

Sequence Diagram(s)

sequenceDiagram
    participant Test as Test Case
    participant Fixture as model_registry_instance_oauth_proxy
    participant Factory as create_model_registry_instance
    participant Registry as ModelRegistry
    participant Client as ModelRegistryClient
    participant Service as ModelRegistry Service

    Test->>Fixture: Request OAuth proxy ModelRegistry instance
    Fixture->>Factory: Call with OAuth proxy config
    Factory->>Registry: Create ModelRegistry (OAuth proxy enabled)
    Fixture-->>Test: Return ModelRegistry instance

    Test->>Service: Retrieve REST endpoint
    Test->>Client: Initialize with OAuth token
    Client->>Service: Register model
    Client->>Service: Retrieve registered model
    Test->>Test: Validate model attributes
Loading

Suggested labels

size/l, Utilities

Suggested reviewers

  • fege
  • dbasunag

Poem

A registry hops with OAuth in tow,
New fixtures and constants help tests to grow.
Imports refined, dependencies raised,
The code is now modular—rabbits amazed!
With tokens and proxies, the models are set,
Another fine hop in the testing vignette.
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@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', '/lgtm', '/wip', '/verified'}

@lugi0
Copy link
Copy Markdown
Contributor Author

lugi0 commented May 29, 2025

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented May 29, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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.

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/model_registry/python_client/test_model_registry_creation_oauth.py (1)

56-56: Consider more robust endpoint parsing.

The string split on ":" assumes the endpoint format is always "server:port". Consider using URL parsing for more robust handling of different endpoint formats.

-        server, port = rest_endpoint.split(":")
+        from urllib.parse import urlparse
+        parsed = urlparse(f"https://{rest_endpoint}")
+        server = parsed.hostname
+        port = parsed.port or 443
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a266fe3 and fac7bc2.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • pyproject.toml (1 hunks)
  • tests/model_registry/conftest.py (4 hunks)
  • tests/model_registry/constants.py (1 hunks)
  • tests/model_registry/image_validation/test_verify_rhoai_images.py (1 hunks)
  • tests/model_registry/negative_tests/test_model_registry_creation_negative.py (1 hunks)
  • tests/model_registry/python_client/test_model_registry_creation_oauth.py (1 hunks)
  • tests/model_registry/rbac/test_mr_rbac.py (1 hunks)
  • tests/model_registry/rbac/utils.py (1 hunks)
  • tests/model_registry/utils.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/model_registry/python_client/test_model_registry_creation_oauth.py (4)
utilities/constants.py (3)
  • DscComponents (146-164)
  • Protocols (87-94)
  • ManagementState (151-153)
tests/model_registry/utils.py (2)
  • get_endpoint_from_mr_service (49-53)
  • get_mr_service_by_label (22-46)
tests/conftest.py (2)
  • admin_client (53-54)
  • current_client_token (86-87)
tests/model_registry/conftest.py (2)
  • model_registry_namespace (55-56)
  • model_registry_instance_oauth_proxy (239-274)
tests/model_registry/conftest.py (2)
tests/conftest.py (1)
  • admin_client (53-54)
utilities/constants.py (2)
  • Annotations (124-139)
  • KubernetesIo (125-129)
🔇 Additional comments (14)
tests/model_registry/image_validation/test_verify_rhoai_images.py (1)

11-11: Update import path for ModelRegistry
The import has been correctly updated to the more specific module path (ocp_resources.model_registry_modelregistry_opendatahub_io) to align with the other test modules.

tests/model_registry/rbac/utils.py (1)

7-7: Align ModelRegistry import with refactoring
Good catch updating the ModelRegistry import to the new module path. This ensures consistency across the RBAC utility functions.

tests/model_registry/utils.py (1)

8-8: Standardize ModelRegistry import source
The import change matches the refactoring in other model registry tests and fixtures. No issues found.

tests/model_registry/negative_tests/test_model_registry_creation_negative.py (1)

6-6: Correct import for negative tests
The negative test now imports ModelRegistry from the precise module path, keeping it in sync with the rest of the suite.

pyproject.toml (1)

66-66: Verify dependency bump for openshift-python-wrapper
The version requirement has been updated from >=11.0.50 to >=11.0.57. Please confirm that this version exists on PyPI and is compatible with the new ModelRegistry import paths, and that no security advisories affect it.

tests/model_registry/rbac/test_mr_rbac.py (1)

12-12: LGTM! Import path update aligns with refactoring effort.

The import path update to use the more specific module ocp_resources.model_registry_modelregistry_opendatahub_io is consistent with similar changes across the test suite and improves import specificity.

tests/model_registry/constants.py (1)

32-36: LGTM! Well-structured OAuth proxy configuration.

The new OAUTH_PROXY_CONFIG_DICT constant provides clear configuration for OAuth proxy setup with appropriate port assignments (8443 for proxy, 443 for route) and service route enablement.

tests/model_registry/python_client/test_model_registry_creation_oauth.py (3)

66-76: LGTM! Comprehensive model registration test.

The model registration uses all the necessary parameters from the model configuration constants, providing thorough coverage of the registration functionality.


78-94: Excellent error handling and validation logic.

The model verification logic with comprehensive attribute checking and clear error reporting is well-implemented. The use of list comprehension for error collection and detailed failure messages will make debugging easier.


57-63:

Details

✅ Verification successful

Clarify the is_secure=False parameter usage.

The client configuration uses https:// protocol but sets is_secure=False. This seems contradictory and could be confusing. Please verify this is the intended configuration for the test environment.


🏁 Script executed:

#!/bin/bash
# Description: Check if other tests use similar configuration patterns

# Search for ModelRegistryClient instantiation patterns
rg -A 10 -B 2 "ModelRegistryClient.*is_secure.*False" --type py

Length of output: 65


🏁 Script executed:

#!/bin/bash
# Locate the ModelRegistryClient class definition
CLASS_FILE=$(rg -l "class ModelRegistryClient" --type py | head -n1)
echo "Class definition file: $CLASS_FILE"

# Show the class definition and its __init__ signature with context
rg -A 5 -B 2 "class ModelRegistryClient" "$CLASS_FILE"
rg -A 5 -B 2 "def __init__" "$CLASS_FILE"

# Search across the repo for any references to `is_secure` to see how it's handled
rg -n "is_secure" --type py

Length of output: 642


Confirmed test-specific configuration of is_secure=False
The ModelRegistryClient tests intentionally set is_secure=False to disable TLS certificate validation against self-signed or mock servers. This pattern is used elsewhere in our suite (e.g. tests/model_registry/conftest.py:370, and noted in tests/model_registry/rbac/utils.py), so no change is required here.

tests/model_registry/conftest.py (4)

12-12: LGTM! Import path consistency maintained.

The import update aligns with the refactoring effort across the test suite to use more specific module paths.


147-196: Excellent factory function design with comprehensive documentation.

The create_model_registry_instance factory function is well-designed with:

  • Clear parameter documentation
  • Sensible defaults for optional parameters
  • Proper return type annotation
  • Good separation of concerns

This refactoring improves code reusability and maintainability across different ModelRegistry configurations.


199-235: Good refactoring to use the factory function.

The fixture refactoring successfully extracts common configuration logic while maintaining the same functionality. The explicit definition of labels and mysql_config improves readability.


238-274:

Details

✅ Verification successful

Well-implemented OAuth proxy fixture.

The new fixture follows the same pattern as the existing fixture but configures OAuth proxy instead of Istio. The structure is consistent and the OAuth configuration is properly applied.

Note: Both fixtures use the same MR_INSTANCE_NAME. Ensure they are not used simultaneously in the same test session to avoid naming conflicts.


🏁 Script executed:

#!/bin/bash
# Description: Check if both fixtures are used in the same test files

# Search for tests that might use both fixtures
rg -l "model_registry_instance_oauth_proxy" --type py | xargs rg -l "model_registry_instance[^_]"

Length of output: 130


Approve OAuth proxy fixture

Verified that no tests import or use both model_registry_instance_oauth_proxy and the existing Istio-based fixture in the same session. The new fixture follows the established pattern and correctly applies the OAuth proxy configuration.

Signed-off-by: lugi0 <lgiorgi@redhat.com>
@lugi0 lugi0 marked this pull request as ready for review June 3, 2025 11:07
@lugi0 lugi0 requested a review from a team as a code owner June 3, 2025 11:07
@lugi0 lugi0 requested review from dbasunag and fege June 3, 2025 11:07
@lugi0
Copy link
Copy Markdown
Contributor Author

lugi0 commented Jun 3, 2025

/verified

@lugi0 lugi0 changed the title [WIP] Add test for oauth deployment of MR Add test for oauth deployment of MR Jun 3, 2025
lugi0 added 2 commits June 3, 2025 13:30
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
@rhods-ci-bot rhods-ci-bot removed Verified Verified pr in Jenkins commented-by-lugi0 labels Jun 3, 2025
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.

Actionable comments posted: 0

♻️ Duplicate comments (1)
tests/model_registry/utils.py (1)

269-318: 🛠️ Refactor suggestion

Address the function complexity issue from past reviews.

The factory function provides good modularity for creating ModelRegistry instances with OAuth proxy support. However, past reviews flagged the high number of parameters (previously 13, now 12), and static analysis still indicates too many arguments.

The previous suggestion to use a configuration dataclass or dictionary to reduce parameter complexity remains valid and should be addressed to improve maintainability.

Additionally, I notice there's a discrepancy in the docstring - line 287 still references admin_client parameter that was removed:

     Args:
-        admin_client: The Kubernetes client to use
         namespace: The namespace to create the model registry in
🧰 Tools
🪛 Pylint (3.3.7)

[refactor] 269-269: Too many arguments (12/5)

(R0913)


[refactor] 269-269: Too many positional arguments (12/5)

(R0917)

🧹 Nitpick comments (3)
tests/model_registry/python_client/test_model_registry_creation_oauth.py (3)

1-10: Consider improving import organization and adding module docstring.

The imports could be better organized following PEP 8 conventions, and the module is missing a docstring.

Apply this diff to improve import organization:

+"""Tests for OAuth proxy model registry creation and functionality."""
+
 import pytest
-from typing import Self
+from typing import Self
+
 from simple_logger.logger import get_logger
 from pytest_testconfig import config as py_config
 from semver import Version
+
 from utilities.infra import get_product_version
 from utilities.constants import DscComponents
 from tests.model_registry.constants import MODEL_NAME, MODEL_DICT
+
 from model_registry import ModelRegistry as ModelRegistryClient
🧰 Tools
🪛 Pylint (3.3.7)

[convention] 1-1: Missing module docstring

(C0114)


[error] 3-3: Unable to import 'simple_logger.logger'

(E0401)


[error] 4-4: Unable to import 'pytest_testconfig'

(E0401)


[error] 5-5: Unable to import 'semver'

(E0401)


[error] 9-9: Unable to import 'model_registry'

(E0401)


[convention] 2-2: standard import "typing.Self" should be placed before third party import "pytest"

(C0411)


[convention] 9-9: third party import "model_registry.ModelRegistry" should be placed before first party imports "utilities.infra.get_product_version", "utilities.constants.DscComponents", "tests.model_registry.constants.MODEL_NAME"

(C0411)


46-47: LGTM: Version comparison logic is now correct.

The version check logic has been fixed from the previous review. It now correctly skips the test when the product version is less than the minimum version (2.21.0) for downstream distributions.

However, the line is quite long (113 characters). Consider breaking it for better readability:

-        if py_config["distribution"] == "downstream" and get_product_version(admin_client=admin_client) < MINVER:
+        if (py_config["distribution"] == "downstream" and 
+            get_product_version(admin_client=admin_client) < MINVER):
🧰 Tools
🪛 Pylint (3.3.7)

[convention] 46-46: Line too long (113/100)

(C0301)


49-77: LGTM: Comprehensive model registration and validation test.

The test method properly:

  • Registers a model with comprehensive attributes using the OAuth proxy-enabled client
  • Retrieves and validates the registered model
  • Provides clear error reporting with detailed attribute mismatches

The test validates the core OAuth proxy functionality for model registry operations.

Note: Line 77 is slightly over the line length limit (108 characters) but the error reporting logic is clear and functional.

🧰 Tools
🪛 Pylint (3.3.7)

[convention] 77-77: Line too long (108/100)

(C0301)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 29847f9 and 7c58103.

📒 Files selected for processing (2)
  • tests/model_registry/python_client/test_model_registry_creation_oauth.py (1 hunks)
  • tests/model_registry/utils.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/python_client/test_model_registry_creation_oauth.py (3)
utilities/infra.py (1)
  • get_product_version (747-770)
utilities/constants.py (2)
  • DscComponents (146-164)
  • ManagementState (151-153)
tests/model_registry/conftest.py (1)
  • model_registry_client (332-361)
🪛 Pylint (3.3.7)
tests/model_registry/python_client/test_model_registry_creation_oauth.py

[convention] 46-46: Line too long (113/100)

(C0301)


[convention] 77-77: Line too long (108/100)

(C0301)


[convention] 1-1: Missing module docstring

(C0114)


[error] 3-3: Unable to import 'simple_logger.logger'

(E0401)


[error] 4-4: Unable to import 'pytest_testconfig'

(E0401)


[error] 5-5: Unable to import 'semver'

(E0401)


[error] 9-9: Unable to import 'model_registry'

(E0401)


[convention] 41-41: Missing function or method docstring

(C0116)


[refactor] 34-34: Too few public methods (1/2)

(R0903)


[convention] 2-2: standard import "typing.Self" should be placed before third party import "pytest"

(C0411)


[convention] 9-9: third party import "model_registry.ModelRegistry" should be placed before first party imports "utilities.infra.get_product_version", "utilities.constants.DscComponents", "tests.model_registry.constants.MODEL_NAME"

(C0411)

tests/model_registry/utils.py

[error] 4-4: Unable to import 'kubernetes.dynamic'

(E0401)


[error] 5-5: Unable to import 'ocp_resources.namespace'

(E0401)


[error] 6-6: Unable to import 'ocp_resources.pod'

(E0401)


[error] 7-7: Unable to import 'ocp_resources.service'

(E0401)


[error] 8-8: Unable to import 'ocp_resources.model_registry_modelregistry_opendatahub_io'

(E0401)


[refactor] 269-269: Too many arguments (12/5)

(R0913)


[refactor] 269-269: Too many positional arguments (12/5)

(R0917)

🔇 Additional comments (4)
tests/model_registry/utils.py (2)

2-2: LGTM: Import addition looks good.

The addition of Any and Dict type hints from typing module supports the type annotations in the new factory function.


8-8: LGTM: Import path updated to more specific module.

The import path change from ocp_resources.model_registry to ocp_resources.model_registry_modelregistry_opendatahub_io aligns with the more specific module structure mentioned in the AI summary.

🧰 Tools
🪛 Pylint (3.3.7)

[error] 8-8: Unable to import 'ocp_resources.model_registry_modelregistry_opendatahub_io'

(E0401)

tests/model_registry/python_client/test_model_registry_creation_oauth.py (2)

15-32: LGTM: Test parameterization with OAuth proxy configuration.

The parametrized test setup properly configures the DSC component with OAuth proxy service fixture. This addresses the previous review comment about using fixtures appropriately.


34-38: LGTM: Test class with appropriate JIRA reference.

The test class includes the JIRA ID as requested in previous reviews, and the docstring clearly describes the purpose.

🧰 Tools
🪛 Pylint (3.3.7)

[refactor] 34-34: Too few public methods (1/2)

(R0903)

Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag left a comment

Choose a reason for hiding this comment

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

Other than the smoke marker and skip, this PR is good from my side.

Signed-off-by: lugi0 <lgiorgi@redhat.com>
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag left a comment

Choose a reason for hiding this comment

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

Based on chats resolving open comments.

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.

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/model_registry/conftest.py (3)

189-200: MySQL config consolidation addresses past feedback.

This fixture consolidates the previously separate database-related fixtures into a single configuration dictionary, which aligns with past review suggestions about making this a constant or consolidated fixture rather than multiple separate ones.

However, consider adding a docstring as indicated by the static analysis warning:

 @pytest.fixture(scope="class")
 def model_registry_mysql_config(
     model_registry_db_deployment: Deployment, model_registry_db_secret: Secret
 ) -> dict[str, Any]:
+    """
+    Creates MySQL configuration dictionary for model registry instances.
+    
+    Args:
+        model_registry_db_deployment: The MySQL deployment fixture
+        model_registry_db_secret: The MySQL secret fixture
+        
+    Returns:
+        dict[str, Any]: MySQL configuration parameters
+    """
     return {
🧰 Tools
🪛 Pylint (3.3.7)

[convention] 194-194: Line too long (114/100)

(C0301)


[convention] 190-190: Missing function or method docstring

(C0116)


[warning] 191-191: Redefining name 'model_registry_db_deployment' from outer scope (line 122)

(W0621)


[warning] 191-191: Redefining name 'model_registry_db_secret' from outer scope (line 106)

(W0621)


194-194: Fix line length issue.

The line exceeds the 100-character limit as flagged by static analysis.

-        "host": f"{model_registry_db_deployment.name}.{model_registry_db_deployment.namespace}.svc.cluster.local",
+        "host": (
+            f"{model_registry_db_deployment.name}."
+            f"{model_registry_db_deployment.namespace}.svc.cluster.local"
+        ),
🧰 Tools
🪛 Pylint (3.3.7)

[convention] 194-194: Line too long (114/100)

(C0301)


341-341: Fix line length issue.

Another line length violation that should be addressed.

-    service_fixture = getattr(request, "param", {}).get("service_fixture", "model_registry_instance_service")
+    service_fixture = getattr(request, "param", {}).get(
+        "service_fixture", "model_registry_instance_service"
+    )
🧰 Tools
🪛 Pylint (3.3.7)

[convention] 341-341: Line too long (109/100)

(C0301)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7c58103 and 746630e.

📒 Files selected for processing (1)
  • tests/model_registry/conftest.py (5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/conftest.py (3)
tests/model_registry/utils.py (3)
  • create_model_registry_instance (269-317)
  • get_mr_service_by_label (22-46)
  • get_endpoint_from_mr_service (49-53)
utilities/constants.py (2)
  • Protocols (87-94)
  • DscComponents (146-164)
tests/conftest.py (2)
  • admin_client (53-54)
  • current_client_token (86-87)
🪛 Pylint (3.3.7)
tests/model_registry/conftest.py

[error] 12-12: Unable to import 'ocp_resources.model_registry_modelregistry_opendatahub_io'

(E0401)


[convention] 44-44: Imports from package utilities are not grouped

(C0412)


[convention] 194-194: Line too long (114/100)

(C0301)


[warning] 152-152: Redefining name 'model_registry_mysql_config' from outer scope (line 190)

(W0621)


[warning] 171-171: Redefining name 'model_registry_namespace' from outer scope (line 57)

(W0621)


[warning] 172-172: Redefining name 'model_registry_mysql_config' from outer scope (line 190)

(W0621)


[convention] 190-190: Missing function or method docstring

(C0116)


[warning] 191-191: Redefining name 'model_registry_db_deployment' from outer scope (line 122)

(W0621)


[warning] 191-191: Redefining name 'model_registry_db_secret' from outer scope (line 106)

(W0621)


[warning] 206-206: Redefining name 'model_registry_namespace' from outer scope (line 57)

(W0621)


[warning] 207-207: Redefining name 'model_registry_instance' from outer scope (line 150)

(W0621)


[warning] 228-228: Redefining name 'model_registry_namespace' from outer scope (line 57)

(W0621)


[warning] 229-229: Redefining name 'model_registry_instance_oauth_proxy' from outer scope (line 170)

(W0621)


[warning] 249-249: Redefining name 'model_registry_instance_service' from outer scope (line 204)

(W0621)


[convention] 341-341: Line too long (109/100)

(C0301)

🔇 Additional comments (7)
tests/model_registry/conftest.py (7)

12-12: Import path update looks good.

The change to use a more specific import path ocp_resources.model_registry_modelregistry_opendatahub_io improves clarity and aligns with the pattern used in other test modules.

🧰 Tools
🪛 Pylint (3.3.7)

[error] 12-12: Unable to import 'ocp_resources.model_registry_modelregistry_opendatahub_io'

(E0401)


32-34: OAuth proxy constants properly imported.

The addition of OAUTH_PROXY_CONFIG_DICT and MODEL_REGISTRY_STANDARD_LABELS constants aligns with the OAuth proxy functionality being introduced and follows the pattern of using constants instead of fixtures as suggested in past reviews.


42-42: Factory function import supports modularization.

The import of create_model_registry_instance from utils aligns with the past review feedback suggesting to move functions to utils.py and promotes better code organization.


150-166: Excellent refactoring using factory function.

The refactoring to use create_model_registry_instance from utils.py successfully addresses the past review feedback about moving functions to utils.py. The fixture now has a clear purpose and proper documentation.

🧰 Tools
🪛 Pylint (3.3.7)

[warning] 151-151: Redefining name 'model_registry_namespace' from outer scope (line 57)

(W0621)


[warning] 152-152: Redefining name 'model_registry_mysql_config' from outer scope (line 190)

(W0621)


169-186: OAuth proxy fixture implementation looks correct.

The new model_registry_instance_oauth_proxy fixture properly implements OAuth proxy configuration using the imported constants. The structure mirrors the regular instance fixture with appropriate OAuth-specific configuration.

🧰 Tools
🪛 Pylint (3.3.7)

[warning] 171-171: Redefining name 'model_registry_namespace' from outer scope (line 57)

(W0621)


[warning] 172-172: Redefining name 'model_registry_mysql_config' from outer scope (line 190)

(W0621)


225-244: OAuth service fixture complements OAuth proxy support.

The new model_registry_instance_oauth_service fixture properly supports the OAuth proxy functionality with comprehensive documentation following the same pattern as the regular service fixture.

🧰 Tools
🪛 Pylint (3.3.7)

[warning] 228-228: Redefining name 'model_registry_namespace' from outer scope (line 57)

(W0621)


[warning] 229-229: Redefining name 'model_registry_instance_oauth_proxy' from outer scope (line 170)

(W0621)


328-357:

Details

✅ Verification successful

Dynamic service selection is clever but verify complexity trade-offs.

The refactored model_registry_client fixture introduces dynamic service fixture selection, which is flexible and supports both regular and OAuth proxy instances. However, this approach introduces some complexity.

The dynamic fixture selection using request.getfixturevalue() is powerful but may make test dependencies less explicit. Verify that this pattern is consistently used across the test suite and consider if the added complexity is justified:


🏁 Script executed:

#!/bin/bash
# Description: Check usage patterns of the model_registry_client fixture to ensure consistency
# Expected: Find all test files that use this fixture and verify the service_fixture parameter usage

echo "=== Searching for model_registry_client fixture usage ==="
rg -A 3 -B 3 "model_registry_client" --type py

echo -e "\n=== Searching for service_fixture parameter usage ==="
rg -A 2 -B 2 "service_fixture" --type py

echo -e "\n=== Checking for other fixtures using getfixturevalue pattern ==="
rg -A 2 -B 2 "getfixturevalue" --type py

Length of output: 7976


Dynamic service fixture selection is consistent across tests
The use of request.getfixturevalue() to choose between regular and OAuth proxy service fixtures mirrors patterns already in place (e.g., in tests/conftest.py and tests/model_serving/model_server/conftest.py), and is exercised in both test_model_registry_creation.py and test_model_registry_creation_oauth.py. The added flexibility aligns with existing dynamic-fixture conventions in this suite, so no further changes are needed.

🧰 Tools
🪛 Pylint (3.3.7)

[convention] 341-341: Line too long (109/100)

(C0301)

Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

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

/lgtm

@lugi0 lugi0 enabled auto-merge (squash) June 3, 2025 15:53
@lugi0 lugi0 merged commit a21ceb3 into opendatahub-io:main Jun 3, 2025
8 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 3, 2025

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

adolfo-ab pushed a commit to adolfo-ab/opendatahub-tests that referenced this pull request Jun 11, 2025
* feat: Add test for oauth deployment of MR

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* feat: refactor and address comments

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: inverted version comparison

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: remove unused fixture from function parameter

Signed-off-by: lugi0 <lgiorgi@redhat.com>

* fix: fix mipy failure in pre-commit

Signed-off-by: lugi0 <lgiorgi@redhat.com>

---------

Signed-off-by: lugi0 <lgiorgi@redhat.com>
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