Fetch upload job image dynamically from installed rhoai version#610
Merged
dbasunag merged 3 commits intoopendatahub-io:mainfrom Sep 16, 2025
Merged
Fetch upload job image dynamically from installed rhoai version#610dbasunag merged 3 commits intoopendatahub-io:mainfrom
dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Contributor
📝 WalkthroughWalkthroughTests now retrieve the async-upload container image at runtime from the cluster ConfigMap Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/cherry-pick', '/hold', '/verified', '/build-push-pr-image', '/lgtm'} |
![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: 0
🧹 Nitpick comments (2)
tests/model_registry/async_job/conftest.py (2)
117-148: Harden ConfigMap read; align exceptions with docstring; fix Ruff TRY003.
- Call get() before accessing instance to avoid None.
- Handle missing/empty data gracefully; keep exception types consistent with docstring.
- Shorten exception messages to satisfy TRY003.
Apply this diff:
@pytest.fixture(scope="class") def async_upload_image(admin_client: DynamicClient) -> str: @@ - Returns: - str: The async upload image URL from the ConfigMap - - Raises: - KeyError: If the ConfigMap or the required key doesn't exist + Returns: + str: The async upload image URL from the ConfigMap. + + Raises: + RuntimeError: If the ConfigMap doesn't exist. + KeyError: If the required key is missing. @@ - if not config_map.exists: - raise RuntimeError( - "ConfigMap 'model-registry-operator-parameters' not found in namespace 'redhat-ods-applications'" - ) + if not config_map.exists: + raise RuntimeError("ConfigMap not found") - try: - return config_map.instance.data["IMAGES_JOBS_ASYNC_UPLOAD"] - except KeyError as e: - raise KeyError(f"Key 'IMAGES_JOBS_ASYNC_UPLOAD' not found in ConfigMap data: {e}") from e + # Ensure instance is populated and handle missing/empty data + config_map.get() + data = getattr(config_map.instance, "data", None) or {} + try: + return data["IMAGES_JOBS_ASYNC_UPLOAD"] + except KeyError as e: + raise KeyError("Missing IMAGES_JOBS_ASYNC_UPLOAD") from e
248-251: Using the dynamically resolved image is correct.Optional: if the referenced tag may change frequently, consider setting imagePullPolicy="Always" to avoid stale pulls in long‑lived clusters.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/async_job/conftest.py(5 hunks)tests/model_registry/async_job/constants.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/model_registry/async_job/constants.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
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.
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T15:59:16.170Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-06-05T10:05:17.642Z
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.
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T15:58:03.769Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/conftest.py:210-219
Timestamp: 2025-08-08T15:58:03.769Z
Learning: In tests/model_registry/async_job/conftest.py, using https in MODEL_SYNC_REGISTRY_SERVER_ADDRESS together with MODEL_SYNC_REGISTRY_IS_SECURE="False" is intentional: IS_SECURE=False disables TLS verification for the Model Registry’s self-signed certificate in test environments. This pattern aligns with the repository’s accepted practice of skipping TLS verification in tests.
Applied to files:
tests/model_registry/async_job/conftest.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/async_job/conftest.py
🧬 Code graph analysis (1)
tests/model_registry/async_job/conftest.py (1)
tests/conftest.py (1)
admin_client(68-69)
🪛 Ruff (0.12.2)
tests/model_registry/async_job/conftest.py
140-142: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
tests/model_registry/async_job/conftest.py (3)
27-27: LGTM: ConfigMap import is appropriate.Importing ConfigMap from ocp_resources is correct for reading cluster params.
183-184: Docstring update reads well.Param description for async_upload_image is clear and matches usage.
161-163: Fixture wiring looks good — verify no lingering ASYNC_UPLOAD_IMAGE references.
async_upload_image DI is clean and backwards-safe. Repo-wide searches (rg --no-config -uu; grep over git-tracked files) returned no matches for ASYNC_UPLOAD_IMAGE, but the run reported missing git-tracked file warnings; confirm CI/workflows or generated/generated-artifact locations have no leftover references.
dbasunag
reviewed
Sep 15, 2025
| config_map = ConfigMap( | ||
| client=admin_client, | ||
| name="model-registry-operator-parameters", | ||
| namespace="redhat-ods-applications", |
Collaborator
There was a problem hiding this comment.
Please use py_config["applications_namespace"] instead, that way we won't fail against odh.
Contributor
Author
There was a problem hiding this comment.
see below comment on intended target for the test
Collaborator
There was a problem hiding this comment.
Testops has not implemented the --downstream-only marker yet. But here we should use py_config["applications_namespace"] instead of hardcoding the value.
dbasunag
reviewed
Sep 15, 2025
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_registry/async_job/conftest.py (1)
118-149: Make namespace configurable; skip gracefully if ConfigMap/key is absent; align docstring; tighten retrieval.
- Hardcoded namespace
redhat-ods-applicationswill break on ODH; usepy_config["applications_namespace"]as previously requested.- For downstream‑only behavior, prefer
pytest.skip(...)over throwing a type‑mismatchedResourceNotFoundError.- Docstring says “KeyError if ConfigMap or key doesn’t exist” but code raises two different exceptions; align it.
- Address Ruff TRY003 by avoiding long exception strings; skipping keeps messages concise.
@pytest.fixture(scope="class") -def async_upload_image(admin_client: DynamicClient) -> str: +def async_upload_image(admin_client: DynamicClient, py_config: dict[str, Any]) -> str: """ - Get the async upload image dynamically from the model-registry-operator-parameters ConfigMap. - - This fetches the image from the cluster at runtime instead of using a hardcoded value. - - Args: - admin_client: Kubernetes client for resource access - - Returns: - str: The async upload image URL from the ConfigMap - - Raises: - KeyError: If the ConfigMap or the required key doesn't exist + Resolve the async-upload image from the model-registry-operator-parameters + ConfigMap in the applications namespace. Skip the test if unavailable. + + Args: + admin_client: Kubernetes client for resource access + py_config: Repository test configuration mapping + + Returns: + str: The async-upload image URL from the ConfigMap """ - config_map = ConfigMap( + namespace = py_config["applications_namespace"] + config_map = ConfigMap( client=admin_client, name="model-registry-operator-parameters", - namespace="redhat-ods-applications", + namespace=namespace, ) - if not config_map.exists: - raise ResourceNotFoundError( - "ConfigMap 'model-registry-operator-parameters' not found in namespace 'redhat-ods-applications'" - ) + if not config_map.exists: + pytest.skip( + f"ConfigMap 'model-registry-operator-parameters' not found in namespace '{namespace}'" + ) - try: - return config_map.instance.data["IMAGES_JOBS_ASYNC_UPLOAD"] - except KeyError as e: - raise KeyError(f"Key 'IMAGES_JOBS_ASYNC_UPLOAD' not found in ConfigMap data: {e}") from e + image = (config_map.instance.data or {}).get("IMAGES_JOBS_ASYNC_UPLOAD") + if not image: + pytest.skip("Key 'IMAGES_JOBS_ASYNC_UPLOAD' missing or empty in ConfigMap data") + return imageTo ensure nothing still references the removed constant, run:
#!/bin/bash # Expect: no matches for the old constant name anywhere rg -nP '\bASYNC_UPLOAD_IMAGE\b' --glob '!**/.venv/**' --glob '!**/dist/**' || true
🧹 Nitpick comments (3)
tests/model_registry/async_job/test_async_upload_e2e.py (2)
135-135: Fix stale log message.Update to reflect the async-upload job image rather than KSERVE_MINIO_IMAGE.
- LOGGER.info("Async upload job test with KSERVE_MINIO_IMAGE: PASSED") + LOGGER.info("Async upload job test with async-upload image: PASSED")
61-61: Downstream-only marker is registered — keep for now; prefer fixture-based autodetection long‑term.pytest.ini declares
downstream_only; marker is used in tests/model_registry/image_validation/test_verify_rhoai_images.py:20 and tests/model_registry/async_job/test_async_upload_e2e.py:61. Keep this mark to avoid PytestUnknownMarkWarning; when the fixture can select the correct image for both ODH/RHOAI, remove the class-level mark and use fixture skip logic.tests/model_registry/async_job/conftest.py (1)
6-6: Don’t raise/import ResourceNotFoundError for missing instance; prefer pytest.skip here.
ResourceNotFoundErrorinkubernetes.dynamic.exceptionsrepresents a missing resource type, not a missing instance. In tests, skipping when the ConfigMap isn’t present is clearer and avoids hard failures outside downstream clusters. Remove this import and handle absence withpytest.skipin the fixture below.-from kubernetes.dynamic.exceptions import ResourceNotFoundError
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/model_registry/async_job/conftest.py(6 hunks)tests/model_registry/async_job/test_async_upload_e2e.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
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#487
File: tests/model_registry/async_job/utils.py:168-216
Timestamp: 2025-08-08T16:00:02.528Z
Learning: In tests/model_registry/async_job/utils.py, embedding the small mnist-8.onnx model as base64 in the initContainer args is acceptable for now per maintainer (lugi0); size limits are not a concern and no refactor is required at this time.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/utils.py:14-32
Timestamp: 2025-08-08T16:00:51.421Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job, keep get_latest_job_pod (tests/model_registry/async_job/utils.py) simple: sorting by creationTimestamp string and selecting pods via label_selector="job-name=<job.name>" is acceptable; do not add datetime parsing or controller-uid filtering unless a concrete failure arises (per guidance from user lugi0).
📚 Learning: 2025-08-08T15:59:16.170Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Applied to files:
tests/model_registry/async_job/test_async_upload_e2e.pytests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-06-05T10:05:17.642Z
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.
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T15:58:03.769Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/conftest.py:210-219
Timestamp: 2025-08-08T15:58:03.769Z
Learning: In tests/model_registry/async_job/conftest.py, using https in MODEL_SYNC_REGISTRY_SERVER_ADDRESS together with MODEL_SYNC_REGISTRY_IS_SECURE="False" is intentional: IS_SECURE=False disables TLS verification for the Model Registry’s self-signed certificate in test environments. This pattern aligns with the repository’s accepted practice of skipping TLS verification in tests.
Applied to files:
tests/model_registry/async_job/conftest.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/async_job/conftest.py
🧬 Code graph analysis (1)
tests/model_registry/async_job/conftest.py (1)
tests/conftest.py (1)
admin_client(68-69)
🪛 Ruff (0.12.2)
tests/model_registry/async_job/conftest.py
141-143: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
tests/model_registry/async_job/conftest.py (2)
28-28: LGTM: ConfigMap import is appropriate.
162-162: Wiring looks correct.Passing
async_upload_imageinto the job and using it for the container image is the right integration point.Also applies to: 249-249
Signed-off-by: lugi0 <lgiorgi@redhat.com>
dbasunag
approved these changes
Sep 16, 2025
|
Status of building tag latest: success. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/model_registry/async_job/conftest.py (1)
19-19: Good: namespace sourced from py_config["applications_namespace"].Aligns with prior feedback to avoid hardcoding and to work against ODH/RHOAI.
🧹 Nitpick comments (1)
tests/model_registry/async_job/conftest.py (1)
6-6: Prefer project-scoped ResourceNotFoundError; replace import in async_job/conftest.py and audit other occurrences.Use tests.model_registry.model_catalog.utils.ResourceNotFoundError (repo-defined) instead of kubernetes.dynamic.exceptions.ResourceNotFoundError.
Apply this diff:
-from kubernetes.dynamic.exceptions import ResourceNotFoundError +from tests.model_registry.model_catalog.utils import ResourceNotFoundErrorrg shows additional kubernetes imports in tests/model_registry/utils.py and tests/model_registry/conftest.py — review those files and decide whether to standardize or keep kubernetes's exception where actual client exceptions are handled.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_registry/async_job/conftest.py(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
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#487
File: tests/model_registry/async_job/utils.py:168-216
Timestamp: 2025-08-08T16:00:02.528Z
Learning: In tests/model_registry/async_job/utils.py, embedding the small mnist-8.onnx model as base64 in the initContainer args is acceptable for now per maintainer (lugi0); size limits are not a concern and no refactor is required at this time.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/utils.py:14-32
Timestamp: 2025-08-08T16:00:51.421Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job, keep get_latest_job_pod (tests/model_registry/async_job/utils.py) simple: sorting by creationTimestamp string and selecting pods via label_selector="job-name=<job.name>" is acceptable; do not add datetime parsing or controller-uid filtering unless a concrete failure arises (per guidance from user lugi0).
📚 Learning: 2025-08-08T15:58:03.524Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/constants.py:23-31
Timestamp: 2025-08-08T15:58:03.524Z
Learning: In opendatahub-io/opendatahub-tests, for tests under tests/model_registry/async_job (e.g., constants.py and related fixtures), runs start from a clean, known Model Registry state. Therefore, using static MODEL_ID, MODEL_VERSION_ID, and MODEL_ARTIFACT_ID values in MODEL_SYNC_CONFIG is intentional and acceptable; dynamic ID injection is not required for these async job tests (per guidance from user lugi0).
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T15:59:16.170Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/test_async_upload_e2e.py:344-356
Timestamp: 2025-08-08T15:59:16.170Z
Learning: In opendatahub-io/opendatahub-tests, tests/model_registry/async_job/test_async_upload_e2e.py’s test_end_to_end_async_upload_job intentionally expects an OCI image index (application/vnd.oci.image.index.v1+json) from pull_manifest_from_oci_registry and asserts on the "manifests" array. This format has been verified by the author (lugi0); no change to Accept headers or assertions is required.
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T15:58:03.769Z
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#487
File: tests/model_registry/async_job/conftest.py:210-219
Timestamp: 2025-08-08T15:58:03.769Z
Learning: In tests/model_registry/async_job/conftest.py, using https in MODEL_SYNC_REGISTRY_SERVER_ADDRESS together with MODEL_SYNC_REGISTRY_IS_SECURE="False" is intentional: IS_SECURE=False disables TLS verification for the Model Registry’s self-signed certificate in test environments. This pattern aligns with the repository’s accepted practice of skipping TLS verification in tests.
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-06-05T10:05:17.642Z
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.
Applied to files:
tests/model_registry/async_job/conftest.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/async_job/conftest.py
🧬 Code graph analysis (1)
tests/model_registry/async_job/conftest.py (2)
tests/model_registry/model_catalog/utils.py (1)
ResourceNotFoundError(22-23)tests/conftest.py (1)
admin_client(68-69)
🪛 Ruff (0.12.2)
tests/model_registry/async_job/conftest.py
142-144: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
tests/model_registry/async_job/conftest.py (4)
119-151: Upstream/Downstream compatibility: consider a fallback instead of hard-failing.If the ConfigMap isn’t present on ODH clusters, either:
- Fallback to resolving from the installed CSV relatedImages, or
- Skip with pytest.skip and a clear message.
This prevents upstream runs from failing until a unified resolver is implemented.
Would you like a follow-up patch to resolve from CSV.relatedImages first and use the ConfigMap as a fallback?
163-165: Fixture wiring LGTM.Passing async_upload_image into model_sync_async_job keeps the job spec source‑of‑truth in one place.
250-253: Correct image source.Using the dynamically resolved async_upload_image in the container spec looks good.
119-151: Harden async_upload_image: fix docstring, handle empty .data, and simplify errors (resolves Ruff TRY003/RUF100).ConfigMap.data may be None; docstring currently states the wrong exception and the long f-string triggers lints. Apply the diff below and re-check for leftover references.
@pytest.fixture(scope="class") def async_upload_image(admin_client: DynamicClient) -> str: """ Get the async upload image dynamically from the model-registry-operator-parameters ConfigMap. This fetches the image from the cluster at runtime instead of using a hardcoded value. Args: admin_client: Kubernetes client for resource access Returns: str: The async upload image URL from the ConfigMap - Raises: - KeyError: If the ConfigMap or the required key doesn't exist + Raises: + ResourceNotFoundError: If the ConfigMap is missing or the required key isn't set """ - config_map = ConfigMap( + ns = py_config["applications_namespace"] + config_map = ConfigMap( client=admin_client, name="model-registry-operator-parameters", - namespace=py_config["applications_namespace"], + namespace=ns, ) - if not config_map.exists: - raise ResourceNotFoundError( - f"ConfigMap 'model-registry-operator-parameters' not found in namespace '{py_config['applications_namespace']}'" # noqa: E501 - ) + if not config_map.exists: + raise ResourceNotFoundError(f"ConfigMap 'model-registry-operator-parameters' not found in '{ns}'") - try: - return config_map.instance.data["IMAGES_JOBS_ASYNC_UPLOAD"] - except KeyError as e: - raise KeyError(f"Key 'IMAGES_JOBS_ASYNC_UPLOAD' not found in ConfigMap data: {e}") from e + data = (getattr(config_map.instance, "data", None) or {}) + image = data.get("IMAGES_JOBS_ASYNC_UPLOAD") + if not image: + raise ResourceNotFoundError("Key 'IMAGES_JOBS_ASYNC_UPLOAD' not set in ConfigMap data") + return imageThe prior verification run returned "No files were searched". Run this corrected check and paste the output to confirm there are no stale references:
#!/bin/bash rg -nP '\b(ASYNC_UPLOAD_IMAGE|IMAGES_JOBS_ASYNC_UPLOAD)\b' -S || true
sheltoncyril
pushed a commit
to sheltoncyril/opendatahub-tests
that referenced
this pull request
Oct 2, 2025
…datahub-io#610) * fix: Fetch upload job image dynamically from installed rhoai version Signed-off-by: lugi0 <lgiorgi@redhat.com> * fix: address review comments Signed-off-by: lugi0 <lgiorgi@redhat.com> * fix: use pyconfig applications namespace Signed-off-by: lugi0 <lgiorgi@redhat.com> --------- Signed-off-by: lugi0 <lgiorgi@redhat.com>
mwaykole
pushed a commit
to mwaykole/opendatahub-tests
that referenced
this pull request
Jan 23, 2026
…datahub-io#610) * fix: Fetch upload job image dynamically from installed rhoai version Signed-off-by: lugi0 <lgiorgi@redhat.com> * fix: address review comments Signed-off-by: lugi0 <lgiorgi@redhat.com> * fix: use pyconfig applications namespace Signed-off-by: lugi0 <lgiorgi@redhat.com> --------- Signed-off-by: lugi0 <lgiorgi@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
3 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
Tests
Chores