Skip to content

Use exitStack to support multiple resource creations #420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dbasunag wants to merge 2 commits into from
Closed

Use exitStack to support multiple resource creations #420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e2ba93c
Use exitStack to support multiple resource creations
dbasunag Jul 10, 2025
a10c4a6
Merge remote-tracking branch 'upstream/main' into multi
dbasunag Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
280 changes: 127 additions & 153 deletions tests/model_registry/conftest.py
View file
Open in desktop

Large diffs are not rendered by default.

15 changes: 5 additions & 10 deletions tests/model_registry/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from typing import Any
from ocp_resources.resource import Resource
from utilities.constants import ModelFormat, Annotations
from utilities.constants import ModelFormat


class ModelRegistryEndpoints:
Expand All @@ -25,7 +25,8 @@ class ModelRegistryEndpoints:
"str_key": "str_value",
},
}
MR_INSTANCE_NAME: str = "model-registry"
MR_INSTANCE_BASE_NAME: str = "model-registry"
MR_INSTANCE_NAME: str = f"{MR_INSTANCE_BASE_NAME}0"
SECURE_MR_NAME: str = "secure-db-mr"
ISTIO_CONFIG_DICT: dict[str, Any] = {
"gateway": {"grpc": {"tls": {}}, "rest": {"tls": {}}},
Expand All @@ -35,7 +36,8 @@ class ModelRegistryEndpoints:
"routePort": 443,
"serviceRoute": "enabled",
}
DB_RESOURCES_NAME: str = "db-model-registry"
DB_BASE_RESOURCES_NAME: str = "db-model-registry"
DB_RESOURCE_NAME: str = f"{DB_BASE_RESOURCES_NAME}0"
MR_DB_IMAGE_DIGEST: str = (
"public.ecr.aws/docker/library/mysql@sha256:9de9d54fecee6253130e65154b930978b1fcc336bcc86dfd06e89b72a2588ebe"
)
Expand All @@ -53,10 +55,3 @@ class ModelRegistryEndpoints:
CA_CONFIGMAP_NAME = "odh-trusted-ca-bundle"
CA_MOUNT_PATH = "/etc/pki/ca-trust/extracted/pem"
CA_FILE_PATH = f"{CA_MOUNT_PATH}/ca-bundle.crt"

MODEL_REGISTRY_STANDARD_LABELS = {
Annotations.KubernetesIo.NAME: MR_INSTANCE_NAME,
Annotations.KubernetesIo.INSTANCE: MR_INSTANCE_NAME,
Annotations.KubernetesIo.PART_OF: MR_OPERATOR_NAME,
Annotations.KubernetesIo.CREATED_BY: MR_OPERATOR_NAME,
}
5 changes: 2 additions & 3 deletions tests/model_registry/image_validation/test_verify_rhoai_images.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import pytest
from typing import Self, Set
from typing import Self, Set, Any
from simple_logger.logger import get_logger
from kubernetes.dynamic import DynamicClient
from pytest_testconfig import config as py_config
Expand All @@ -8,7 +8,6 @@
from utilities.general import (
validate_container_images,
)
from ocp_resources.model_registry_modelregistry_opendatahub_io import ModelRegistry
from ocp_resources.pod import Pod

LOGGER = get_logger(name=__name__)
Expand Down Expand Up @@ -43,7 +42,7 @@ class TestModelRegistryImages:
def test_verify_model_registry_images(
self: Self,
admin_client: DynamicClient,
model_registry_instance: ModelRegistry,
model_registry_instance: list[Any],
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Jul 11, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify if model_registry_instance parameter is used.

The type annotation change from ModelRegistry to list[Any] aligns with the fixture changes, but the test doesn't appear to use the model_registry_instance parameter in its logic.

🏁 Script executed:

#!/bin/bash
# Check if model_registry_instance parameter is used in this test
rg -A 10 -B 5 "model_registry_instance" tests/model_registry/image_validation/test_verify_rhoai_images.py

Length of output: 819

Clarify model_registry_instance fixture usage

The model_registry_instance parameter isn’t referenced in the test body but is needed to spin up the ModelRegistry instance before its pods are validated. To prevent confusion (and linter “unused parameter” errors), please annotate it with a comment or a noqa directive.

• File: tests/model_registry/image_validation/test_verify_rhoai_images.py
Line 45
Current:
model_registry_instance: list[Any],
Suggested:
model_registry_instance: list[Any], # noqa: F841 – ensures registry instance is created for pod validation

🤖 Prompt for AI Agents 
In tests/model_registry/image_validation/test_verify_rhoai_images.py at line 45,
the parameter model_registry_instance is not used in the test body but is
required to initialize the ModelRegistry instance. To avoid confusion and linter
warnings about unused variables, add a noqa directive comment to the parameter
line like this: model_registry_instance: list[Any],  # noqa: F841 – ensures
registry instance is created for pod validation.

model_registry_operator_pod: Pod,
model_registry_instance_pod: Pod,
related_images_refs: Set[str],
Expand Down
5 changes: 2 additions & 3 deletions tests/model_registry/negative_tests/test_db_migration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import pytest
from typing import Self
from typing import Self, Any
from simple_logger.logger import get_logger
from ocp_resources.model_registry_modelregistry_opendatahub_io import ModelRegistry
from pytest_testconfig import config as py_config
from ocp_resources.pod import Pod
from utilities.constants import DscComponents
Expand Down Expand Up @@ -32,7 +31,7 @@ class TestDBMigration:
def test_db_migration_negative(
self: Self,
admin_client: DynamicClient,
model_registry_instance: ModelRegistry,
model_registry_instance: list[Any],
model_registry_db_instance_pod: Pod,
set_mr_db_dirty: int,
delete_mr_deployment: None,
Expand Down
12 changes: 6 additions & 6 deletions tests/model_registry/negative_tests/test_model_registry_creation_negative.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
from utilities.constants import DscComponents, Annotations
from tests.model_registry.constants import (
MR_OPERATOR_NAME,
MR_INSTANCE_NAME,
MR_INSTANCE_BASE_NAME,
ISTIO_CONFIG_DICT,
DB_RESOURCES_NAME,
DB_BASE_RESOURCES_NAME,
)
from kubernetes.dynamic.exceptions import ForbiddenError

Expand Down Expand Up @@ -65,7 +65,7 @@ def test_registering_model_negative(
"host": f"{model_registry_db_deployment_negative_test.name}."
f"{model_registry_db_deployment_negative_test.namespace}.svc.cluster.local",
"database": model_registry_db_secret_negative_test.string_data["database-name"],
"passwordSecret": {"key": "database-password", "name": DB_RESOURCES_NAME},
"passwordSecret": {"key": "database-password", "name": DB_BASE_RESOURCES_NAME},
"port": 3306,
"skipDBCreation": False,
"username": model_registry_db_secret_negative_test.string_data["database-user"],
Expand All @@ -75,11 +75,11 @@ def test_registering_model_negative(
match=f"namespace must be {expected_namespace}",
):
with ModelRegistry(
name=MR_INSTANCE_NAME,
name=MR_INSTANCE_BASE_NAME,
namespace=model_registry_namespace_for_negative_tests.name,
label={
Annotations.KubernetesIo.NAME: MR_INSTANCE_NAME,
Annotations.KubernetesIo.INSTANCE: MR_INSTANCE_NAME,
Annotations.KubernetesIo.NAME: MR_INSTANCE_BASE_NAME,
Annotations.KubernetesIo.INSTANCE: MR_INSTANCE_BASE_NAME,
Annotations.KubernetesIo.PART_OF: MR_OPERATOR_NAME,
Annotations.KubernetesIo.CREATED_BY: MR_OPERATOR_NAME,
},
Expand Down
8 changes: 4 additions & 4 deletions tests/model_registry/python_client/test_model_registry_creation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,10 @@ class TestModelRegistryCreation:
@pytest.mark.smoke
def test_registering_model(
self: Self,
model_registry_client: ModelRegistryClient,
model_registry_client: list[ModelRegistryClient],
registered_model: RegisteredModel,
):
model = model_registry_client.get_registered_model(name=MODEL_NAME)
model = model_registry_client[0].get_registered_model(name=MODEL_NAME)
expected_attrs = {
"id": registered_model.id,
"name": registered_model.name,
Expand Down Expand Up @@ -127,7 +127,7 @@ def test_model_registry_pod_log_mlmd_removal(
],
)
def test_model_registry_endpoint_response(
self, model_registry_rest_url: str, model_registry_rest_headers: dict[str, str], endpoint: str
self, model_registry_rest_url: list[str], model_registry_rest_headers: dict[str, str], endpoint: str
):
"""
RHOAIENG-26239: Test to ensure model registry endpoints are responsive
Expand All @@ -137,6 +137,6 @@ def test_model_registry_endpoint_response(
Ensure endpoint is responsive via get call
"""
output = execute_model_registry_get_command(
url=f"{model_registry_rest_url}/{endpoint}", headers=model_registry_rest_headers, json_output=False
url=f"{model_registry_rest_url[0]}/{endpoint}", headers=model_registry_rest_headers, json_output=False
)
assert output["raw_output"].lower() == "OK".lower()
1 change: 0 additions & 1 deletion tests/model_registry/rbac/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from tests.model_registry.rbac.group_utils import create_group
from tests.model_registry.constants import MR_INSTANCE_NAME


LOGGER = get_logger(name=__name__)
DEFAULT_TOKEN_DURATION = "10m"

Expand Down
18 changes: 9 additions & 9 deletions tests/model_registry/rbac/test_mr_rbac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,20 +42,20 @@
],
indirect=True,
)
@pytest.mark.usefixtures("updated_dsc_component_state_scope_class")
@pytest.mark.usefixtures("updated_dsc_component_state_scope_class", "model_registry_instance")
class TestUserPermission:
@pytest.mark.sanity
def test_user_permission_non_admin_user(
self: Self,
test_idp_user,
model_registry_instance_rest_endpoint: str,
model_registry_instance_rest_endpoint: list[str],
login_as_test_user: None,
):
"""
This test verifies that non-admin users cannot access the Model Registry (403 Forbidden)
"""
client_args = build_mr_client_args(
rest_endpoint=model_registry_instance_rest_endpoint, token=get_openshift_token()
rest_endpoint=model_registry_instance_rest_endpoint[0], token=get_openshift_token()
)
with pytest.raises(ForbiddenException) as exc_info:
ModelRegistryClient(**client_args)
Expand All @@ -65,7 +65,7 @@ def test_user_permission_non_admin_user(
@pytest.mark.sanity
def test_user_added_to_group(
self: Self,
model_registry_instance_rest_endpoint: str,
model_registry_instance_rest_endpoint: list[str],
test_idp_user: UserTestSession,
model_registry_group_with_user: Group,
login_as_test_user: Generator[UserTestSession, None, None],
Expand All @@ -79,7 +79,7 @@ def test_user_added_to_group(
wait_timeout=240,
sleep=5,
func=assert_positive_mr_registry,
model_registry_instance_rest_endpoint=model_registry_instance_rest_endpoint,
model_registry_instance_rest_endpoint=model_registry_instance_rest_endpoint[0],
token=get_openshift_token(),
)
for _ in sampler:
Expand All @@ -90,7 +90,7 @@ def test_user_added_to_group(
def test_create_group(
self: Self,
test_idp_user: UserTestSession,
model_registry_instance_rest_endpoint: str,
model_registry_instance_rest_endpoint: list[str],
created_role_binding_group: RoleBinding,
login_as_test_user: None,
):
Expand All @@ -103,14 +103,14 @@ def test_create_group(
3. Users in the group can access the Model Registry
"""
assert_positive_mr_registry(
model_registry_instance_rest_endpoint=model_registry_instance_rest_endpoint,
model_registry_instance_rest_endpoint=model_registry_instance_rest_endpoint[0],
)

@pytest.mark.sanity
def test_add_single_user_role_binding(
self: Self,
test_idp_user: UserTestSession,
model_registry_instance_rest_endpoint: str,
model_registry_instance_rest_endpoint: list[str],
created_role_binding_user: RoleBinding,
login_as_test_user: None,
):
Expand All @@ -122,5 +122,5 @@ def test_add_single_user_role_binding(
2. The user can access the Model Registry after being granted access
"""
assert_positive_mr_registry(
model_registry_instance_rest_endpoint=model_registry_instance_rest_endpoint,
model_registry_instance_rest_endpoint=model_registry_instance_rest_endpoint[0],
)
10 changes: 5 additions & 5 deletions tests/model_registry/rbac/test_mr_rbac_sa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,18 @@ class TestModelRegistryRBAC:
def test_service_account_access_denied(
self: Self,
sa_token: str,
model_registry_instance_rest_endpoint: str,
model_registry_instance_rest_endpoint: list[str],
):
"""
Verifies SA access is DENIED (403 Forbidden) by default via REST.
Does NOT use mr_access_role or mr_access_role_binding fixtures.
"""
LOGGER.info("--- Starting RBAC Test: Access Denied ---")
LOGGER.info(f"Targeting Model Registry REST endpoint: {model_registry_instance_rest_endpoint}")
LOGGER.info(f"Targeting Model Registry REST endpoint: {model_registry_instance_rest_endpoint[0]}")
LOGGER.info("Expecting initial access DENIAL (403 Forbidden)")

client_args = build_mr_client_args(
rest_endpoint=model_registry_instance_rest_endpoint, token=sa_token, author="rbac-test-denied"
rest_endpoint=model_registry_instance_rest_endpoint[0], token=sa_token, author="rbac-test-denied"
)
LOGGER.debug(f"Attempting client connection with args: {client_args}")

Expand All @@ -69,7 +69,7 @@ def test_service_account_access_denied(
def test_service_account_access_granted(
self: Self,
sa_token: str,
model_registry_instance_rest_endpoint: str,
model_registry_instance_rest_endpoint: list[str],
):
"""
Verifies SA access is GRANTED via REST after applying Role and RoleBinding fixtures.
Expand All @@ -80,7 +80,7 @@ def test_service_account_access_granted(

try:
client_args = build_mr_client_args(
rest_endpoint=model_registry_instance_rest_endpoint, token=sa_token, author="rbac-test-granted"
rest_endpoint=model_registry_instance_rest_endpoint[0], token=sa_token, author="rbac-test-granted"
)
LOGGER.debug(f"Attempting client connection with args: {client_args}")
mr_client_success = ModelRegistryClient(**client_args)
Expand Down
Loading