test: add test to initiate mr with secure db connection#320
test: add test to initiate mr with secure db connection#320fege merged 34 commits intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update introduces secure MySQL SSL/TLS integration testing for the Model Registry, including new constants, certificate and secret utilities, and comprehensive pytest fixtures for deploying and validating Model Registry behavior with both valid and invalid CA certificates. It also adds new tests, updates configuration fixtures, and enhances certificate management utilities. Changes
Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
Documentation and Community
|
|
/wip |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/hold', '/wip', '/verified'} |
|
@coderabbitai fullreview |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@fege I'll conduct a full review of this PR. Let me first gather information about the changes made. 🏁 Script executed: Length of output: 252 Let me check the branch structure and find the correct way to get the PR changes. 🏁 Script executed: Length of output: 629 Now I can see the branch structure. Let me get the changes between main and the current PR branch. 🏁 Script executed: Length of output: 94943 @fege I've completed a comprehensive review of this PR. Here are my findings: 🔍 Summary of ChangesThis PR includes several significant modifications:
📝 Detailed Review✅ Positive Changes1. CI/CD Workflow Fix # .github/workflows/verify_build_container.yml
+ - uses: actions/checkout@v4Good fix - adds missing checkout step before the existing checkout with specific configuration. 2. Model Registry Security Enhancement
|
dbasunag
left a comment
dbasunag
left a comment
There was a problem hiding this comment.
I started reviewing before noticing it is draft and I feel giving this reviews now, would help with moving the PR fast. Let me know if you disagree, then I will totally stay off till you indicate otherwise.
yes that was exactly my intention, to get early reviews to avoid to take a "wrong" path in the solution 👍 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/model_registry/rest_api/test_model_registry_secure_db.py (1)
74-95: LGTM! Comprehensive secure DB validation with verification issue.The test properly validates successful model registration with secure database connections using valid CA certificates.
The assertion on line 92 only validates the ID field as noted in previous reviews. This is consistent with the existing codebase approach.
Fix line length issues:
""" - Deploys Model Registry with a secure MySQL DB (SSL/TLS), registers a model, and checks functionality. - Uses a CA bundle file for SSL verification by passing it directly to the verify parameter. + Deploys Model Registry with a secure MySQL DB (SSL/TLS), registers a model, + and checks functionality. Uses a CA bundle file for SSL verification by + passing it directly to the verify parameter. """ model_name = generate_random_name(prefix="model-rest-api") MODEL_REGISTER_DATA["register_model_data"]["name"] = model_name @@ -89,6 +89,9 @@ data_dict=MODEL_REGISTER_DATA, verify=local_ca_bundle, ) - assert result["register_model"].get("id"), "Model registration failed with secure DB connection." + assert result["register_model"].get( + "id" + ), "Model registration failed with secure DB connection." for k, v in MODEL_REGISTER_DATA["register_model_data"].items(): assert result["register_model"][k] == v, f"Expected {k}={v}, got {result[k]}" - LOGGER.info(f"Model registered successfully with secure DB using {local_ca_bundle}") + LOGGER.info( + f"Model registered successfully with secure DB using {local_ca_bundle}" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 81-81: Line too long (109/100)
(C0301)
[convention] 92-92: Line too long (105/100)
(C0301)
🧹 Nitpick comments (7)
tests/model_registry/utils.py (2)
13-13: Fix import organization and order.The static analysis identifies several import issues that should be addressed:
- Line 13 is too long (115 characters)
- Import order is incorrect - third-party imports should come before first-party imports
- Imports from the same package should be grouped together
Apply this diff to fix the import organization:
-from tests.model_registry.constants import MR_DB_IMAGE_DIGEST, ISTIO_CONFIG_DICT, DB_RESOURCES_NAME, SECURE_MR_NAME +from ocp_resources.secret import Secret +from tests.model_registry.constants import ( + MR_DB_IMAGE_DIGEST, + ISTIO_CONFIG_DICT, + DB_RESOURCES_NAME, + SECURE_MR_NAME, +) from utilities.exceptions import ProtocolNotSupportedError, TooManyServicesError from utilities.constants import Protocols, Annotations -from ocp_resources.secret import SecretAlso applies to: 16-16
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 13-13: Line too long (115/100)
(C0301)
270-302: LGTM! Well-structured utility function with minor formatting issue.The
create_secure_model_registryfunction is well-designed and properly constructs a ModelRegistry resource with secure MySQL configuration. The function signature and implementation are clear and appropriate.Fix the line length issue on line 293:
- "host": f"{model_registry_db_service.name}.{model_registry_db_service.namespace}.svc.cluster.local", + "host": ( + f"{model_registry_db_service.name}." + f"{model_registry_db_service.namespace}.svc.cluster.local" + ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 293-293: Line too long (112/100)
(C0301)
tests/model_registry/rest_api/utils.py (2)
32-35: LGTM! SSL verification enhancement with minor documentation issue.The addition of the
verifyparameter to enable configurable SSL certificate verification is well-implemented and necessary for the secure database testing functionality.Add a docstring to maintain consistency with other functions in the module:
def execute_model_registry_post_command( url: str, headers: dict[str, str], data_json: dict[str, Any], verify: bool | str = False ) -> dict[Any, Any]: + """ + Execute a POST request to the Model Registry API. + + Args: + url: The API endpoint URL + headers: HTTP headers for the request + data_json: JSON data to send in the request body + verify: SSL certificate verification (bool or path to CA bundle) + + Returns: + The JSON response as a dictionary + """🧰 Tools
🪛 Pylint (3.3.7)
[convention] 32-32: Missing function or method docstring
(C0116)
64-96: LGTM! Proper parameter propagation with minor formatting issue.The
verifyparameter is correctly propagated through all the POST calls in theregister_model_rest_apifunction, enabling end-to-end SSL verification control.Fix the line length issue on line 88:
- url=f"{model_registry_rest_url}{MODEL_REGISTRY_BASE_URI}model_versions/{model_version['id']}/artifacts", + url=( + f"{model_registry_rest_url}{MODEL_REGISTRY_BASE_URI}" + f"model_versions/{model_version['id']}/artifacts" + ),🧰 Tools
🪛 Pylint (3.3.7)
[convention] 88-88: Line too long (112/100)
(C0301)
[convention] 64-64: Missing function or method docstring
(C0116)
tests/model_registry/rest_api/test_model_registry_secure_db.py (2)
1-11: Add module docstring and fix import organization.The module is missing a docstring and has import order issues that should be addressed for consistency.
Apply this diff to add documentation and fix imports:
+""" +Test module for Model Registry functionality with secure MySQL database connections. + +This module contains tests that validate SSL/TLS connectivity scenarios including +both valid and invalid CA certificate configurations. +""" +from typing import Self + import pytest import requests -from typing import Self + from tests.model_registry.rest_api.utils import register_model_rest_api from tests.model_registry.rest_api.constants import MODEL_REGISTER_DATA +from tests.model_registry.utils import generate_random_name from utilities.constants import DscComponents + from simple_logger.logger import get_logger -from tests.model_registry.utils import generate_random_name🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 7-7: Unable to import 'simple_logger.logger'
(E0401)
[convention] 3-3: standard import "typing.Self" should be placed before third party imports "pytest", "requests"
(C0411)
[convention] 7-7: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.utils.register_model_rest_api", "tests.model_registry.rest_api.constants.MODEL_REGISTER_DATA", "utilities.constants.DscComponents"
(C0411)
[convention] 8-8: Imports from package tests are not grouped
(C0412)
44-65: LGTM! Robust SSL error validation with minor issues.The test correctly validates that SSL certificate verification fails with an invalid CA, and the assertion properly checks for the expected error message.
Address the unused parameter warning and line length:
def test_register_model_with_invalid_ca( self: Self, - patch_invalid_ca: dict[str, str], + patch_invalid_ca: dict[str, str], # pylint: disable=unused-argument model_registry_rest_url: str, model_registry_rest_headers: dict[str, str], ) -> None: """ - Test that model registration fails with an SSLError when the Model Registry is deployed - with an invalid CA certificate. + Test that model registration fails with an SSLError when the Model Registry + is deployed with an invalid CA certificate. """Note: The
patch_invalid_caparameter is needed for the fixture to execute even if not directly used in the test body.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 46-46: Unused argument 'patch_invalid_ca'
(W0613)
tests/model_registry/rest_api/conftest.py (1)
16-16: Remove unused logger import and variable.The
get_loggerimport andLOGGERvariable are not used anywhere in this file.Apply this diff to remove the unused logger:
-from simple_logger.logger import get_logger from ocp_resources.model_registry import ModelRegistry -LOGGER = get_logger(name=__name__) -Also applies to: 20-20
🧰 Tools
🪛 Pylint (3.3.7)
[error] 16-16: Unable to import 'simple_logger.logger'
(E0401)
[convention] 16-16: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/constants.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(1 hunks)tests/model_registry/rest_api/utils.py(3 hunks)tests/model_registry/utils.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/model_registry/utils.py (2)
utilities/constants.py (2)
Annotations(124-139)KubernetesIo(125-129)tests/model_registry/conftest.py (3)
model_registry_namespace(54-55)model_registry_db_service(59-84)model_registry_db_secret(103-115)
tests/model_registry/rest_api/utils.py (1)
tests/model_registry/rest_api/conftest.py (2)
model_registry_rest_url(24-26)model_registry_rest_headers(30-35)
🪛 Pylint (3.3.7)
tests/model_registry/utils.py
[convention] 13-13: Line too long (115/100)
(C0301)
[error] 16-16: Unable to import 'ocp_resources.secret'
(E0401)
[convention] 16-16: third party import "ocp_resources.secret.Secret" should be placed before first party imports "tests.model_registry.constants.MR_DB_IMAGE_DIGEST", "utilities.exceptions.ProtocolNotSupportedError", "utilities.constants.Protocols"
(C0411)
[convention] 16-16: Imports from package ocp_resources are not grouped
(C0412)
[convention] 293-293: Line too long (112/100)
(C0301)
tests/model_registry/rest_api/utils.py
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 88-88: Line too long (112/100)
(C0301)
tests/model_registry/rest_api/test_model_registry_secure_db.py
[convention] 33-33: Line too long (109/100)
(C0301)
[convention] 81-81: Line too long (109/100)
(C0301)
[convention] 92-92: Line too long (105/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 7-7: Unable to import 'simple_logger.logger'
(E0401)
[warning] 46-46: Unused argument 'patch_invalid_ca'
(W0613)
[convention] 3-3: standard import "typing.Self" should be placed before third party imports "pytest", "requests"
(C0411)
[convention] 7-7: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.utils.register_model_rest_api", "tests.model_registry.rest_api.constants.MODEL_REGISTER_DATA", "utilities.constants.DscComponents"
(C0411)
[convention] 8-8: Imports from package tests are not grouped
(C0412)
tests/model_registry/rest_api/conftest.py
[convention] 7-7: Line too long (109/100)
(C0301)
[convention] 11-11: Line too long (112/100)
(C0301)
[convention] 12-12: Line too long (108/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 9-9: Unable to import 'ocp_resources.deployment'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.service'
(E0401)
[error] 13-13: Unable to import 'ocp_resources.resource'
(E0401)
[error] 14-14: Unable to import 'ocp_resources.secret'
(E0401)
[error] 15-15: Unable to import 'ocp_resources.config_map'
(E0401)
[error] 16-16: Unable to import 'simple_logger.logger'
(E0401)
[error] 17-17: Unable to import 'ocp_resources.model_registry'
(E0401)
[convention] 9-9: third party import "ocp_resources.deployment.Deployment" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols"
(C0411)
[convention] 10-10: third party import "ocp_resources.service.Service" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols"
(C0411)
[convention] 13-13: third party import "ocp_resources.resource.ResourceEditor" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 14-14: third party import "ocp_resources.secret.Secret" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 15-15: third party import "ocp_resources.config_map.ConfigMap" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 16-16: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 17-17: third party import "ocp_resources.model_registry.ModelRegistry" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 11-11: Imports from package tests are not grouped
(C0412)
[convention] 13-13: Imports from package ocp_resources are not grouped
(C0412)
[convention] 17-17: Imports from package ocp_resources are not grouped
(C0412)
[convention] 77-77: Line too long (106/100)
(C0301)
[convention] 124-124: Line too long (104/100)
(C0301)
[convention] 138-138: Line too long (115/100)
(C0301)
[convention] 154-154: Line too long (109/100)
(C0301)
[convention] 156-156: Line too long (120/100)
(C0301)
[convention] 168-168: Line too long (109/100)
(C0301)
[convention] 179-179: Line too long (112/100)
(C0301)
[warning] 99-99: Redefining name 'patch_invalid_ca' from outer scope (line 66)
(W0621)
[warning] 164-164: Using open without explicitly specifying an encoding
(W1514)
[warning] 177-177: Catching too general exception Exception
(W0718)
[warning] 172-172: Using open without explicitly specifying an encoding
(W1514)
[warning] 175-175: Using open without explicitly specifying an encoding
(W1514)
🔇 Additional comments (4)
tests/model_registry/constants.py (1)
47-50: LGTM! Well-defined constants for secure configuration.The new constants are appropriately named and will help maintain consistency across the secure Model Registry test infrastructure. The file path construction using f-string is clean and readable.
tests/model_registry/rest_api/conftest.py (3)
65-92: Well-structured fixture for testing invalid CA scenarios.The
patch_invalid_cafixture is properly implemented with:
- Parametrizable CA ConfigMap and file names
- Clean resource management using ResourceEditor
- Appropriate yielding of the CA file path for test consumption
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 77-77: Line too long (106/100)
(C0301)
94-112: Clean implementation for Model Registry with custom CA.The fixture properly leverages the
patch_invalid_cafixture to create a Model Registry instance configured with a custom CA certificate.🧰 Tools
🪛 Pylint (3.3.7)
[warning] 99-99: Redefining name 'patch_invalid_ca' from outer scope (line 66)
(W0621)
114-151: Comprehensive fixture for secure MySQL and Model Registry setup.Excellent implementation that:
- Properly configures MySQL with SSL/TLS by adding the
--ssl-caargument- Correctly mounts the CA ConfigMap in the MySQL deployment
- Uses nested context managers for clean resource management
- Ensures Model Registry is available before yielding
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 124-124: Line too long (104/100)
(C0301)
[convention] 138-138: Line too long (115/100)
(C0301)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/model_registry/rest_api/conftest.py (2)
195-195: Add explicit encoding to file operations.File operations should specify encoding explicitly to avoid platform-dependent behavior.
Apply this diff to add explicit encoding:
- with open(ca_bundle_path, "w") as f: + with open(ca_bundle_path, "w", encoding="utf-8") as f: f.write(ca_bundle_content) try: router_secret = Secret(client=admin_client, name="router-ca", namespace="openshift-ingress-operator") router_ca_b64 = router_secret.instance.data.get("tls.crt") if router_ca_b64: router_ca_content = base64.b64decode(router_ca_b64).decode("utf-8") - with open(ca_bundle_path, "r") as bundle: + with open(ca_bundle_path, "r", encoding="utf-8") as bundle: bundle_content = bundle.read() if router_ca_content not in bundle_content: - with open(ca_bundle_path, "a") as bundle_append: + with open(ca_bundle_path, "a", encoding="utf-8") as bundle_append: bundle_append.write("\n" + router_ca_content)Also applies to: 203-203, 206-206
🧰 Tools
🪛 Pylint (3.3.7)
[warning] 195-195: Using open without explicitly specifying an encoding
(W1514)
208-211: Improve exception handling specificity.Catching generic
Exceptionis too broad and might hide unexpected errors. Consider catching more specific exceptions or logging the error before failing.Apply this diff to improve exception handling:
- except Exception: + except Exception as e: + LOGGER.warning(f"Failed to append router CA: {e}") pytest.fail( "router-ca secret not found in openshift-ingress-operator. Proceeding without appending ingress CA." )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 210-210: Line too long (112/100)
(C0301)
[warning] 208-208: Catching too general exception Exception
(W0718)
🧹 Nitpick comments (2)
tests/model_registry/utils.py (1)
13-13: Consider reordering imports to follow PEP 8 conventions.The imports should be grouped: standard library, third-party packages, then local imports. The
Secretimport is currently mixed with local imports.Apply this diff to improve import organization:
from kubernetes.dynamic import DynamicClient from ocp_resources.namespace import Namespace from ocp_resources.pod import Pod from ocp_resources.service import Service +from ocp_resources.secret import Secret from ocp_resources.model_registry_modelregistry_opendatahub_io import ModelRegistry from kubernetes.dynamic.exceptions import ResourceNotFoundError from simple_logger.logger import get_logger from timeout_sampler import TimeoutExpiredError, TimeoutSampler from kubernetes.dynamic.exceptions import NotFoundError from tests.model_registry.constants import MR_DB_IMAGE_DIGEST, DB_RESOURCES_NAME from utilities.exceptions import ProtocolNotSupportedError, TooManyServicesError from utilities.constants import Protocols, Annotations -from ocp_resources.secret import SecretAlso applies to: 16-16
tests/model_registry/rest_api/conftest.py (1)
1-29: Organize imports following PEP 8 conventions.The imports should be grouped and ordered: standard library, third-party packages, then local imports. Currently, third-party imports are mixed with local imports.
Apply this diff to improve import organization:
-from typing import Any, Generator import os import base64 +from typing import Any, Generator + from kubernetes.dynamic import DynamicClient +from ocp_resources.config_map import ConfigMap +from ocp_resources.deployment import Deployment +from ocp_resources.model_registry import ModelRegistry +from ocp_resources.resource import ResourceEditor +from ocp_resources.secret import Secret +from ocp_resources.service import Service import pytest +from simple_logger.logger import get_logger + from tests.model_registry.rest_api.constants import MODEL_REGISTRY_BASE_URI from tests.model_registry.rest_api.utils import register_model_rest_api, execute_model_registry_patch_command -from utilities.constants import Protocols -from ocp_resources.deployment import Deployment -from ocp_resources.service import Service from tests.model_registry.utils import ( get_model_registry_deployment_template_dict, create_model_registry_instance, make_mysql_config, ) from tests.model_registry.constants import ( DB_RESOURCES_NAME, CA_MOUNT_PATH, CA_FILE_PATH, CA_CONFIGMAP_NAME, SECURE_MR_NAME, ISTIO_CONFIG_DICT, MODEL_REGISTRY_STANDARD_LABELS, ) -from ocp_resources.resource import ResourceEditor -from ocp_resources.secret import Secret -from ocp_resources.config_map import ConfigMap -from simple_logger.logger import get_logger -from ocp_resources.model_registry import ModelRegistry +from utilities.constants import Protocols🧰 Tools
🪛 Pylint (3.3.7)
[convention] 7-7: Line too long (109/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 9-9: Unable to import 'ocp_resources.deployment'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.service'
(E0401)
[error] 25-25: Unable to import 'ocp_resources.resource'
(E0401)
[error] 26-26: Unable to import 'ocp_resources.secret'
(E0401)
[error] 27-27: Unable to import 'ocp_resources.config_map'
(E0401)
[error] 28-28: Unable to import 'simple_logger.logger'
(E0401)
[error] 29-29: Unable to import 'ocp_resources.model_registry'
(E0401)
[convention] 9-9: third party import "ocp_resources.deployment.Deployment" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols"
(C0411)
[convention] 10-10: third party import "ocp_resources.service.Service" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols"
(C0411)
[convention] 25-25: third party import "ocp_resources.resource.ResourceEditor" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 26-26: third party import "ocp_resources.secret.Secret" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 27-27: third party import "ocp_resources.config_map.ConfigMap" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 28-28: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 29-29: third party import "ocp_resources.model_registry.ModelRegistry" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 11-15: Imports from package tests are not grouped
(C0412)
[convention] 25-25: Imports from package ocp_resources are not grouped
(C0412)
[convention] 29-29: Imports from package ocp_resources are not grouped
(C0412)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/model_registry/constants.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/utils.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/model_registry/constants.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/model_registry/utils.py (2)
utilities/exceptions.py (2)
ProtocolNotSupportedError(8-13)TooManyServicesError(16-21)utilities/constants.py (2)
Protocols(87-94)Annotations(124-139)
🪛 Pylint (3.3.7)
tests/model_registry/rest_api/conftest.py
[convention] 7-7: Line too long (109/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 9-9: Unable to import 'ocp_resources.deployment'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.service'
(E0401)
[error] 25-25: Unable to import 'ocp_resources.resource'
(E0401)
[error] 26-26: Unable to import 'ocp_resources.secret'
(E0401)
[error] 27-27: Unable to import 'ocp_resources.config_map'
(E0401)
[error] 28-28: Unable to import 'simple_logger.logger'
(E0401)
[error] 29-29: Unable to import 'ocp_resources.model_registry'
(E0401)
[convention] 9-9: third party import "ocp_resources.deployment.Deployment" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols"
(C0411)
[convention] 10-10: third party import "ocp_resources.service.Service" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols"
(C0411)
[convention] 25-25: third party import "ocp_resources.resource.ResourceEditor" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 26-26: third party import "ocp_resources.secret.Secret" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 27-27: third party import "ocp_resources.config_map.ConfigMap" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 28-28: third party import "simple_logger.logger.get_logger" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 29-29: third party import "ocp_resources.model_registry.ModelRegistry" should be placed before first party imports "tests.model_registry.rest_api.constants.MODEL_REGISTRY_BASE_URI", "tests.model_registry.rest_api.utils.register_model_rest_api", "utilities.constants.Protocols", "tests.model_registry.utils.get_model_registry_deployment_template_dict", "tests.model_registry.constants.DB_RESOURCES_NAME"
(C0411)
[convention] 11-15: Imports from package tests are not grouped
(C0412)
[convention] 25-25: Imports from package ocp_resources are not grouped
(C0412)
[convention] 29-29: Imports from package ocp_resources are not grouped
(C0412)
[convention] 89-89: Line too long (106/100)
(C0301)
[convention] 146-146: Line too long (104/100)
(C0301)
[convention] 160-160: Line too long (115/100)
(C0301)
[convention] 185-185: Line too long (109/100)
(C0301)
[convention] 187-187: Line too long (120/100)
(C0301)
[convention] 199-199: Line too long (109/100)
(C0301)
[convention] 210-210: Line too long (112/100)
(C0301)
[warning] 111-111: Redefining name 'patch_invalid_ca' from outer scope (line 78)
(W0621)
[warning] 195-195: Using open without explicitly specifying an encoding
(W1514)
[warning] 208-208: Catching too general exception Exception
(W0718)
[warning] 203-203: Using open without explicitly specifying an encoding
(W1514)
[warning] 206-206: Using open without explicitly specifying an encoding
(W1514)
tests/model_registry/utils.py
[error] 16-16: Unable to import 'ocp_resources.secret'
(E0401)
[convention] 16-16: third party import "ocp_resources.secret.Secret" should be placed before first party imports "tests.model_registry.constants.MR_DB_IMAGE_DIGEST", "utilities.exceptions.ProtocolNotSupportedError", "utilities.constants.Protocols"
(C0411)
[convention] 16-16: Imports from package ocp_resources are not grouped
(C0412)
🔇 Additional comments (3)
tests/model_registry/utils.py (1)
321-349: Well-implemented MySQL configuration utility.The
make_mysql_configfunction is well-structured and correctly constructs the MySQL configuration dictionary with SSL support. The function properly:
- Builds the service host using Kubernetes DNS conventions
- Extracts database credentials from the secret
- Includes SSL CA configuration for secure connections
- Uses sensible defaults for port and resource name
tests/model_registry/rest_api/conftest.py (2)
77-103: Good fixture design for CA certificate testing.The
patch_invalid_cafixture is well-implemented for testing invalid CA certificate scenarios. It properly uses theResourceEditorcontext manager to patch the ConfigMap and yields the CA file path for use in tests.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 89-89: Line too long (106/100)
(C0301)
136-181: Well-structured secure MySQL deployment fixture.The
deploy_secure_mysql_and_mrfixture effectively configures MySQL with SSL/TLS by:
- Adding SSL CA arguments to the MySQL container
- Mounting the CA ConfigMap as a volume
- Creating a secure Model Registry instance
- Using proper nested context managers for resource lifecycle
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 146-146: Line too long (104/100)
(C0301)
[convention] 160-160: Line too long (115/100)
(C0301)
lugi0
left a comment
lugi0
left a comment
There was a problem hiding this comment.
A potential improvement that we can also handle in a separate PR, other than that LGTM
rhods-ci-bot
added
commented-by-fege
and removed
commented-by-lugi0
commented-by-dbasunag
commented-by-fege
labels
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
utilities/certificates_utils.py (1)
127-138: Doc-string refers to wrong resource typeThe parameter description still says “client: The admin client to create the ConfigMap” while this helper creates a Secret.
Please fix to avoid confusion for future readers.tests/model_registry/rest_api/test_model_registry_secure_db.py (1)
79-88: Assertion on string value is brittle; assert the exception class instead
requests.exceptions.SSLErroris already captured bypytest.raises.
Checking that the string contains “SSLError” is redundant and may break
if the exception message changes.- assert "SSLError" in str(exc_info.value), ( - f"Expected SSL certificate verification failure, got: {exc_info.value}" - ) + assert exc_info.type is requests.exceptions.SSLErrortests/model_registry/rest_api/utils.py (1)
36-40: Logging leaks full response bodies
LOGGER.info(..., rep: {resp.text})logs the entire server response which may contain
credentials or PII (e.g., connection strings).
Consider downgrading to DEBUG or redacting sensitive fields before logging.tests/model_registry/rest_api/conftest.py (1)
140-147: Hard-coded arg/volume indices assume single-container template
mysql_template["spec"]["containers"][0]mutates the first container
unconditionally. If the template ever adds an init-container before MySQL,
this will patch the wrong one.
Search for the container by name (as you already do later in
patch_mysql_deployment_with_ssl_ca) for consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/model_registry/conftest.py(1 hunks)tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(1 hunks)tests/model_registry/rest_api/utils.py(6 hunks)utilities/certificates_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/conftest.py
🧰 Additional context used
🧠 Learnings (2)
tests/model_registry/rest_api/utils.py (1)
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/utils.py:40-44
Timestamp: 2025-06-18T06:42:37.696Z
Learning: In tests/model_registry/rest_api/utils.py, the user prefers to keep the default value of verify=False in execute_model_registry_post_command to maintain backward compatibility, rather than changing it to True for security reasons.
tests/model_registry/rest_api/conftest.py (2)
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:257-264
Timestamp: 2025-06-18T08:27:21.097Z
Learning: In tests/model_registry/rest_api/conftest.py, the user fege prefers using next() without a default value to raise StopIteration when the mysql container is not found, rather than gracefully handling it with None checks. This is a deliberate "fail fast" design choice.
🪛 Pylint (3.3.7)
tests/model_registry/rest_api/conftest.py
[refactor] 151-151: Too many arguments (6/5)
(R0913)
[refactor] 151-151: Too many positional arguments (6/5)
(R0917)
[refactor] 239-239: Too many arguments (6/5)
(R0913)
[refactor] 239-239: Too many positional arguments (6/5)
(R0917)
[refactor] 239-239: Too many local variables (16/15)
(R0914)
[refactor] 261-261: Do not raise StopIteration in generator, use return statement instead
(R1708)
tests/model_registry/rest_api/test_model_registry_secure_db.py
[refactor] 58-58: Too many arguments (6/5)
(R0913)
[refactor] 58-58: Too many positional arguments (6/5)
(R0917)
[refactor] 105-105: Too many arguments (6/5)
(R0913)
[refactor] 105-105: Too many positional arguments (6/5)
(R0917)
🔇 Additional comments (1)
tests/model_registry/rest_api/test_model_registry_secure_db.py (1)
2-3:typing.Selfrequires Python ≥ 3.11 – confirm runtime versionThe project previously targeted Python 3.10.
If the test suite still runs against 3.10 images, importingSelfwill raiseImportError.
Please verify the CI image or gate the import with a fallback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/model_registry/rest_api/conftest.py (1)
228-229: Add explicit encoding to file operations.File operations should specify encoding explicitly to avoid platform-dependent behavior.
This issue was already identified in past reviews and appears to have been addressed in commit 3a056d2, but the change may not be visible in the current diff.
🧹 Nitpick comments (4)
tests/model_registry/utils.py (1)
266-288: Consider using deep copy for deployment spec manipulation.The function uses
list(spec["volumes"])which creates a shallow copy. If the original volumes contain nested dictionaries, mutations could affect the original spec unexpectedly.Consider using deep copy for safer manipulation:
+import copy def add_mysql_certs_volumes_to_deployment( spec: dict[str, Any], ca_configmap_name: str, ) -> list[dict[str, Any]]: """ Adds the MySQL certs volumes to the deployment. ... """ - volumes = list(spec["volumes"]) + volumes = copy.deepcopy(spec["volumes"]) volumes.extend([ {"name": ca_configmap_name, "configMap": {"name": ca_configmap_name}}, {"name": "mysql-server-cert", "secret": {"secretName": "mysql-server-cert"}}, {"name": "mysql-server-key", "secret": {"secretName": "mysql-server-key"}}, ]) return volumestests/model_registry/rest_api/test_model_registry_secure_db.py (2)
42-82: Test method complexity could be reduced.The test method has 7 parameters which makes it complex to understand and maintain. Consider extracting some setup logic into separate fixtures or helper methods.
The static analysis correctly identifies this as having too many arguments. While the test logic is sound, consider breaking down the fixture dependencies or using fixture composition to reduce parameter count.
84-124: Test method complexity - same issue as first test.Similar to the first test method, this one also has 7 parameters. The complexity warning applies here as well.
Consider the same approach as suggested for the first test method to reduce parameter complexity.
tests/model_registry/rest_api/conftest.py (1)
199-208: CA bundle file creation in current directory may cause conflicts.Creating the CA bundle file in the current working directory with a fixed name could cause conflicts if tests run in parallel or if the file already exists.
Consider using a unique temporary file path:
def local_ca_bundle(request: pytest.FixtureRequest, admin_client: DynamicClient) -> Generator[str, Any, Any]: """ Creates a local CA bundle file by fetching the CA bundle from a ConfigMap and appending the router CA from a Secret. ... """ + import tempfile + import os ca_bundle_filename = getattr(request, "param", {}).get("cert_name", "ca-bundle.crt") + temp_dir = tempfile.mkdtemp(prefix="mr_ca_") + ca_bundle_path = os.path.join(temp_dir, ca_bundle_filename) - ca_bundle_path = getattr(request, "param", {}).get("cert_name", "ca-bundle.crt") create_ca_bundle_with_router_cert( client=admin_client, namespace=py_config["model_registry_namespace"], ca_bundle_path=ca_bundle_path, cert_name=ca_bundle_path, ) yield ca_bundle_path - os.remove(ca_bundle_path) + import shutil + shutil.rmtree(temp_dir, ignore_errors=True)However, I notice from past comments you're keeping the current implementation for now, so this is just a suggestion for future consideration.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/rest_api/conftest.py(2 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(1 hunks)tests/model_registry/utils.py(1 hunks)utilities/certificates_utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utilities/certificates_utils.py
🧰 Additional context used
🧠 Learnings (1)
tests/model_registry/rest_api/conftest.py (2)
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:257-264
Timestamp: 2025-06-18T08:27:21.114Z
Learning: In tests/model_registry/rest_api/conftest.py, the user fege prefers using next() without a default value to raise StopIteration when the mysql container is not found, rather than gracefully handling it with None checks. This is a deliberate "fail fast" design choice.
🪛 Pylint (3.3.7)
tests/model_registry/rest_api/conftest.py
[refactor] 156-156: Too many arguments (6/5)
(R0913)
[refactor] 156-156: Too many positional arguments (6/5)
(R0917)
[refactor] 244-244: Too many arguments (6/5)
(R0913)
[refactor] 244-244: Too many positional arguments (6/5)
(R0917)
[refactor] 260-260: Do not raise StopIteration in generator, use return statement instead
(R1708)
tests/model_registry/rest_api/test_model_registry_secure_db.py
[refactor] 55-55: Too many arguments (7/5)
(R0913)
[refactor] 55-55: Too many positional arguments (7/5)
(R0917)
[refactor] 99-99: Too many arguments (7/5)
(R0913)
[refactor] 99-99: Too many positional arguments (7/5)
(R0917)
🔇 Additional comments (2)
tests/model_registry/rest_api/test_model_registry_secure_db.py (1)
75-75: Confirming URL annotation format forget_endpoint_from_mr_serviceI looked at
get_endpoint_from_mr_serviceintests/model_registry/utils.py. It simply readssvc.instance.metadata.annotations[f"{ADDRESS_ANNOTATION_PREFIX}{protocol}"]where
ADDRESS_ANNOTATION_PREFIX = "routing.opendatahub.io/external-address-". To see how that annotation is set, we need to inspect the fixtures intests/model_registry/conftest.pythat deploy the model registry and create the Route/Service.#!/bin/bash # Show where the external-address annotation is populated rg -n "external-address-REST" -C3 tests/model_registry/conftest.py sed -n '230,300p' tests/model_registry/conftest.pytests/model_registry/rest_api/conftest.py (1)
346-349: Good practice using deep copy for test data.The fixture correctly uses
copy.deepcopy(MODEL_REGISTER_DATA)to avoid mutating the original constant, which is a good practice for test data generation.
|
Status of building tag latest: success. |
5 participants
Description
How Has This Been Tested?
Merge criteria: