OCI registry and example of usage#493
Merged
fege merged 7 commits intoopendatahub-io:mainfrom Aug 6, 2025
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughNew pytest fixtures are introduced to set up an OCI Registry with MinIO S3 backend and OpenShift Route exposure for testing. Supporting utilities for pushing/pulling OCI blobs and manifests are added, along with a test verifying registry operations. Constants for OCI registry configuration are defined in a new class. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/build-push-pr-image', '/hold', '/cherry-pick', '/lgtm', '/verified'} |
mwaykole
reviewed
Aug 5, 2025
![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 (1)
tests/model_registry/async_job/test_basic_oci_registry.py (1)
31-56: LGTM! Comprehensive end-to-end test with good validation.The test performs a complete OCI registry workflow: blob push → manifest creation → manifest push → manifest pull → validation. The assertions properly verify the manifest structure and integrity.
Consider adding error handling for network failures:
def test_oci_registry_push_and_pull_operations( self, oci_registry_route: Route, ) -> None: """Test pushing and pulling content to/from the OCI registry with MinIO backend.""" + + if not oci_registry_route.instance.spec.host: + pytest.fail("OCI registry route host is not available") registry_host = oci_registry_route.instance.spec.host registry_url = f"http://{registry_host}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/conftest.py(3 hunks)tests/model_registry/async_job/test_basic_oci_registry.py(1 hunks)tests/model_registry/async_job/utils.py(1 hunks)utilities/constants.py(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: fege
PR: opendatahub-io/opendatahub-tests#320
File: tests/model_registry/rest_api/conftest.py:200-216
Timestamp: 2025-06-05T14:32:40.247Z
Learning: In the opendatahub-tests repository, the test fixtures should raise exceptions on cleanup failures rather than just logging warnings. The user fege prefers strict cleanup behavior where tests fail if cleanup doesn't work properly, rather than silently continuing.
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#401
File: tests/model_registry/rest_api/mariadb/conftest.py:89-110
Timestamp: 2025-07-04T00:17:47.799Z
Learning: In tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should always use OAUTH_PROXY_CONFIG_DICT for the oauth_proxy parameter regardless of the is_model_registry_oauth parameter value, based on expected product behavior for MariaDB-backed ModelRegistry instances.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#354
File: tests/model_registry/rbac/test_mr_rbac.py:64-77
Timestamp: 2025-06-16T11:26:53.789Z
Learning: In Model Registry RBAC tests, client instantiation tests are designed to verify the ability to create and use the MR python client, with actual API functionality testing covered by separate existing tests.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#338
File: tests/model_registry/rbac/test_mr_rbac.py:24-53
Timestamp: 2025-06-06T12:22:57.057Z
Learning: In the opendatahub-tests repository, prefer keeping test parameterization configurations inline rather than extracting them to separate variables/constants, as it makes triaging easier by avoiding the need to jump between different parts of the file to understand the test setup.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
📚 Learning: in model registry rbac tests, client instantiation tests are designed to verify the ability to creat...
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_basic_oci_registry.pytests/conftest.py
📚 Learning: in tests/model_registry/rest_api/mariadb/conftest.py, the model_registry_with_mariadb fixture should...
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/test_basic_oci_registry.pytests/conftest.py
📚 Learning: in tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fix...
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:733-770
Timestamp: 2025-07-17T15:42:23.880Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 and model_registry_instance_2 fixtures do not need explicit database dependency fixtures (like db_deployment_1, db_secret_1, etc.) in their function signatures. Pytest's dependency injection automatically handles the fixture dependencies when they reference db_name_1 and db_name_2 parameters. This is the correct pattern for these Model Registry instance fixtures.
Applied to files:
tests/model_registry/async_job/test_basic_oci_registry.pytests/conftest.py
📚 Learning: in tests/model_registry/rbac/test_mr_rbac_sa.py, bounds checking for model_registry_instance_rest_en...
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/async_job/test_basic_oci_registry.pytests/conftest.py
📚 Learning: in tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated m...
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:0-0
Timestamp: 2025-07-17T15:42:26.275Z
Learning: In tests/model_registry/conftest.py, the model_registry_instance_1 fixture (and similar duplicated Model Registry instance fixtures) do not require admin_client, db_deployment_1, or db_secret_1 parameters as explicit dependencies, even though these dependencies exist implicitly through the fixture dependency chain.
Applied to files:
tests/model_registry/async_job/test_basic_oci_registry.pytests/conftest.py
📚 Learning: in tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fi...
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:595-612
Timestamp: 2025-07-17T15:42:04.167Z
Learning: In tests/model_registry/conftest.py, the db_deployment_1 fixture (and similar duplicated resource fixtures) do not require the admin_client parameter or explicit dependencies on related fixtures like db_secret_1, db_pvc_1, and db_service_1, even though the original model_registry_db_deployment fixture includes these parameters.
Applied to files:
tests/conftest.py
📚 Learning: for trustyai image validation tests: operator image tests require admin_client, related_images_refs,...
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/conftest.py
📚 Learning: in tests/model_registry/conftest.py, the db_service_1 and db_service_2 fixtures do not require the a...
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/conftest.py
📚 Learning: in tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the adm...
Learnt from: lugi0
PR: opendatahub-io/opendatahub-tests#446
File: tests/model_registry/conftest.py:666-676
Timestamp: 2025-07-17T15:41:54.284Z
Learning: In tests/model_registry/conftest.py, the db_secret_1 and db_secret_2 fixtures do not require the admin_client parameter in their signatures, unlike some other Secret fixtures in the codebase. The user lugi0 confirmed this is the correct pattern for these specific fixtures.
Applied to files:
tests/conftest.py
📚 Learning: in tests/model_registry/rbac/conftest.py, predictable names are intentionally used for test resource...
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/conftest.py
📚 Learning: in tests/model_registry/conftest.py, service resources can be created without explicitly passing the...
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/conftest.py
🔇 Additional comments (12)
utilities/constants.py (1)
249-266: LGTM! Well-structured constants for OCI registry configuration.The new
OCIRegistryclass follows established patterns in the codebase and provides appropriate constants for registry setup. The configuration includes proper Istio integration with sidecar injection and route exposure.tests/model_registry/async_job/test_basic_oci_registry.py (1)
17-26: LGTM! Proper parametrization following established patterns.The parametrization correctly configures both MinIO and OCI registry pods using the constants from
utilities/constants.py.tests/model_registry/async_job/utils.py (4)
10-27: LGTM! Proper implementation of OCI registry blob upload.The function correctly implements the two-step OCI registry blob upload process: initiate upload (POST) followed by content upload (PUT). The SHA256 digest calculation and HTTP status code validation are appropriate.
29-39: LGTM! Correct OCI manifest structure.The manifest follows OCI Image Manifest Specification with proper schema version, media types, and structure. The JSON encoding with compact separators is appropriate for network transmission.
41-51: LGTM! Proper OCI manifest push implementation.The function correctly uses the OCI registry manifest API with appropriate Content-Type header and status code validation.
53-63: LGTM! Correct OCI manifest pull implementation.The function properly uses the Accept header to request OCI manifest format and validates the response before returning parsed JSON.
tests/conftest.py (6)
22-22: LGTM! Required import for Route fixture.
53-53: LGTM! Required import for OCIRegistry constants.
571-578: LGTM! Proper namespace fixture following established patterns.The fixture follows the same pattern as
minio_namespacewith unique naming and proper cleanup.
580-659: LGTM! Comprehensive OCI registry pod configuration.The fixture properly configures the OCI registry to use MinIO as S3 backend with all necessary environment variables. The security context and pod readiness wait are appropriate. The parameterization allows for flexible configuration while maintaining good defaults.
661-681: LGTM! Standard service configuration following established patterns.The service configuration is consistent with other services in the file, using appropriate port, protocol, and session affinity settings.
683-692: LGTM! Simple and correct route configuration.The route fixture properly exposes the OCI registry service externally, following the established pattern in the codebase.
lugi0
reviewed
Aug 5, 2025
lugi0
previously approved these changes
Aug 5, 2025
rhods-ci-bot
added
commented-by-fege
and removed
commented-by-mwaykole
commented-by-lugi0
lgtm-by-lugi0
labels
lugi0
previously approved these changes
Aug 5, 2025
dbasunag
reviewed
Aug 5, 2025
lugi0
approved these changes
Aug 5, 2025
dbasunag
reviewed
Aug 5, 2025
adolfo-ab
approved these changes
Aug 6, 2025
rhods-ci-bot
added
commented-by-fege
and removed
lgtm-by-lugi0
commented-by-dbasunag
labels
|
Status of building tag latest: success. |
This was referenced Aug 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
How Has This Been Tested?
Merge criteria: