Add end to end test for MR async job #487
Conversation
Signed-off-by: lugi0 <[email protected]>
📝 WalkthroughWalkthroughAdds async model-upload test infrastructure: constants, utilities, a Job subclass with volume support, fixtures to create S3/OCI secrets and Jobs, helpers to seed MinIO and interact with OCI, a new E2E test, SA-scoped token/RBAC fixtures in model_registry conftest, and a shift to group-based fixtures in rbac conftest. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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{'/build-push-pr-image', '/verified', '/cherry-pick', '/wip', '/lgtm', '/hold'} |
Signed-off-by: lugi0 <[email protected]>
Signed-off-by: lugi0 <[email protected]>
|
@mwaykole only way I can think of to make this work in disconnected environments |
|
/build-push-pr-image |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/async_job/utils.py (1)
90-95: Handle absolute vs relative Location header and assert presenceThe Distribution spec allows Location to be absolute or relative. Current concatenation can break on absolute URLs. Also assert that Location exists.
- upload_location = upload_response.headers.get("Location") - LOGGER.info(f"Upload location: {upload_location}") - base_url = f"{registry_url}{upload_location}" + upload_location = upload_response.headers.get("Location") + assert upload_location, "Missing Location header in blob upload initiation response" + LOGGER.info(f"Upload location: {upload_location}") + # Support both absolute and relative Location values + base_url = urljoin(f"{registry_url.rstrip('/')}/", upload_location) upload_url = f"{base_url}?digest={blob_digest}"
♻️ Duplicate comments (2)
tests/model_registry/async_job/utils.py (1)
16-31: Refresh Job before reading status and short-circuit on failuresAvoid stale reads and return early if the controller has already recorded a failure.
Apply:
def check_job_completion(admin_client: DynamicClient, job: Job) -> bool: @@ - job_status = job.instance.status + # Ensure we read the latest Job status from the API server + job.refresh() + job_status = getattr(job.instance, "status", None) @@ if hasattr(job_status, "succeeded") and job_status.succeeded: LOGGER.info("Job completed successfully") return True + + # If controller has recorded failures, report immediately + if hasattr(job_status, "failed") and job_status.failed: + LOGGER.warning(f"Job reports failed={job_status.failed}") + return Falsetests/model_registry/async_job/conftest.py (1)
101-111: Fix double-JSON encoding in OCI secretACCESS_TYPE and OCI_HOST are JSON-dumped before base64, resulting in extra quotes when consumed from mounted files. Encode raw strings instead.
with Secret( @@ - data_dict={ - ".dockerconfigjson": b64_encoded_string(json.dumps(dockerconfig)), - "ACCESS_TYPE": b64_encoded_string(json.dumps('["Push,Pull"]')), - "OCI_HOST": b64_encoded_string(json.dumps(f"{oci_registry_host}:{OCIRegistry.Metadata.DEFAULT_PORT}")), - }, + data_dict={ + ".dockerconfigjson": b64_encoded_string(json.dumps(dockerconfig)), + "ACCESS_TYPE": b64_encoded_string('["Push","Pull"]'), + "OCI_HOST": b64_encoded_string(f"{oci_registry_host}:{OCIRegistry.Metadata.DEFAULT_PORT}"), + }, type="kubernetes.io/dockerconfigjson",Note: Mixing extra keys into a dockerconfigjson-type Secret is unconventional but works; keep if the job expects these as files next to .dockerconfigjson.
🧹 Nitpick comments (3)
tests/model_registry/async_job/utils.py (2)
1-3: Import urljoin for robust Location handlingPrepare to safely combine registry_url with Location header values that may be absolute or relative.
import hashlib import requests import json +from urllib.parse import urljoin
102-111: Use byte length for config.sizeManifest sizes are byte counts; using len(config_json) on a str can be wrong for non-ASCII. Encode to UTF-8 first.
"config": { "mediaType": "application/vnd.oci.image.config.v1+json", - "size": len(config_json), + "size": len(config_json.encode("utf-8")), "digest": config_digest, },tests/model_registry/conftest.py (1)
335-377: Minor: drop unused variables from run_command resultres and err aren’t used; switch them to underscores for clarity.
- res, out, err = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30) + _, out, _ = run_command(command=shlex.split(cmd), verify_stderr=False, check=True, timeout=30)
📜 Review details
Configuration used: .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 (5)
tests/model_registry/async_job/conftest.py(2 hunks)tests/model_registry/async_job/constants.py(1 hunks)tests/model_registry/async_job/test_async_upload_e2e.py(1 hunks)tests/model_registry/async_job/utils.py(2 hunks)tests/model_registry/conftest.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/model_registry/async_job/constants.py
- tests/model_registry/async_job/test_async_upload_e2e.py
🧰 Additional context used
🧠 Learnings (14)
📓 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: 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-08-08T16:00:51.421Z
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).
Applied to files:
tests/model_registry/async_job/utils.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/utils.pytests/model_registry/async_job/conftest.py
📚 Learning: 2025-08-08T16:00:02.528Z
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.
Applied to files:
tests/model_registry/async_job/utils.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/utils.pytests/model_registry/async_job/conftest.py
📚 Learning: 2025-06-05T14:32:40.247Z
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.
Applied to files:
tests/model_registry/async_job/utils.pytests/model_registry/async_job/conftest.py
📚 Learning: 2025-07-30T14:15:25.605Z
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#429
File: tests/model_registry/rbac/test_mr_rbac_sa.py:45-45
Timestamp: 2025-07-30T14:15:25.605Z
Learning: In tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_endpoint list access is not needed because upstream fixture validation already ensures endpoints exist before the tests execute. The Model Registry setup process validates endpoint availability, making additional bounds checks redundant.
Applied to files:
tests/model_registry/conftest.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/conftest.py
📚 Learning: 2025-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/conftest.pytests/model_registry/async_job/conftest.py
📚 Learning: 2025-06-16T11:25:39.599Z
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.
Applied to files:
tests/model_registry/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-05-05T13:48:02.409Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#283
File: tests/model_registry/test_user_permission.py:31-31
Timestamp: 2025-05-05T13:48:02.409Z
Learning: In the opendatahub-tests repository, bypassing TLS verification with `--insecure-skip-tls-verify=true` is an acceptable practice in test environments, particularly in the model registry tests.
Applied to files:
tests/model_registry/async_job/conftest.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/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
🧬 Code Graph Analysis (3)
tests/model_registry/async_job/utils.py (3)
utilities/constants.py (7)
MinIo(272-332)PodConfig(255-262)PodConfig(288-328)Metadata(250-253)Metadata(273-276)Credentials(278-282)Buckets(284-286)utilities/general.py (1)
b64_encoded_string(55-68)tests/conftest.py (2)
admin_client(66-67)minio_service(522-540)
tests/model_registry/conftest.py (3)
tests/model_registry/utils.py (1)
generate_namespace_name(319-320)utilities/general.py (1)
generate_random_name(308-333)tests/conftest.py (1)
admin_client(66-67)
tests/model_registry/async_job/conftest.py (6)
tests/model_registry/conftest.py (6)
service_account(325-332)sa_token(336-376)model_registry_namespace(60-61)model_registry_instance(65-98)mr_access_role_binding(416-452)model_registry_client(219-245)utilities/constants.py (6)
OCIRegistry(249-269)MinIo(272-332)Protocols(91-98)Labels(180-209)Credentials(278-282)Buckets(284-286)utilities/general.py (1)
b64_encoded_string(55-68)tests/model_registry/async_job/utils.py (2)
get_async_job_s3_secret_dict(139-166)upload_test_model_to_minio_from_image(169-253)tests/model_registry/utils.py (2)
get_mr_service_by_label(41-65)get_endpoint_from_mr_service(68-72)tests/conftest.py (3)
admin_client(66-67)minio_service(522-540)teardown_resources(87-94)
🔇 Additional comments (14)
tests/model_registry/async_job/utils.py (4)
65-70: LGTM: simple latest-pod selection is acceptable hereSorting by creationTimestamp string and selecting by label_selector="job-name=<job.name>" matches the agreed simplicity for these tests.
127-136: LGTM: manifest pull helperTimeouts and basic assertions are fine for test utilities. Accept header is OK as-is per test expectations.
139-167: LGTM: S3 secret dict helperEncodes keys appropriately for Secret.data usage.
169-254: LGTM: image-based MinIO uploader podSecure defaults on containers and explicit wait/logging are good for test robustness.
tests/model_registry/conftest.py (4)
311-322: LGTM: deterministic namespace per test fileUsing generate_namespace_name aligns with the repo’s pattern to expose cleanup issues via predictable names.
324-333: LGTM: ServiceAccount fixtureRandomized, scoped SA with clean teardown via context manager looks good.
379-413: LGTM: minimal Role granting service getScope to target namespace and resourceNames is appropriately narrow.
415-453: LGTM: RoleBinding to SA namespace groupBinding the namespace’s serviceaccounts group to the Role is a pragmatic approach for these tests.
tests/model_registry/async_job/conftest.py (6)
39-52: LGTM: JobWithVolumes shimPractical extension until upstream support lands in the wrapper.
54-80: LGTM: S3 Secret fixtureEndpoint derivation from Service and opaque secret data encoding are correct.
115-234: LGTM: async Job wiring
- Dynamic MR endpoint/OCI URI derivation is correct.
- Volume mounts and env wiring match the fixtures and constants.
- IS_SECURE=False with https is intentional in this repo’s tests.
353-357: LGTM: route host extractionWorks for building registry URLs in dependent fixtures.
359-372: LGTM: image-based MinIO test dataReuses the shared uploader helper and keeps object_key consistent with consumers.
374-388: LGTM: registered model fixtureStraightforward pass-through to client with paramized fields.
|
Status of building tag pr-487: success. |
rhods-ci-bot
added
build-push-pr-image
commented-by-fege
and removed
commented-by-adolfo-ab
labels
|
/build-push-pr-image |
|
Status of building tag pr-487: success. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/async_job/utils.py (1)
49-57: Handle absolute vs relative Location header when pushing blobsConcatenating registry_url + Location breaks if the registry returns an absolute URL in Location. Use urljoin to handle both absolute and relative values.
Apply:
- upload_location = upload_response.headers.get("Location") + upload_location = upload_response.headers.get("Location") LOGGER.info(f"Upload location: {upload_location}") - base_url = f"{registry_url}{upload_location}" - upload_url = f"{base_url}?digest={blob_digest}" + from urllib.parse import urljoin + base_url = urljoin(registry_url + "/", upload_location or "") + upload_url = f"{base_url}?digest={blob_digest}"Additionally, add at the top of this file:
from urllib.parse import urljoin # if you prefer module-level import
♻️ Duplicate comments (1)
tests/model_registry/async_job/conftest.py (1)
101-109: Fix double-encoding in OCI secret (ACCESS_TYPE, OCI_HOST)These two values are JSON-dumped before base64-encoding, adding extra quotes and escaping that will surprise consumers reading plain strings from the mounted files.
Apply:
with Secret( client=admin_client, name=f"async-job-oci-secret-{shortuuid.uuid().lower()}", namespace=service_account.namespace, data_dict={ ".dockerconfigjson": b64_encoded_string(json.dumps(dockerconfig)), - "ACCESS_TYPE": b64_encoded_string(json.dumps('["Push,Pull"]')), - "OCI_HOST": b64_encoded_string(json.dumps(f"{oci_registry_host}:{OCIRegistry.Metadata.DEFAULT_PORT}")), + "ACCESS_TYPE": b64_encoded_string('["Push","Pull"]'), + "OCI_HOST": b64_encoded_string(f"{oci_registry_host}:{OCIRegistry.Metadata.DEFAULT_PORT}"), }, type="kubernetes.io/dockerconfigjson", ) as secret: yield secret
🧹 Nitpick comments (7)
tests/model_registry/async_job/test_async_upload_e2e.py (4)
1-1: Confirm Python version for typing.Self (fallback to typing_extensions if needed)If your test runners include Python < 3.11, import Self from typing_extensions to avoid ImportError.
Apply:
-from typing import Self +try: + from typing import Self # Python 3.11+ +except ImportError: # pragma: no cover - compatibility with older runners + from typing_extensions import SelfWould you like me to scan the repo configuration for python_requires or CI matrix to confirm the minimum supported Python version?
25-30: Align model_storage_path with the uploaded MinIO object to avoid silent mismatchesThe fixture create_test_data_in_minio_from_image uploads to my-model/model.onnx, but MODEL_DATA sets storage_path to "path/to/test/model". If the async job reads the artifact’s storage_path from MR, this mismatch can cause unexpected failures or false positives.
Apply:
MODEL_DATA = { **MODEL_DICT, "model_name": MODEL_NAME, "model_storage_key": MODEL_SYNC_CONFIG["SOURCE_AWS_KEY"], - "model_storage_path": "path/to/test/model", + # Keep in sync with create_test_data_in_minio_from_image (object_key) + "model_storage_path": "my-model/model.onnx", }If "path/to/test/model" is intentional (e.g., the job overrides source), please add a short comment to clarify so future readers don’t align it incorrectly.
79-81: Build registry URL scheme from the Route (supports TLS and non-TLS)Routes may be terminated with TLS. Deriving the scheme from the Route resource avoids 302s or 403s in stricter setups.
Apply:
- registry_host = oci_registry_route.instance.spec.host - registry_url = f"http://{registry_host}" + registry_host = oci_registry_route.instance.spec.host + scheme = "https" if getattr(oci_registry_route.instance.spec, "tls", None) else "http" + registry_url = f"{scheme}://{registry_host}"
91-95: Nit: log message says “layer(s)” but you’re asserting an image index with “manifests”The assertion is correct (intentionally expecting an OCI index per maintainers). The log line should reflect “manifest(s)”.
Apply:
- LOGGER.info(f"Manifest contains {len(manifest['manifests'])} layer(s)") + LOGGER.info(f"Manifest contains {len(manifest['manifests'])} manifest(s)")tests/model_registry/async_job/utils.py (1)
65-74: Use byte lengths in manifest sizes for correctnessThe manifest’s size fields should be byte counts. For config_json, encode to UTF-8 before measuring.
Apply:
manifest = { "schemaVersion": 2, "mediaType": "application/vnd.oci.image.manifest.v1+json", "config": { "mediaType": "application/vnd.oci.image.config.v1+json", - "size": len(config_json), + "size": len(config_json.encode("utf-8")), "digest": config_digest, }, "layers": [{"mediaType": "application/vnd.oci.image.layer.v1.tar", "size": len(data), "digest": blob_digest}], }tests/model_registry/async_job/conftest.py (2)
43-52: JobWithVolumes shim is reasonable; consider upstreamingExtending Job to inject volumes in to_dict is fine for now. If you’d like, I can help upstream support to the wrapper to avoid this local subclass.
Do you want me to open an issue/PR on the wrapper with this minimal enhancement?
205-214: Clarify intentional TLS verification skip for MR clientUsing https with MODEL_SYNC_REGISTRY_IS_SECURE="False" is intentional (skip TLS verification for self-signed certs). A short inline comment will prevent future “cleanup” attempts.
Apply:
"env": [ # Proxy settings {"name": "HTTP_PROXY", "value": ""}, {"name": "HTTPS_PROXY", "value": ""}, {"name": "NO_PROXY", "value": "*.svc.cluster.local"}, @@ {"name": "MODEL_SYNC_REGISTRY_AUTHOR", "value": "RHOAI async job test"}, {"name": "MODEL_SYNC_REGISTRY_USER_TOKEN", "value": sa_token}, + # Intentionally False: we use HTTPS above but disable verification due to test env self-signed certs {"name": "MODEL_SYNC_REGISTRY_IS_SECURE", "value": "False"}, ],
📜 Review details
Configuration used: .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 (3)
tests/model_registry/async_job/conftest.py(2 hunks)tests/model_registry/async_job/test_async_upload_e2e.py(1 hunks)tests/model_registry/async_job/utils.py(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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.
📚 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.pytests/model_registry/async_job/utils.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/test_async_upload_e2e.pytests/model_registry/async_job/conftest.pytests/model_registry/async_job/utils.py
📚 Learning: 2025-08-08T16:00:51.421Z
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).
Applied to files:
tests/model_registry/async_job/test_async_upload_e2e.pytests/model_registry/async_job/utils.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/async_job/test_async_upload_e2e.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-07-04T00:17:47.799Z
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.
Applied to files:
tests/model_registry/async_job/conftest.py
📚 Learning: 2025-06-05T14:32:40.247Z
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.
Applied to files:
tests/model_registry/async_job/conftest.pytests/model_registry/async_job/utils.py
📚 Learning: 2025-05-05T13:48:02.409Z
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#283
File: tests/model_registry/test_user_permission.py:31-31
Timestamp: 2025-05-05T13:48:02.409Z
Learning: In the opendatahub-tests repository, bypassing TLS verification with `--insecure-skip-tls-verify=true` is an acceptable practice in test environments, particularly in the model registry tests.
Applied to files:
tests/model_registry/async_job/conftest.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/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-08T16:00:02.528Z
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.
Applied to files:
tests/model_registry/async_job/utils.py
🔇 Additional comments (3)
tests/model_registry/async_job/test_async_upload_e2e.py (1)
96-110: Model Registry validations look goodGood checks for LIVE states and artifact attributes. The URI shape oci://{host}/{repo} (no tag) matches the expected form in these tests.
tests/model_registry/async_job/utils.py (2)
15-33: get_latest_job_pod: simple and effective for this test scopeLabel selection and string-based creationTimestamp sort are acceptable per the agreed test guidelines for async-job utilities.
207-213: Good: correctly calling Pod.log()Nice catch on using upload_pod.log() (method) instead of attribute; this avoids returning a bound method object.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/model_registry/async_job/test_basic_oci_registry.py (1)
35-35: Avoid an unconditional skip; either remove the test, gate it, or at least add a traceable reason.Unconditionally skipping this test will likely leave dead code in the suite and silently reduce coverage. Given the new async E2E path expects an OCI image index (per team decision) while this legacy test asserts an image manifest, either remove this test now or gate it behind an env var/marker. At minimum, make the skip reason actionable (reference the tracking issue).
Minimal improvement to the skip reason:
- @pytest.mark.skip(reason="To remove soon") + @pytest.mark.skip(reason="Legacy OCI manifest test; superseded by async upload E2E (RHOAISTRAT-456). Remove once new flow is stable.")Optional: Gate behind an env var so it can still be run locally (requires adding an import at the top):
+import os- @pytest.mark.skip(reason="To remove soon") + @pytest.mark.skipif(os.getenv("RUN_LEGACY_OCI_REGISTRY_TESTS") != "1", + reason="Legacy OCI manifest test; superseded by async upload E2E (RHOAISTRAT-456). Set RUN_LEGACY_OCI_REGISTRY_TESTS=1 to run.")
📜 Review details
Configuration used: .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/async_job/test_basic_oci_registry.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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).
📚 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_basic_oci_registry.py
|
Status of building tag latest: success. |
When the S3/OCI fixtures are ready, we can simply:
Summary by CodeRabbit