Add validation for MR to be ready after it is GA#574
Merged
dbasunag merged 6 commits intoopendatahub-io:mainfrom Sep 15, 2025
Merged
Add validation for MR to be ready after it is GA#574dbasunag merged 6 commits intoopendatahub-io:mainfrom
dbasunag merged 6 commits intoopendatahub-io:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new pytest module that verifies the Data Science Cluster's Model Registry managementState is MANAGED, its registries namespace exists and is Active, and the Model Registry readiness condition in the DSC status is True. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/build-push-pr-image', '/wip', '/lgtm', '/cherry-pick', '/hold'} |
![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: 2
🧹 Nitpick comments (2)
tests/model_registry/cluster_health/test_mr_default.py (2)
31-37: Defensive check for missing DSC status.conditionsIf status or conditions is absent, the loop raises; fail cleanly with a clear reason.
- for condition in dsc_resource.instance.status.conditions: + conditions = getattr(getattr(dsc_resource.instance, "status", None), "conditions", None) + if not conditions: + pytest.fail("DSC status.conditions missing; MR not ready") + for condition in conditions: if condition.type == DscComponents.COMPONENT_MAPPING[DscComponents.MODELREGISTRY]: LOGGER.info(f"MR ready in DSC: {condition.status}") assert condition.status == "True" break else: pytest.fail("MR not ready in DSC")
23-30: Improve debuggability with assertion messages (optional)Adding short messages helps triage when this fails in CI.
- assert ( + assert ( dsc_resource.instance.spec.components[DscComponents.MODELREGISTRY].managementState == DscComponents.ManagementState.MANAGED - ) + ), "Expected MR managementState=Managed in DSC" @@ - assert namespace.instance.status.phase == Namespace.Status.ACTIVE + assert namespace.instance.status.phase == Namespace.Status.ACTIVE, "MR namespace not Active"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/model_registry/cluster_health/test_mr_default.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
🧬 Code graph analysis (1)
tests/model_registry/cluster_health/test_mr_default.py (3)
utilities/infra.py (1)
get_data_science_cluster(868-869)utilities/constants.py (2)
DscComponents(156-177)ManagementState(162-164)tests/conftest.py (2)
admin_client(66-67)dsc_resource(415-416)
🔇 Additional comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
15-21: LGTM: Scope and checks align with MR GA readiness validationThe three checks (managementState, namespace phase, readiness condition) are appropriate for post-GA validation.
lugi0
reviewed
Sep 2, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
22-26: Use the configured DSC name (or the dsc_resource fixture) to avoid targeting the wrong clusterCalling get_data_science_cluster without dsc_name defaults to "default-dsc", which may not match the environment’s DSC.
- dsc_resource = get_data_science_cluster(client=admin_client) + dsc_resource = get_data_science_cluster( + client=admin_client, dsc_name=py_config["dsc_name"] + )(Optional) Alternatively, inject the existing dsc_resource fixture and drop this call.
🧹 Nitpick comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
34-40: Optional: Clearer failure when DSC conditions are missingGuarding against absent/empty conditions yields a more actionable failure than a TypeError from iterating None.
- for condition in dsc_resource.instance.status.conditions: + conditions = getattr(dsc_resource.instance.status, "conditions", None) + assert conditions, "DSC status.conditions is empty" + for condition in conditions: if condition.type == DscComponents.COMPONENT_MAPPING[DscComponents.MODELREGISTRY]: LOGGER.info(f"MR ready in DSC: {condition.status}") assert condition.status == "True" break else: pytest.fail("MR not ready in DSC")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/model_registry/cluster_health/test_mr_default.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-02T07:29:44.457Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#574
File: tests/model_registry/cluster_health/test_mr_default.py:28-29
Timestamp: 2025-09-02T07:29:44.457Z
Learning: In tests/model_registry/cluster_health/test_mr_default.py, passing admin_client to Namespace constructor is not necessary according to fege, indicating the current approach without explicit client parameter is acceptable for this test.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-07-17T15:43:04.876Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-07-17T15:43:04.876Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-06-16T11:26:53.789Z
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.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
🧬 Code graph analysis (1)
tests/model_registry/cluster_health/test_mr_default.py (3)
utilities/infra.py (1)
get_data_science_cluster(868-869)utilities/constants.py (2)
DscComponents(156-177)ManagementState(162-164)tests/conftest.py (2)
admin_client(66-67)dsc_resource(415-416)
🔇 Additional comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
31-33: LGTM: Validates registries namespace matches configured expectationAsserting the resolved registries namespace equals py_config["model_registry_namespace"] addresses earlier feedback and prevents drift.
dbasunag
reviewed
Sep 2, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
25-31: Don’t mutate cluster state in a health check; also guard for missing NamespaceRemove ensure_exists=True so the test doesn’t create the namespace, and assert instance is present before reading status. (Acknowledging prior feedback; not adding client=admin_client per learning that it’s unnecessary here.)
- namespace = Namespace( - name=dsc_resource.instance.spec.components[DscComponents.MODELREGISTRY].registriesNamespace, - ensure_exists=True, - ) - assert namespace.instance.status.phase == Namespace.Status.ACTIVE + namespace = Namespace( + name=dsc_resource.instance.spec.components[DscComponents.MODELREGISTRY].registriesNamespace + ) + assert namespace.instance is not None, "MR registriesNamespace not found; health check must not create it" + assert namespace.instance.status.phase == Namespace.Status.ACTIVE assert namespace.instance.metadata.name == py_config["model_registry_namespace"]
🧹 Nitpick comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
32-39: Make readiness-condition lookup robust and clearerHandle absent status/conditions and avoid the for-else by selecting the condition directly.
- for condition in dsc_resource.instance.status.conditions: - if condition.type == DscComponents.COMPONENT_MAPPING[DscComponents.MODELREGISTRY]: - assert condition.status == "True" - break - else: - pytest.fail("MR ready condition not found in DSC") + expected_type = DscComponents.COMPONENT_MAPPING[DscComponents.MODELREGISTRY] + conditions = (getattr(dsc_resource.instance, "status", None) and dsc_resource.instance.status.conditions) or [] + cond = next((c for c in conditions if c.type == expected_type), None) + assert cond is not None, f"{expected_type} condition not found in DSC status" + assert cond.status == "True"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/model_registry/cluster_health/test_mr_default.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-02T07:29:44.473Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#574
File: tests/model_registry/cluster_health/test_mr_default.py:28-29
Timestamp: 2025-09-02T07:29:44.473Z
Learning: In tests/model_registry/cluster_health/test_mr_default.py, passing admin_client to Namespace constructor is not necessary according to fege, indicating the current approach without explicit client parameter is acceptable for this test.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-08-11T12:14:34.102Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-08-21T19:08:14.982Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#527
File: tests/model_registry/rbac/conftest.py:358-359
Timestamp: 2025-08-21T19:08:14.982Z
Learning: In tests/model_registry/rbac/conftest.py, the model catalog configmap only exists in one specific namespace (py_config["model_registry_namespace"]) regardless of where model registry instances are deployed, making it a shared resource that serves all model registry instances across namespaces.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-07-17T15:43:04.876Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, Service resources can be created without explicitly passing the admin_client parameter when using the context manager approach with "with Service()". The client parameter is optional for Service resource creation.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
📚 Learning: 2025-07-17T15:43:04.876Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:579-591
Timestamp: 2025-07-17T15:43:04.876Z
Learning: In tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the admin_client parameter for Service resource creation, despite the existing model_registry_db_service fixture using client=admin_client. This inconsistency was confirmed as intentional by user lugi0.
Applied to files:
tests/model_registry/cluster_health/test_mr_default.py
🧬 Code graph analysis (1)
tests/model_registry/cluster_health/test_mr_default.py (2)
utilities/constants.py (2)
DscComponents(156-177)ManagementState(162-164)tests/conftest.py (1)
dsc_resource(415-416)
🔇 Additional comments (1)
tests/model_registry/cluster_health/test_mr_default.py (1)
16-21: LGTM: managementState check is correct and targetedCompares against the canonical constant and uses the injected DSC fixture.
adolfo-ab
approved these changes
Sep 9, 2025
dbasunag
approved these changes
Sep 9, 2025
lugi0
approved these changes
Sep 10, 2025
|
Status of building tag latest: success. |
mwaykole
pushed a commit
to mwaykole/opendatahub-tests
that referenced
this pull request
Jan 23, 2026
* change: add validation for MR to be ready after it is GA * change: use registerd namespace * change: assert ns name based on distribution * change: split test and use fixture for the dsc_resource --------- Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
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
5 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:
Summary by CodeRabbit