Use exitStack to support multiple resource creations#429
Merged
dbasunag merged 22 commits intoopendatahub-io:mainfrom Aug 5, 2025
Merged
Use exitStack to support multiple resource creations#429dbasunag merged 22 commits intoopendatahub-io:mainfrom
dbasunag merged 22 commits intoopendatahub-io:mainfrom
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change refactors Model Registry test fixtures and utilities to support managing multiple resource instances in parallel. It updates fixture signatures, return types, and usages throughout the test suite to work with lists of resources and clients, introduces new utility functions for resource creation, and adjusts constant definitions and label handling for consistency. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (21)📓 Common learnings📚 Learning: in tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fix...Applied to files:
📚 Learning: in tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated m...Applied to files:
📚 Learning: in tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fi...Applied to files:
📚 Learning: in tests/model_registry/conftest.py, the model_registry_instance_rest_endpoint fixture contains a bu...Applied to files:
📚 Learning: in tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the a...Applied to files:
📚 Learning: in tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_en...Applied to files:
📚 Learning: in tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should...Applied to files:
📚 Learning: in tests/model_serving/model_runtime/mlserver/basic_model_deployment/test_mlserver_basic_model_deplo...Applied to files:
📚 Learning: in tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the adm...Applied to files:
📚 Learning: in tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resource...Applied to files:
📚 Learning: for trustyai image validation tests: operator image tests require admin_client, related_images_refs,...Applied to files:
📚 Learning: in the opendatahub-tests repository, prefer keeping test parameterization configurations inline rath...Applied to files:
📚 Learning: in the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures r...Applied to files:
📚 Learning: in the ocp-resources library, when creating kubernetes resources like role and rolebinding, it's pre...Applied to files:
📚 Learning: in ocp_resources library, when reading from existing oauth resources (like `oauth(name="cluster")`),...Applied to files:
📚 Learning: in model registry rbac tests, client instantiation tests are designed to verify the ability to creat...Applied to files:
📚 Learning: in tests/model_registry/rest_api/utils.py, the user prefers to keep the default value of verify=fals...Applied to files:
📚 Learning: in tests/model_registry/conftest.py, service resources can be created without explicitly passing the...Applied to files:
📚 Learning: in negative tests for model registry database migration failures (like test_db_migration_negative), ...Applied to files:
📚 Learning: the `build_mr_client_args` function in the model registry rbac tests uses simple string splitting (`...Applied to files:
🧬 Code Graph Analysis (1)tests/model_registry/conftest.py (4)
🔇 Additional comments (13)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/verified', '/cherry-pick', '/lgtm', '/wip', '/build-push-pr-image'} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/model_registry/utils.py (1)
453-485: Consider improving variable naming and add error handling.The function is well-structured but could benefit from minor improvements:
- Variable names like
num_deploymentcould be more descriptive (e.g.,deployment_index)- Consider adding validation that the secret exists before proceeding
- for num_deployment in range(0, num): - name = f"{base_name}{num_deployment}" - secret_name = f"{DB_BASE_RESOURCES_NAME}{num_deployment}" + for deployment_index in range(0, num): + name = f"{base_name}{deployment_index}" + secret_name = f"{DB_BASE_RESOURCES_NAME}{deployment_index}"tests/model_registry/rbac/test_mr_rbac_sa.py (1)
83-83: Consider updating log message for clarity.The log message now prints the entire list of endpoints instead of just the first one being used in the test. Consider updating for clarity:
- LOGGER.info(f"Targeting Model Registry REST endpoint: {model_registry_instance_rest_endpoint[0]}") + LOGGER.info(f"Targeting Model Registry REST endpoint: {model_registry_instance_rest_endpoint[0]} (first of {len(model_registry_instance_rest_endpoint)} available)")This provides more context about the multi-instance setup while clearly indicating which endpoint is being tested.
tests/model_registry/conftest.py (1)
379-390: Consider adding a clarifying comment.The fixture accepts a list of clients but intentionally uses only the first one. This is reasonable since registering the same model multiple times would be redundant, but it would be helpful to document this design choice.
def registered_model(request: FixtureRequest, model_registry_client: list[ModelRegistryClient]) -> RegisteredModel: + # Use only the first client as registering the same model multiple times is redundant return model_registry_client[0].register_model(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
tests/model_registry/conftest.py(8 hunks)tests/model_registry/constants.py(3 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(0 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(3 hunks)tests/model_registry/rbac/test_mr_rbac.py(6 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(3 hunks)tests/model_registry/rest_api/conftest.py(11 hunks)tests/model_registry/rest_api/mariadb/conftest.py(2 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(1 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(2 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(2 hunks)tests/model_registry/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/model_registry/image_validation/test_verify_rhoai_images.py
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
tests/model_registry/rest_api/mariadb/conftest.py (5)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
tests/model_registry/rest_api/test_model_registry_rest_api.py (8)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
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.705Z
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (7)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
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.705Z
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.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
tests/model_registry/python_client/test_model_registry_creation.py (5)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#283
File: tests/model_registry/rbac/utils.py:31-32
Timestamp: 2025-05-22T14:14:45.746Z
Learning: In the OpenDataHub tests codebase, the ModelRegistryClient accepts the port parameter as a string, and explicit conversion to integer is not required.
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.705Z
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/constants.py (4)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
tests/model_registry/rbac/test_mr_rbac.py (8)
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
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.705Z
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
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.
tests/model_registry/upgrade/test_model_registry_upgrade.py (7)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
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.705Z
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
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.
tests/model_registry/utils.py (5)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
tests/model_registry/rbac/test_mr_rbac_sa.py (6)
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
tests/model_registry/rest_api/test_model_registry_secure_db.py (6)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
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.705Z
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
tests/model_registry/conftest.py (7)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
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.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
tests/model_registry/rest_api/conftest.py (10)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
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.
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.705Z
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.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
🧬 Code Graph Analysis (9)
tests/model_registry/rest_api/mariadb/conftest.py (1)
tests/model_registry/utils.py (2)
wait_for_pods_running(213-246)get_mr_standard_labels(488-494)
tests/model_registry/rest_api/test_model_registry_rest_api.py (2)
tests/model_registry/conftest.py (1)
model_registry_instance_mysql(180-226)tests/model_registry/rest_api/utils.py (1)
ModelRegistryV1Alpha1(253-254)
tests/model_registry/python_client/test_model_registry_creation.py (1)
tests/model_registry/conftest.py (4)
model_registry_client(351-375)registered_model(379-390)model_registry_rest_url(476-478)model_registry_rest_headers(482-487)
tests/model_registry/constants.py (1)
utilities/constants.py (1)
ModelFormat(12-19)
tests/model_registry/rbac/test_mr_rbac.py (4)
tests/model_registry/conftest.py (1)
model_registry_instance_rest_endpoint(230-242)tests/model_registry/rbac/conftest.py (2)
login_as_test_user(358-372)test_idp_user(238-275)tests/model_registry/rbac/utils.py (1)
build_mr_client_args(17-38)utilities/infra.py (1)
get_openshift_token(596-604)
tests/model_registry/upgrade/test_model_registry_upgrade.py (3)
tests/model_registry/conftest.py (3)
model_registry_client(351-375)registered_model(379-390)model_registry_instance_mysql(180-226)tests/model_registry/utils.py (1)
get_and_validate_registered_model(322-348)tests/model_registry/rest_api/utils.py (1)
ModelRegistryV1Alpha1(253-254)
tests/model_registry/rbac/test_mr_rbac_sa.py (3)
tests/model_registry/conftest.py (1)
model_registry_instance_rest_endpoint(230-242)tests/model_registry/rbac/conftest.py (1)
sa_token(60-101)tests/model_registry/rbac/utils.py (1)
build_mr_client_args(17-38)
tests/model_registry/conftest.py (4)
tests/model_registry/rest_api/utils.py (1)
ModelRegistryV1Alpha1(253-254)utilities/constants.py (2)
Labels(178-206)Protocols(89-96)tests/model_registry/utils.py (8)
get_endpoint_from_mr_service(63-67)get_mr_service_by_label(36-60)wait_for_pods_running(213-246)get_mr_service_objects(378-410)get_mr_pvc_objects(413-430)get_mr_secret_objects(433-450)get_mr_deployment_objects(453-485)get_model_registry_objects(497-529)tests/conftest.py (4)
admin_client(65-66)teardown_resources(86-93)dsc_resource(347-348)current_client_token(81-82)
tests/model_registry/rest_api/conftest.py (4)
tests/model_registry/utils.py (2)
get_mr_standard_labels(488-494)get_mysql_config(532-540)tests/model_registry/conftest.py (6)
model_registry_rest_url(476-478)model_registry_rest_headers(482-487)model_registry_db_secret(114-137)model_registry_namespace(55-56)model_registry_mysql_metadata_db(170-176)model_registry_db_deployment(141-166)tests/model_registry/rest_api/utils.py (1)
register_model_rest_api(54-87)tests/conftest.py (1)
admin_client(65-66)
🔇 Additional comments (37)
tests/model_registry/utils.py (6)
378-410: LGTM! Well-structured utility function for creating Service objects.The function follows a consistent pattern and properly handles multiple service creation. The annotation template is correctly formatted for MySQL service exposure.
413-430: LGTM! Consistent PVC creation utility.The function properly creates PersistentVolumeClaim objects with appropriate access modes and size configuration.
433-450: LGTM! Secret creation utility handles sensitive data appropriately.The function correctly uses string_data and applies proper annotations for database secrets.
488-494: LGTM! Good replacement for the removed constant.The function provides a dynamic way to generate standard labels, which is more flexible than the previous static constant approach.
497-529: Complex function handling multiple configurations correctly.The function properly handles different Model Registry configurations including Istio, OAuth proxy, and MySQL settings. The conditional SSL certificate handling is appropriate.
532-540: LGTM! Clean MySQL configuration helper.The function provides a centralized way to generate MySQL configuration with proper service DNS naming.
tests/model_registry/negative_tests/test_model_registry_creation_negative.py (2)
15-15: LGTM! Constant name update aligns with refactoring.The change from
DB_RESOURCES_NAMEtoDB_RESOURCE_NAMEis consistent with the broader constant refactoring in this PR.
74-74: LGTM! Updated usage of renamed constant.The passwordSecret name correctly uses the updated constant name.
tests/model_registry/rest_api/mariadb/conftest.py (2)
8-8: LGTM! Updated import for dynamic label generation.The import correctly includes the new
get_mr_standard_labelsfunction that replaces the removed constant.
70-70: LGTM! Proper usage of dynamic label function.The function call correctly passes
MR_INSTANCE_NAMEas the resource name parameter, maintaining the same label generation behavior as the previous constant.tests/model_registry/rest_api/test_model_registry_rest_api.py (3)
105-106: LGTM! Correct access pattern for updated fixture.The change to access
model_registry_instance_mysql[0]is consistent with the fixture refactoring that now returns a list of instances.
114-114: LGTM! Consistent fixture usage update.The change properly accesses the first instance from the updated list-based fixture.
121-122: LGTM! Maintains test functionality with updated fixture.The change correctly accesses the instance from the list while maintaining the same test validation logic.
tests/model_registry/rest_api/test_model_registry_secure_db.py (2)
56-60: LGTM! Improved readability with multi-line fixture format.The multi-line format for the
@pytest.mark.usefixturesdecorator improves readability by clearly showing each fixture dependency.
91-91: LGTM! Updated parameter list for fixture refactoring.The addition of
deploy_secure_mysql_and_mrto the parameter list is consistent with the fixture refactoring in this PR.tests/model_registry/rbac/test_mr_rbac.py (2)
73-73: Consistent pattern implementation looks good.The systematic update to use
list[str]parameters and access the first element with[0]is implemented consistently across all test methods. This aligns well with the broader refactoring to support multiple Model Registry instances.However, consider applying the same bounds checking suggestion from the previous comment to these usages as well for robustness.
Also applies to: 87-87, 98-98, 111-111, 118-118, 130-130
56-56: Verify list bounds before accessing the first element.While the type change from
strtolist[str]aligns with the multi-instance support refactoring, accessing[0]without bounds checking could cause anIndexErrorif the fixture returns an empty list.Consider adding defensive checks or using the fixture design to guarantee at least one element. Based on the relevant code in
tests/model_registry/conftest.py, the fixture should return at least one endpoint, but defensive programming would be beneficial:- client_args = build_mr_client_args( - rest_endpoint=model_registry_instance_rest_endpoint[0], token=get_openshift_token() - ) + assert model_registry_instance_rest_endpoint, "No model registry endpoints available" + client_args = build_mr_client_args( + rest_endpoint=model_registry_instance_rest_endpoint[0], token=get_openshift_token() + )Also applies to: 63-63
⛔ Skipped due to learnings
Learnt from: lugi0 PR: opendatahub-io/opendatahub-tests#296 File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0 Timestamp: 2025-05-07T10:40:15.681Z Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.Learnt from: dbasunag PR: opendatahub-io/opendatahub-tests#354 File: tests/model_registry/rbac/test_mr_rbac.py:64-77 Timestamp: 2025-06-16T11:26:53.789Z Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.Learnt from: dbasunag PR: opendatahub-io/opendatahub-tests#401 File: tests/model_registry/rest_api/mariadb/conftest.py:89-110 Timestamp: 2025-07-04T00:17:47.799Z Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.Learnt from: dbasunag PR: opendatahub-io/opendatahub-tests#354 File: tests/model_registry/rbac/conftest.py:166-175 Timestamp: 2025-06-16T11:25:39.599Z Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.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.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.705Z 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.Learnt from: dbasunag PR: opendatahub-io/opendatahub-tests#354 File: tests/model_registry/rbac/conftest.py:355-371 Timestamp: 2025-06-16T11:25:51.461Z Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.Learnt from: dbasunag PR: opendatahub-io/opendatahub-tests#354 File: tests/model_registry/rbac/conftest.py:212-224 Timestamp: 2025-06-16T13:00:51.478Z Learning: In the opendatahub-tests repository, the OAuth cluster configuration's identityProviders field should not be empty/None and doesn't require defensive programming checks when concatenating with lists.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: adolfo-ab PR: opendatahub-io/opendatahub-tests#334 File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65 Timestamp: 2025-06-05T10:05:17.642Z Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.tests/model_registry/constants.py (2)
3-3: Import change looks appropriate.The removal of
Annotationsfrom the import statement suggests it's no longer used in this file, which is consistent with the AI summary mentioning the removal ofMODEL_REGISTRY_STANDARD_LABELSthat referencedAnnotations.
28-29: Well-designed constants for multi-instance support.The introduction of base name constants (
MR_INSTANCE_BASE_NAME,DB_BASE_RESOURCES_NAME) paired with numbered instance names provides a clean foundation for generating multiple Model Registry instances. The "0" suffix pattern suggests this is the first instance in a series.This design will facilitate the creation of multiple instances with predictable, sequential naming when combined with the utility functions mentioned in the AI summary.
Also applies to: 39-40
tests/model_registry/python_client/test_model_registry_creation.py (2)
65-65: Consistent multi-instance support implementation.The change from single
ModelRegistryClienttolist[ModelRegistryClient]with[0]access follows the same pattern as other files in this refactoring.Consider adding bounds checking as suggested in previous files:
+ assert model_registry_client, "No model registry clients available" model = model_registry_client[0].get_registered_model(name=MODEL_NAME)Also applies to: 68-68
133-133: URL list handling follows established pattern.The
model_registry_rest_urlparameter change tolist[str]and[0]access is consistent with the endpoint handling in other files.Same bounds checking suggestion applies here as well for defensive programming.
Also applies to: 143-143
tests/model_registry/upgrade/test_model_registry_upgrade.py (2)
29-29: Good addition of missing fixtures.Adding
model_registry_mysql_metadata_dbandmodel_registry_instance_mysqlfixtures to theTestPreUpgradeModelRegistryclass improves consistency with the test setup and aligns with the multi-instance support refactoring.
34-34: Comprehensive multi-instance support implementation.The systematic conversion of both
model_registry_clientandmodel_registry_instance_mysqlparameters to lists with[0]access is implemented consistently across all test methods. This provides a solid foundation for future multi-instance testing scenarios.However, consider adding defensive bounds checking for all list accesses to prevent potential
IndexErrorexceptions:+ assert model_registry_client, "No model registry clients available" errors = get_and_validate_registered_model( model_registry_client=model_registry_client[0], model_name=MODEL_NAME, registered_model=registered_model )Apply similar checks to all other list access points in this file.
Also applies to: 38-38, 49-49, 53-53, 62-62, 65-65, 72-72, 74-74, 81-81, 85-86
tests/model_registry/conftest.py (8)
1-2: LGTM!The import of
ExitStackand the updated constants/utilities align well with the refactoring to support multiple resource instances.Also applies to: 27-34, 41-45
60-84: LGTM!The fixture correctly implements multiple resource management using
ExitStackwhile maintaining backward compatibility with defaultnum=1.
87-111: Consistent implementation with other fixtures.The PVC fixture follows the same pattern for multiple resource management.
170-177: LGTM!The fixture correctly updated to handle lists while maintaining its role as a convenience fixture.
180-227: LGTM!The fixture properly handles multiple Model Registry instances with appropriate wait conditions for each instance.
230-243: LGTM!The fixture correctly retrieves endpoints for all Model Registry instances in the namespace.
351-376: LGTM!The fixture correctly creates Model Registry clients for each endpoint. The simple string splitting is intentional per the design.
476-479: LGTM!The fixture correctly transforms all REST endpoints to HTTPS URLs.
tests/model_registry/rest_api/conftest.py (6)
19-20: LGTM!The import changes align with the refactoring to use centralized utility functions.
Also applies to: 24-24
49-56: LGTM!The fixture correctly handles the list parameter by using the first URL.
123-144: LGTM!The fixture correctly handles the list parameter and uses the updated constant.
148-180: LGTM!The fixture has been enhanced to support configurable SSL certificate mapping while properly using the new utility functions.
241-268: LGTM!The fixture correctly handles the list parameter and properly uses
getfixturevaluefor conditional fixture invocation.
208-208: Good naming improvement.The parameter rename from
mysql_ssl_secretstomysql_ssl_artifact_pathsbetter reflects that it contains file paths rather than secret objects.Also applies to: 216-216
lugi0
requested changes
Jul 14, 2025
Contributor
lugi0
left a comment
lugi0
left a comment
There was a problem hiding this comment.
I personally do not like this approach because it does not solve the issues we are trying to solve.
Other than forcing us to deal with lists through the whole call stack between fixtures and tests, it only parametrizes the number of resources without enabling us to control any of the other parameters of each resource.
Furthermore, it does not deal with the issue of dependency chains between fixtures in any way.
IMHO, any of the proposed implementations in #403 would solve the same issue of multiple resources as well as tackling the other issues we have, hence they are all preferable to this one.
fege
reviewed
Jul 14, 2025
fege
reviewed
Jul 14, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
tests/model_registry/conftest.py (1)
388-402: Add bounds checking for safe list access.The fixture accesses
model_registry_client[0]without verifying the list is not empty. Consider adding bounds checking to prevent potential IndexError.def registered_model( request: FixtureRequest, model_registry_client: list[ModelRegistryClient] ) -> Generator[RegisteredModel, None, None]: + if not model_registry_client: + raise ValueError("No model registry clients available") yield model_registry_client[0].register_model(tests/model_registry/rest_api/conftest.py (1)
247-267: Good adaptation to new deployment retrieval pattern.The fixture correctly uses the new
get_mr_deploymentfunction and proper parameter naming. However, consider adding bounds checking when accessingmodel_registry_db_deployments[0].model_registry_db_deployments = get_mr_deployment(admin_client=admin_client, mr_namespace=model_registry_namespace) + if not model_registry_db_deployments: + raise ValueError("No model registry database deployments found")
♻️ Duplicate comments (1)
tests/model_registry/conftest.py (1)
182-187: This fixture still depends on 4 other fixtures.The fixture dependency issue raised in previous reviews remains unresolved. Consider refactoring to reduce the number of fixture dependencies.
🧹 Nitpick comments (1)
tests/model_registry/conftest.py (1)
60-68: Remove commented-out code.The commented-out alternative implementation of the
model_registry_namespacefixture should be removed to keep the codebase clean, unless there's a specific reason to preserve it.-# @pytest.fixture(scope="class") -# def model_registry_namespace(request: pytest.FixtureRequest, admin_client: DynamicClient) -> str: -# param_namespace = getattr(request, "param") or py_config["model_registry_namespace"] -# dsc = get_data_science_cluster(client=admin_client) -# namespace = dsc.instance.spec.components.modelregistry.registriesNamespace -# LOGGER.info(f"Model Registry namespace from DSC: {namespace}, from test: {param_namespace}") -# assert param_namespace == namespace -# return namespace -#
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
tests/model_registry/conftest.py(6 hunks)tests/model_registry/constants.py(4 hunks)tests/model_registry/image_validation/test_verify_rhoai_images.py(0 hunks)tests/model_registry/negative_tests/test_model_registry_creation_negative.py(2 hunks)tests/model_registry/python_client/test_model_registry_creation.py(4 hunks)tests/model_registry/rbac/test_mr_rbac.py(6 hunks)tests/model_registry/rbac/test_mr_rbac_sa.py(3 hunks)tests/model_registry/rest_api/conftest.py(11 hunks)tests/model_registry/rest_api/mariadb/conftest.py(2 hunks)tests/model_registry/rest_api/test_model_registry_rest_api.py(1 hunks)tests/model_registry/rest_api/test_model_registry_secure_db.py(3 hunks)tests/model_registry/rest_api/test_multiple_mr.py(1 hunks)tests/model_registry/rest_api/utils.py(2 hunks)tests/model_registry/upgrade/test_model_registry_upgrade.py(2 hunks)tests/model_registry/utils.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/model_registry/image_validation/test_verify_rhoai_images.py
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/model_registry/rest_api/mariadb/conftest.py
- tests/model_registry/rest_api/test_model_registry_rest_api.py
- tests/model_registry/negative_tests/test_model_registry_creation_negative.py
- tests/model_registry/rbac/test_mr_rbac.py
- tests/model_registry/rest_api/test_multiple_mr.py
- tests/model_registry/rest_api/utils.py
- tests/model_registry/constants.py
- tests/model_registry/rest_api/test_model_registry_secure_db.py
- tests/model_registry/python_client/test_model_registry_creation.py
- tests/model_registry/rbac/test_mr_rbac_sa.py
- tests/model_registry/upgrade/test_model_registry_upgrade.py
- tests/model_registry/utils.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
tests/model_registry/rest_api/conftest.py (8)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
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.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
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.705Z
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.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:355-371
Timestamp: 2025-06-16T11:25:51.461Z
Learning: In the opendatahub-tests repository, the login_as_test_user fixture in tests/model_registry/rbac/conftest.py intentionally logs back in as the original admin user without providing a password using login_with_user_password. This behavior is acceptable in their testing environment.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
tests/model_registry/conftest.py (7)
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/conftest.py:166-175
Timestamp: 2025-06-16T11:25:39.599Z
Learning: In tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resources (like RoleBindings and groups) instead of random names. This design choice prioritizes exposing cleanup failures from previous test runs through name collisions rather than masking such issues with random names. The philosophy is that test failures should be observable and informative to help debug underlying infrastructure or cleanup issues.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
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.
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#409
File: tests/model_registry/negative_tests/test_db_migration.py:47-57
Timestamp: 2025-07-09T08:50:22.172Z
Learning: In negative tests for model registry database migration failures (like test_db_migration_negative), the model registry pod is expected to be in a failed state (not Running) when the database is dirty. The test fetches logs from the failed pod to verify the expected error message about dirty database version, so waiting for Running status would be inappropriate.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#296
File: tests/model_registry/rbac/test_mr_rbac_sa.py:0-0
Timestamp: 2025-05-07T10:40:15.681Z
Learning: The `build_mr_client_args` function in the Model Registry RBAC tests uses simple string splitting (`rest_endpoint.split(":")`) for endpoint parsing by design. This implementation is intentionally non-robust to serve as a detection mechanism for any unexpected changes to the endpoint format.
🧬 Code Graph Analysis (1)
tests/model_registry/rest_api/conftest.py (4)
tests/model_registry/rest_api/utils.py (2)
get_mr_deployment(268-269)register_model_rest_api(58-91)tests/model_registry/utils.py (2)
get_mr_standard_labels(488-494)get_mysql_config(532-540)tests/model_registry/conftest.py (4)
model_registry_rest_url(487-489)model_registry_rest_headers(493-498)model_registry_db_secret(125-148)model_registry_namespace(56-57)tests/conftest.py (1)
admin_client(65-66)
🔇 Additional comments (7)
tests/model_registry/conftest.py (4)
1-1: Good addition of ExitStack import.The ExitStack import is essential for the multi-resource management pattern being implemented.
70-95: Consistent multi-resource pattern implementation.The fixture follows a good pattern for multi-resource management using ExitStack. The implementation properly handles both post-upgrade and normal scenarios.
240-253: Good implementation for multiple endpoint support.The fixture correctly handles multiple Model Registry instances and their corresponding endpoints. The assertion ensures at least one instance exists before proceeding.
360-385: Well-implemented multi-client support.The fixture correctly creates multiple ModelRegistryClient instances from the list of endpoints, maintaining consistency in client configuration.
tests/model_registry/rest_api/conftest.py (3)
11-11: Good addition of utility function imports.The new imports support the refactored fixture implementations and align with the multi-resource pattern.
Also applies to: 20-22, 25-25
124-146: Fixture correctly adapted to list-based pattern.The fixture properly handles the new list-based secret parameter and uses appropriate constants.
149-180: Good parameterization for SSL configuration.The fixture correctly adds parameterization support for SSL configuration while maintaining the existing functionality. The use of utility functions is consistent with the refactoring pattern.
Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
rhods-ci-bot
added
commented-by-dbasunag
and removed
commented-by-adolfo-ab
commented-by-lugi0
commented-by-dbasunag
commented-by-fege
labels
rhods-ci-bot
added
commented-by-dbasunag
and removed
commented-by-lugi0
commented-by-dbasunag
labels
adolfo-ab
previously approved these changes
Aug 5, 2025
adolfo-ab
approved these changes
Aug 5, 2025
Collaborator
Author
|
/build-push-pr-image |
|
Status of building tag pr-429: success. |
|
Status of building tag latest: success. |
3 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
How Has This Been Tested?
Merge criteria: