Add image validation for TrustyAI Operator and service image#334
Add image validation for TrustyAI Operator and service image#334sheltoncyril wants to merge 6 commits intoopendatahub-io:mainfrom
Conversation
WalkthroughA new test for validating TrustyAI operator and service images was introduced, including utility functions and supporting fixtures. The Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TestValidateTrustyAIImages
participant Client as admin_client
participant ConfigMap as trustyai_operator_configmap
participant Utils as validate_trustyai_operator_image
participant Deployment as TrustyAI Operator Deployment
Test->>Client: Use admin_client fixture
Test->>ConfigMap: Access trustyai_operator_configmap fixture
Test->>Utils: Call validate_trustyai_operator_image(client, related_images_refs, configmap_data)
Utils->>Deployment: Get operator deployment resource
Utils->>Utils: Extract container image
Utils->>Utils: Assert image matches configmap, is in related_images_refs, and is properly formatted
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/wip', '/verified', '/hold'} |
There was a problem hiding this comment.
I think these tests should go under trustyai_service/test_trustyai_service.py, since we're checking the operator images as well. I would create a new directory for all the tests that are "transversal", maybe model_explainability/trustyai_operator/test_trustyai_images.py, wdyt?
There was a problem hiding this comment.
I can do this but it would involve a lot of refactoring fixtures to a higher level especially for the service, do you recommend doing this for both or should I split the operator related fixtures and move it up?
Co-authored-by: Adolfo Aguirrezabal <aaguirre@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/conftest.py (1)
543-547: Add docstring and improve variable naming.The fixture implementation is correct but could benefit from documentation and clearer variable naming.
Apply this diff to improve the fixture:
@pytest.fixture(scope="session") def related_images_refs(admin_client: DynamicClient) -> Set[str]: + """Extract image references from CSV related images. + + Returns: + Set of image URLs from the ClusterServiceVersion related images. + """ related_images = get_csv_related_images(admin_client=admin_client) - related_images_refs = {img["image"] for img in related_images} - return related_images_refs + image_refs = {img["image"] for img in related_images} + return image_refs🧰 Tools
🪛 Pylint (3.3.7)
[convention] 544-544: Missing function or method docstring
(C0116)
[warning] 544-544: Redefining name 'admin_client' from outer scope (line 54)
(W0621)
[warning] 546-546: Redefining name 'related_images_refs' from outer scope (line 544)
(W0621)
tests/model_explainability/trustyai_service/conftest.py (1)
460-469: Add docstring to the fixture.The fixture implementation is correct but needs documentation.
Apply this diff to add a docstring:
@pytest.fixture(scope="class") def trustyai_operator_configmap( admin_client: DynamicClient, ) -> ConfigMap: + """Provides access to the TrustyAI operator ConfigMap. + + Returns: + ConfigMap object for the TrustyAI operator configuration. + """ return ConfigMap( client=admin_client, namespace=py_config["applications_namespace"], name=f"{TRUSTYAI_SERVICE_NAME}-operator-config", ensure_exists=True, )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 461-461: Missing function or method docstring
(C0116)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/conftest.py(3 hunks)tests/model_explainability/trustyai_service/conftest.py(3 hunks)tests/model_explainability/trustyai_service/test_trustyai_service.py(2 hunks)tests/model_explainability/trustyai_service/utils.py(2 hunks)tests/model_registry/image_validation/conftest.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/model_registry/image_validation/conftest.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/conftest.py (1)
utilities/operator_utils.py (1)
get_csv_related_images(55-77)
tests/model_explainability/trustyai_service/utils.py (3)
utilities/exceptions.py (2)
TooManyPodsError(113-114)UnexpectedFailureError(117-118)utilities/general.py (1)
validate_image_format(197-212)tests/conftest.py (1)
related_images_refs(544-547)
🪛 Pylint (3.3.7)
tests/conftest.py
[convention] 544-544: Missing function or method docstring
(C0116)
[warning] 544-544: Redefining name 'admin_client' from outer scope (line 54)
(W0621)
[warning] 546-546: Redefining name 'related_images_refs' from outer scope (line 544)
(W0621)
tests/model_explainability/trustyai_service/conftest.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 24-24: Unable to import 'pytest_testconfig'
(E0401)
[convention] 461-461: Missing function or method docstring
(C0116)
tests/model_explainability/trustyai_service/test_trustyai_service.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 4-4: Unable to import 'ocp_resources.config_map'
(E0401)
[error] 5-5: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 6-6: Unable to import 'ocp_resources.trustyai_service'
(E0401)
[convention] 48-48: Line too long (108/100)
(C0301)
[convention] 52-52: Missing function or method docstring
(C0116)
[refactor] 52-52: Too many arguments (7/5)
(R0913)
[refactor] 52-52: Too many positional arguments (7/5)
(R0917)
[warning] 55-55: Unused argument 'current_client_token'
(W0613)
[warning] 56-56: Unused argument 'model_namespace'
(W0613)
[warning] 58-58: Unused argument 'trustyai_service_with_pvc_storage'
(W0613)
[refactor] 47-47: Too few public methods (1/2)
(R0903)
tests/model_explainability/trustyai_service/utils.py
[error] 4-4: Unable to import 'kubernetes.dynamic'
(E0401)
[error] 5-5: Unable to import 'ocp_resources.deployment'
(E0401)
[error] 6-6: Unable to import 'ocp_resources.maria_db'
(E0401)
[error] 7-7: Unable to import 'ocp_resources.mariadb_operator'
(E0401)
[error] 8-8: Unable to import 'ocp_resources.namespace'
(E0401)
[error] 9-9: Unable to import 'ocp_resources.pod'
(E0401)
[error] 10-10: Unable to import 'ocp_resources.trustyai_service'
(E0401)
[error] 11-11: Unable to import 'pytest_testconfig'
(E0401)
[error] 12-12: Unable to import 'simple_logger.logger'
(E0401)
[error] 13-13: Unable to import 'timeout_sampler'
(E0401)
[convention] 156-156: Line too long (101/100)
(C0301)
[convention] 162-162: Line too long (113/100)
(C0301)
🔇 Additional comments (1)
tests/model_explainability/trustyai_service/utils.py (1)
155-163: LGTM! Well-designed validation function.The function provides comprehensive validation of the TrustyAI operator image against multiple criteria, which aligns perfectly with the PR objectives to ensure image consistency with the ConfigMap.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 156-156: Line too long (101/100)
(C0301)
[convention] 162-162: Line too long (113/100)
(C0301)
| raise UnexpectedResourceCountError(f"Expected {expected_num_pods} pods, found {len(pods)}") | ||
| return pods | ||
| raise UnexpectedResourceCountError(f"Expected {expected_num_pods} pod(s), found {len(pods)}") | ||
| return pods[0] if expected_num_pods == 1 else pods |
There was a problem hiding this comment.
@lugi0 will this change be a problem? return the pod if only 1 is required and list of pods if many. I've changed your usages as well. Please LMK if it's a bad idea, I'll revert it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
utilities/general.py (2)
225-226: Python < 3.10 incompatibility in type annotationThe
list[Pod] | PodPEP-604 union syntax requires Python ≥ 3.10.
If CI/production is still on 3.9 (or earlier), this will raise aSyntaxError.-from typing import List, Tuple +from typing import List, Tuple, Union ... -) -> list[Pod] | Pod: +) -> Union[List[Pod], Pod]:Please confirm the runtime version and adjust accordingly.
249-252: Mixed return types hurt usability & static analysisReturning either a single
Podor alist[Pod]based onexpected_num_podsmakes every caller perform anisinstancecheck or rely on knowing the magic value passed in.
Consider always returning a list (of length 1) and letting the caller pick[0]when it truly wants a singleton. This keeps the API deterministic and mypy-friendly.If you want the ergonomic helper, expose a thin wrapper:
def wait_for_single_pod_by_labels(...): return wait_for_pods_by_labels(..., expected_num_pods=1)[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_explainability/trustyai_service/test_trustyai_service.py(2 hunks)tests/model_explainability/trustyai_service/utils.py(2 hunks)tests/model_registry/conftest.py(2 hunks)utilities/general.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_explainability/trustyai_service/test_trustyai_service.py
- tests/model_explainability/trustyai_service/utils.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code Graph Analysis (1)
utilities/general.py (1)
utilities/exceptions.py (1)
UnexpectedResourceCountError(121-122)
🔇 Additional comments (1)
tests/model_registry/conftest.py (1)
295-301: Fixture adaptation looks correctRemoving the
[0]index matches the new behaviour ofwait_for_pods_by_labelsand keeps the fixture type consistent. No further action required.
|
closing due to merge conflicts |
3 participants
Check if TrustyAI operator deployment oauth-proxy
and trustyai-service pods are using the right images from the configmap in redhat-ods-applications namespace.
Makes sure the TrustyAI deployment and service pods are created with the right images using the configmap as the source of truth.
Adresses RHOAIENG-22502 and RHOAIENG-23499
Description
Added a new test which validates whether the deployment/service pods have the same data in the image fields as the configmap.
How Has This Been Tested?
Test run successfully on a PSI cluster.
Merge criteria:
Summary by CodeRabbit
New Features
Tests
Chores