fix: bump wrapper library to get support for volumes in Job class#590
Conversation
Signed-off-by: lugi0 <lgiorgi@redhat.com>
📝 WalkthroughWalkthroughUpdated dependency constraint for openshift-python-wrapper in pyproject.toml. In tests, removed a custom Job wrapper class, switched fixtures to use the base Job with volumes passed directly, and deleted override logic that injected volumes post-serialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/wip', '/build-push-pr-image', '/cherry-pick', '/hold', '/lgtm'} |
![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
🧹 Nitpick comments (1)
tests/model_registry/async_job/conftest.py (1)
203-225: Optional: set service account on the pod template to align with how creds are used.Since the job relies on a specific ServiceAccount (you already pass sa_token), explicitly set the pod’s serviceAccountName to avoid surprises with default SA.
If supported by the wrapper in Job(...), pass the appropriate kwarg (e.g., service_account or service_account_name) with service_account.name.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.toml(1 hunks)tests/model_registry/async_job/conftest.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/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
🔇 Additional comments (5)
pyproject.toml (1)
65-65: Verify PyPI release, Python 3.13 support & Job.volumes
Manually confirm that openshift-python-wrapper 11.0.92 is published with artifacts on PyPI, itsrequires_python>=3.10covers Python 3.13, and thatocp_resources.job.Job.__init__exposes avolumesparameter (PR #2509).tests/model_registry/async_job/conftest.py (4)
203-225: LGTM: Using native Job(volumes=...) removes brittle post-serialization patching.This is the right direction and aligns the fixture with upstream capabilities.
177-201: Ensure env var values are strings.Kubernetes env.value must be a string. Verify MODEL_SYNC_DESTINATION_OCI_ENABLE_TLS_VERIFY in MODEL_SYNC_CONFIG is a string (e.g., "true"/"false"), not a boolean.
You can assert at runtime before creating the Job:
- environment_variables = [ + environment_variables = [ + # defensive check + # assert isinstance(MODEL_SYNC_CONFIG["DESTINATION_OCI_ENABLE_TLS_VERIFY"], str)
203-225: Sanity-check volumes land in the created Job spec.To catch regressions in wrapper behavior, verify the volumes appear in spec.template.spec.volumes before waiting on completion.
Example one-liner after entering the context:
) as job: + # Verify volumes wired correctly (helps validate wrapper change) + assert any(v.get("name") == "source-credentials" for v in job.instance.spec.template.spec.volumes) + assert any(v.get("name") == "destination-credentials" for v in job.instance.spec.template.spec.volumes) job.wait_for_condition(condition="Complete", status="True")
1-380: No leftover references to JobWithVolumes found.
|
Tested locally with previous vars and it works correctly, however with the current secret setup we need to wait for kubeflow/model-registry#1499 to be synced into downstream otherwise this test would fail |
|
Status of building tag latest: success. |
…endatahub-io#590) Signed-off-by: lugi0 <lgiorgi@redhat.com>
5 participants
Bump the wrapper library to min. version 11.0.92 to onboard RedHatQE/openshift-python-wrapper#2509 and get support for volumes in the Job class
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Chores
Tests