test: Add async job signing test automation#1312
Merged
dbasunag merged 7 commits intoopendatahub-io:mainfrom Mar 27, 2026
Merged
test: Add async job signing test automation#1312dbasunag merged 7 commits intoopendatahub-io:mainfrom
dbasunag merged 7 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/build-push-pr-image', '/hold', '/wip', '/cherry-pick', '/lgtm', '/verified'} |
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFixture for reading async upload container images relocated from async job test conftest to parent conftest. Signing infrastructure added including fixtures for secrets, model registration, image verification, native signing job orchestration, and utilities for MinIO uploads and OCI image polling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Security Issues Requiring Attention
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Around line 765-788: Tests currently reuse the same MinIO prefix
(MODEL_SYNC_CONFIG["SOURCE_AWS_KEY"]) causing stale model.sig artifacts to
persist between scenarios; update the upload fixtures (e.g.,
upload_signed_model_to_minio) to isolate and clean the prefix before upload by
deriving a unique source_key per scenario (for example append
model_registry_namespace or a short UUID/timestamp to
MODEL_SYNC_CONFIG["SOURCE_AWS_KEY"]) and modify the run_minio_uploader_pod
mc_commands to first remove any existing objects under that prefix (e.g., run
"mc rm --recursive --force testminio/{bucket}/{source_key}/" or equivalent) then
perform the mc cp uploads and ls; ensure the same unique source_key change is
applied to the other uploader fixtures referenced (around the 855-875 region) so
each test writes to and cleans its own object prefix.
- Around line 694-708: The job fixtures that accept signing_registered_model
must use that fixture's actual IDs instead of the static MODEL_SYNC_* constants;
update the code that constructs the job environment (the fixtures creating the
async job envs around lines 792-829 and 906-970) to set MODEL_SYNC_MODEL_ID =
signing_registered_model.id, MODEL_SYNC_MODEL_VERSION_ID =
signing_registered_model.current_version.id (or
signing_registered_model.versions[0].id if current_version isn't present), and
MODEL_SYNC_MODEL_ARTIFACT_ID = the registered version's artifact id (e.g.,
signing_registered_model.current_version.artifact_id or
signing_registered_model.versions[0].artifact_id); replace all usages of
MODEL_SYNC_MODEL_ID, MODEL_SYNC_MODEL_VERSION_ID, and
MODEL_SYNC_MODEL_ARTIFACT_ID in those job fixtures so the jobs target the
model/version/artifact just registered by signing_registered_model.
In
`@tests/model_registry/model_registry/python_client/signing/test_async_signing.py`:
- Around line 50-92: Several test functions are missing explicit return type
annotations and the signing_job_pod fixture parameter is untyped, breaking mypy;
update each test function (test_model_signed_before_upload,
test_async_job_uploads_signed_model, test_job_log_signing_disabled,
test_sign_oci_image, test_verify_oci_image) to declare a return type -> None and
add a precise type for signing_job_pod (use the same Pod type or fixture type
used elsewhere in tests, e.g., Pod or kubernetes.client.models.V1Pod; import
that type if needed) and ensure other fixtures (Signer, oci_image_with_digest)
keep their existing types; run mypy to confirm the file is clean.
In
`@tests/model_registry/model_registry/python_client/signing/test_native_job_signing.py`:
- Around line 52-97: The three tests test_native_signing_job_completes,
test_job_log_contains_signed_image, and test_job_log_contains_verified_image are
missing explicit parameter and return type annotations; update their signatures
to fully typed forms (e.g., def test_native_signing_job_completes(self,
native_signing_job_pod: <PodType>) -> None:) using the same concrete pod/fixture
type used elsewhere in the test suite (or typing.Any if a concrete type import
is not available), and add the -> None return annotation for each test; also add
any necessary import for the chosen type so the file is mypy-clean.
In `@tests/model_registry/model_registry/python_client/signing/utils.py`:
- Around line 227-251: get_base_async_job_env_vars currently injects the
registry bearer token as a plain "value" under MODEL_SYNC_REGISTRY_USER_TOKEN;
change this to reference a Kubernetes Secret via valueFrom.secretKeyRef and
widen the function return type to allow valueFrom entries. Specifically, update
get_base_async_job_env_vars signature to accept a secret name (e.g.,
sa_token_secret_name) or accept both sa_token and sa_token_secret_name, change
the return type from list[dict[str, str]] to a type that permits mixed
value/valueFrom dicts (e.g., list[dict[str, Any]]), and replace the
{"name":"MODEL_SYNC_REGISTRY_USER_TOKEN","value": sa_token} entry with
{"name":"MODEL_SYNC_REGISTRY_USER_TOKEN","valueFrom":{"secretKeyRef":{"name":
sa_token_secret_name,"key":"token"}}}; mirror the pattern used in
tests/fixtures/files.py and tests/fixtures/vector_io.py so callers/tests are
updated to supply the secret reference instead of a literal token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f880b922-bbfb-4974-9c7c-6d8c6a772c82
📒 Files selected for processing (7)
tests/model_registry/model_registry/async_job/conftest.pytests/model_registry/model_registry/conftest.pytests/model_registry/model_registry/python_client/signing/conftest.pytests/model_registry/model_registry/python_client/signing/constants.pytests/model_registry/model_registry/python_client/signing/test_async_signing.pytests/model_registry/model_registry/python_client/signing/test_native_job_signing.pytests/model_registry/model_registry/python_client/signing/utils.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_registry/async_job/conftest.py
tests/model_registry/model_registry/python_client/signing/conftest.py
Outdated
Show resolved
Hide resolved
tests/model_registry/model_registry/python_client/signing/test_signing_async_job.py
Outdated
Show resolved
Hide resolved
tests/model_registry/model_registry/python_client/signing/test_native_job_signing.py
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/model_registry/model_registry/python_client/signing/utils.py (1)
227-251:⚠️ Potential issue | 🟠 MajorDo not inject bearer token as literal env var (CWE-522, CWE-312).
MODEL_SYNC_REGISTRY_USER_TOKENat line 242 is serialized into the Job/Pod spec in cleartext. Any principal withget podspermission can exfiltrate it. Store in aSecretand reference viavalueFrom.secretKeyRef.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/utils.py` around lines 227 - 251, The function get_base_async_job_env_vars currently injects the service account bearer token as a literal env var MODEL_SYNC_REGISTRY_USER_TOKEN; instead create a Kubernetes Secret (or accept an existing secret name) and replace the plain env var with an env var entry that uses valueFrom.secretKeyRef to reference the secret key (i.e., secretName and key) instead of serializing sa_token directly; update callers of get_base_async_job_env_vars to pass/derive the secret name/key (or change the signature to accept secretName and secretKey) so the Job/Pod spec stores the token only as a Secret reference.tests/model_registry/model_registry/python_client/signing/test_async_signing.py (1)
50-92: 🛠️ Refactor suggestion | 🟠 MajorAdd type annotations to test methods and fixture parameters.
Test methods lack
-> Nonereturn type, andsigning_job_podparameter is untyped. This breaks mypy strict mode.Proposed fix
+from ocp_resources.pod import Pod + `@pytest.mark.dependency`(name="test_model_signed_before_upload") - def test_model_signed_before_upload(self, signed_model_dir: Path): + def test_model_signed_before_upload(self, signed_model_dir: Path) -> None: """Verify model was signed locally before upload to MinIO.""" assert check_model_signature_file(model_dir=str(signed_model_dir)) LOGGER.info(f"Model signed successfully at {signed_model_dir}") `@pytest.mark.dependency`( name="test_async_job_uploads_signed_model", depends=["test_model_signed_before_upload"], ) def test_async_job_uploads_signed_model( self, - signing_job_pod, + signing_job_pod: Pod, - ): + ) -> None: """Verify async job completes and uploads signed model to OCI registry.""" assert signing_job_pod.name.startswith(f"{ASYNC_UPLOAD_JOB_NAME}-signing") LOGGER.info(f"Async upload job completed, pod: {signing_job_pod.name}") `@pytest.mark.dependency`(depends=["test_async_job_uploads_signed_model"]) def test_job_log_signing_disabled( self, - signing_job_pod, + signing_job_pod: Pod, - ): + ) -> None: """Verify the job pod log indicates signing is disabled (external signing flow).""" expected_message = "Signing is disabled" assert expected_message in signing_job_pod.log(), f"Expected '{expected_message}' not found in job pod log" `@pytest.mark.dependency`( name="test_sign_oci_image", depends=["test_async_job_uploads_signed_model"], ) - def test_sign_oci_image(self, signer: Signer, oci_image_with_digest: str): + def test_sign_oci_image(self, signer: Signer, oci_image_with_digest: str) -> None: """Sign the OCI image that was created by the async upload job.""" LOGGER.info(f"Signing OCI image: {oci_image_with_digest}") signer.sign_image(image=oci_image_with_digest) LOGGER.info("OCI image signed successfully") `@pytest.mark.dependency`(depends=["test_sign_oci_image"]) - def test_verify_oci_image(self, signer: Signer, oci_image_with_digest: str): + def test_verify_oci_image(self, signer: Signer, oci_image_with_digest: str) -> None: """Verify the signed OCI image.""" LOGGER.info(f"Verifying OCI image: {oci_image_with_digest}") signer.verify_image(image=oci_image_with_digest) LOGGER.info("OCI image verified successfully")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/test_async_signing.py` around lines 50 - 92, Add explicit return type annotations "-> None" to the test functions test_model_signed_before_upload, test_async_job_uploads_signed_model, test_job_log_signing_disabled, test_sign_oci_image, and test_verify_oci_image, and add a type annotation for the signing_job_pod parameter (e.g., signing_job_pod: Any or a concrete pod type like V1Pod) so mypy strict mode passes; import Any from typing if you use it and keep the existing Signer annotation for test_sign_oci_image/test_verify_oci_image unchanged.
🧹 Nitpick comments (2)
tests/model_registry/model_registry/python_client/signing/utils.py (1)
340-354:verify=Falsedisables TLS certificate validation (CWE-295).Acceptable for internal test registries with self-signed certs, but document the rationale. If these utilities are ever repurposed outside test scaffolding, this becomes a security hole.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/utils.py` around lines 340 - 354, The requests.get calls that build tags_url and manifest_url pass verify=False which disables TLS validation; update the utility to make TLS verification explicit and configurable: revert to verify=True by default and add a parameter or environment toggle (e.g., allow_insecure or TEST_REGISTRY_INSECURE) used by the calls to requests.get (both the tags_url and manifest_url requests) so tests can opt into insecure mode only when needed, and add a clear comment in the function describing that verify may be disabled only for internal test registries with self-signed certs and why.tests/model_registry/model_registry/python_client/signing/constants.py (1)
34-39:model_nametimestamp is fixed at module import time.
int(time.time())executes once when the module loads, not per-test. If tests run over extended periods or if module caching causes reuse across sessions, you may get collisions. If this is intentional for test correlation, it's fine. If unique names per test run are needed, consider generating inside a fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/constants.py` around lines 34 - 39, SIGNING_MODEL_DATA currently fixes model_name at module import by using int(time.time()) in the module-level dict; change this so each test run gets a fresh name by moving the timestamp/unique-id generation out of the module-level SIGNING_MODEL_DATA and into a factory or fixture that builds the dict per-test (e.g., provide a function or pytest fixture that returns a copy of SIGNING_MODEL_DATA with model_name set to f"signing-model-{int(time.time())}" or a UUID); update any callers to use that factory/fixture rather than the module constant to avoid name collisions across test runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_registry/model_registry/python_client/signing/utils.py`:
- Around line 361-363: The generator that sets
os.environ["COSIGN_ALLOW_INSECURE_REGISTRY"] = "true" and yields image_ref must
guarantee cleanup even if an exception occurs during the yield; wrap the env var
set and yield in a try/finally so that
os.environ.pop("COSIGN_ALLOW_INSECURE_REGISTRY", None) is executed in the
finally block, ensuring the environment variable is removed regardless of test
failures or exceptions while the generator is active.
- Line 319: The call job.wait_for_condition(condition="Complete", status="True")
can block indefinitely; modify the invocation in
tests/model_registry/model_registry/python_client/signing/utils.py to pass an
explicit timeout (e.g., timeout=<seconds> or a configurable constant) to
wait_for_condition so the test fails fast instead of hanging; update any related
helpers or constants (e.g., add DEFAULT_WAIT_TIMEOUT) if needed and ensure tests
that expect longer runs override the timeout where appropriate.
---
Duplicate comments:
In
`@tests/model_registry/model_registry/python_client/signing/test_async_signing.py`:
- Around line 50-92: Add explicit return type annotations "-> None" to the test
functions test_model_signed_before_upload, test_async_job_uploads_signed_model,
test_job_log_signing_disabled, test_sign_oci_image, and test_verify_oci_image,
and add a type annotation for the signing_job_pod parameter (e.g.,
signing_job_pod: Any or a concrete pod type like V1Pod) so mypy strict mode
passes; import Any from typing if you use it and keep the existing Signer
annotation for test_sign_oci_image/test_verify_oci_image unchanged.
In `@tests/model_registry/model_registry/python_client/signing/utils.py`:
- Around line 227-251: The function get_base_async_job_env_vars currently
injects the service account bearer token as a literal env var
MODEL_SYNC_REGISTRY_USER_TOKEN; instead create a Kubernetes Secret (or accept an
existing secret name) and replace the plain env var with an env var entry that
uses valueFrom.secretKeyRef to reference the secret key (i.e., secretName and
key) instead of serializing sa_token directly; update callers of
get_base_async_job_env_vars to pass/derive the secret name/key (or change the
signature to accept secretName and secretKey) so the Job/Pod spec stores the
token only as a Secret reference.
---
Nitpick comments:
In `@tests/model_registry/model_registry/python_client/signing/constants.py`:
- Around line 34-39: SIGNING_MODEL_DATA currently fixes model_name at module
import by using int(time.time()) in the module-level dict; change this so each
test run gets a fresh name by moving the timestamp/unique-id generation out of
the module-level SIGNING_MODEL_DATA and into a factory or fixture that builds
the dict per-test (e.g., provide a function or pytest fixture that returns a
copy of SIGNING_MODEL_DATA with model_name set to
f"signing-model-{int(time.time())}" or a UUID); update any callers to use that
factory/fixture rather than the module constant to avoid name collisions across
test runs.
In `@tests/model_registry/model_registry/python_client/signing/utils.py`:
- Around line 340-354: The requests.get calls that build tags_url and
manifest_url pass verify=False which disables TLS validation; update the utility
to make TLS verification explicit and configurable: revert to verify=True by
default and add a parameter or environment toggle (e.g., allow_insecure or
TEST_REGISTRY_INSECURE) used by the calls to requests.get (both the tags_url and
manifest_url requests) so tests can opt into insecure mode only when needed, and
add a clear comment in the function describing that verify may be disabled only
for internal test registries with self-signed certs and why.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 49193a8f-dba6-4d5f-a893-3ae05100cbe8
📒 Files selected for processing (7)
tests/model_registry/model_registry/async_job/conftest.pytests/model_registry/model_registry/conftest.pytests/model_registry/model_registry/python_client/signing/conftest.pytests/model_registry/model_registry/python_client/signing/constants.pytests/model_registry/model_registry/python_client/signing/test_async_signing.pytests/model_registry/model_registry/python_client/signing/test_native_job_signing.pytests/model_registry/model_registry/python_client/signing/utils.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_registry/async_job/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_registry/model_registry/conftest.py
- tests/model_registry/model_registry/python_client/signing/test_native_job_signing.py
- tests/model_registry/model_registry/python_client/signing/conftest.py
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/model_registry/model_registry/python_client/signing/test_signing_async_job.py`:
- Around line 69-76: The test test_job_log_signing_disabled is too brittle by
asserting an exact phrase; instead assert semantic tokens from
signing_job_pod.log() (e.g., that it contains both "sign" and "disabled" or
match a case-insensitive regex like r"sign.*disabled") so small wording changes
won't break the test—update the assertion to check for those key tokens (or use
re.search with flags=re.I) against signing_job_pod.log() and keep the failure
message describing the required tokens.
- Around line 60-66: The test_async_job_uploads_signed_model currently only
asserts signing_job_pod.name.startswith(f"{ASYNC_UPLOAD_JOB_NAME}-signing")
which doesn't prove the job succeeded; update the test to assert the pod
completion status (e.g., verify signing_job_pod.status.phase == "Succeeded" or
check container termination state and exit_code == 0 in
signing_job_pod.status.container_statuses[*].state.terminated) and optionally
ensure any relevant conditions/messages indicate success before logging;
reference the test function test_async_job_uploads_signed_model, the
signing_job_pod fixture/object, and ASYNC_UPLOAD_JOB_NAME to locate and change
the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 94c01c72-aac6-494d-a102-eef067e32267
📒 Files selected for processing (2)
tests/model_registry/model_registry/python_client/signing/test_infrastructure_signing.pytests/model_registry/model_registry/python_client/signing/test_signing_async_job.py
tests/model_registry/model_registry/python_client/signing/test_signing_async_job.py
Outdated
Show resolved
Hide resolved
tests/model_registry/model_registry/python_client/signing/test_signing_async_job.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/model_registry/model_registry/python_client/signing/constants.py (1)
34-39:model_nametimestamp is evaluated once at import time.
int(time.time())executes when the module loads, not when fixtures consumeSIGNING_MODEL_DATA. If multiple test classes import this module within the same process (e.g.,pytest-xdistworker or sequential runs without process isolation), they share an identicalmodel_name, risking registry collisions.Consider generating the name lazily inside the fixture or using a UUID suffix.
Option: Generate unique name per fixture invocation
Move the dynamic name generation into the fixture that uses it, or wrap in a factory function:
def get_signing_model_data() -> dict[str, Any]: return { **MODEL_DICT, "model_name": f"signing-model-{int(time.time())}-{shortuuid.uuid().lower()}", "model_storage_key": MODEL_SYNC_CONFIG["SOURCE_AWS_KEY"], "model_storage_path": "path/to/test/model", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/constants.py` around lines 34 - 39, SIGNING_MODEL_DATA currently sets model_name using int(time.time()) at import time which causes identical names across tests; change to generate the name lazily (e.g., replace the module-level SIGNING_MODEL_DATA usage with a factory function like get_signing_model_data or move the model_name generation into the fixture that consumes SIGNING_MODEL_DATA) so each invocation creates a unique model_name (use time plus a UUID/shortuuid suffix) and update references to use the factory (or fixture) instead of the static SIGNING_MODEL_DATA.tests/model_registry/model_registry/python_client/signing/conftest.py (1)
878-902:identity_token_secretcreates token via file round-trip.
generate_tokenwrites to disk, then this fixture reads it back. Not a defect, but direct in-memory handling would avoid I/O. Acceptable for test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/conftest.py` around lines 878 - 902, The fixture identity_token_secret currently uses generate_token(temp_base_folder=...) which writes a file and is read back; change generate_token (or add a helper like generate_token_content) to return the token string directly (e.g., an optional flag return_content=True) and update identity_token_secret to call that API to get token_content in-memory, then use b64_encoded_string(token_content) without writing/reading disk; adjust tests that call generate_token if needed to preserve backward compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_registry/model_registry/python_client/signing/utils.py`:
- Around line 324-345: The loop in get_oci_image_with_digest uses TimeoutSampler
to wait for the registry but silently continues if TimeoutSampler exhausts,
causing downstream obscure errors; update the for sample in TimeoutSampler(...)
loop to add an else: block that raises a TimeoutError (or RuntimeError)
including registry_url and timeout duration when no sample.ok is observed, so
the function fails fast and provides a clear message about the registry
readiness failure.
- Around line 260-271: The function get_model_registry_host accesses
model_registry_instance[0] unguarded; add an explicit guard at the start of
get_model_registry_host to check for an empty model_registry_instance and raise
a clear exception (e.g., ValueError or a custom error) with a descriptive
message, or assert non-empty input; then continue to call
get_mr_service_by_label and get_endpoint_from_mr_service as before and return
the host part from the endpoint. Ensure the error message mentions
get_model_registry_host and model_registry_instance so callers can diagnose
missing instances.
---
Nitpick comments:
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Around line 878-902: The fixture identity_token_secret currently uses
generate_token(temp_base_folder=...) which writes a file and is read back;
change generate_token (or add a helper like generate_token_content) to return
the token string directly (e.g., an optional flag return_content=True) and
update identity_token_secret to call that API to get token_content in-memory,
then use b64_encoded_string(token_content) without writing/reading disk; adjust
tests that call generate_token if needed to preserve backward compatibility.
In `@tests/model_registry/model_registry/python_client/signing/constants.py`:
- Around line 34-39: SIGNING_MODEL_DATA currently sets model_name using
int(time.time()) at import time which causes identical names across tests;
change to generate the name lazily (e.g., replace the module-level
SIGNING_MODEL_DATA usage with a factory function like get_signing_model_data or
move the model_name generation into the fixture that consumes
SIGNING_MODEL_DATA) so each invocation creates a unique model_name (use time
plus a UUID/shortuuid suffix) and update references to use the factory (or
fixture) instead of the static SIGNING_MODEL_DATA.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3fca50a1-2e6f-4705-8c17-bd9b47bfef9a
📒 Files selected for processing (8)
tests/model_registry/model_registry/async_job/conftest.pytests/model_registry/model_registry/conftest.pytests/model_registry/model_registry/python_client/signing/conftest.pytests/model_registry/model_registry/python_client/signing/constants.pytests/model_registry/model_registry/python_client/signing/test_infrastructure_signing.pytests/model_registry/model_registry/python_client/signing/test_native_job_signing.pytests/model_registry/model_registry/python_client/signing/test_signing_async_job.pytests/model_registry/model_registry/python_client/signing/utils.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_registry/async_job/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/model_registry/model_registry/conftest.py
- tests/model_registry/model_registry/python_client/signing/test_native_job_signing.py
- tests/model_registry/model_registry/python_client/signing/test_signing_async_job.py
fege
reviewed
Mar 27, 2026
tests/model_registry/model_registry/python_client/signing/test_native_job_signing.py
Show resolved
Hide resolved
jonburdo
reviewed
Mar 27, 2026
Member
jonburdo
left a comment
jonburdo
left a comment
There was a problem hiding this comment.
Looks great! I think we can just whittle it down to pretty much one e2e test.
If (in the future) we want more testing on the async upload job, I think we'll want to spend our energy and testing resources on making sure we cover all three intent types.
tests/model_registry/model_registry/python_client/signing/test_native_job_signing.py
Show resolved
Hide resolved
tests/model_registry/model_registry/python_client/signing/test_signing_async_job.py
Outdated
Show resolved
Hide resolved
rhods-ci-bot
added
commented-by-dbasunag
and removed
commented-by-dbasunag
lgtm-by-fege
labels
Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/model_registry/model_registry/python_client/signing/conftest.py (2)
759-823:⚠️ Potential issue | 🟠 MajorUse
signing_registered_modelIDs when building async job env vars.Line 770 injects
signing_registered_model, but Line 809-Line 815 only usesget_base_async_job_env_vars(...)plus signing flags; no model/version/artifact ID overrides are added. This can point the job at wrong registry entities when defaults drift.Suggested fix
def native_signing_async_job( @@ ) -> Generator[Job, Any, Any]: @@ + model_version = getattr(signing_registered_model, "current_version", None) or signing_registered_model.versions[0] + model_artifact_id = getattr(model_version, "artifact_id", None) + assert model_artifact_id, "Registered model artifact_id is required for async signing job" + + registered_model_env_vars = [ + {"name": "MODEL_SYNC_MODEL_ID", "value": str(signing_registered_model.id)}, + {"name": "MODEL_SYNC_MODEL_VERSION_ID", "value": str(model_version.id)}, + {"name": "MODEL_SYNC_MODEL_ARTIFACT_ID", "value": str(model_artifact_id)}, + ] + yield from create_async_upload_job( @@ - environment_variables=get_base_async_job_env_vars( + environment_variables=get_base_async_job_env_vars( mr_host=mr_host, sa_token=sa_token, oci_internal=oci_internal, oci_repo=NATIVE_SIGNING_REPO, ) - + signing_env_vars, + + registered_model_env_vars + + signing_env_vars,#!/bin/bash set -euo pipefail echo "1) Verify helper signature and defaults:" rg -n --type=py 'def get_base_async_job_env_vars\(' tests/model_registry/model_registry/python_client/signing/utils.py -A60 echo echo "2) Verify model-id env vars usage in signing conftest and helper:" rg -n --type=py 'MODEL_SYNC_MODEL_ID|MODEL_SYNC_MODEL_VERSION_ID|MODEL_SYNC_MODEL_ARTIFACT_ID' \ tests/model_registry/model_registry/python_client/signing/conftest.py \ tests/model_registry/model_registry/python_client/signing/utils.py -C2 echo echo "3) Verify native fixture currently does not thread signing_registered_model into env:" rg -n --type=py 'def native_signing_async_job\(|signing_registered_model|environment_variables=' \ tests/model_registry/model_registry/python_client/signing/conftest.py -A120As per coding guidelines, "
**/*.py: ... proper test dependency should be also encouraged".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/conftest.py` around lines 759 - 823, The native_signing_async_job fixture builds signing_env_vars but never injects the signing_registered_model IDs, so the async job may target wrong registry entities; update native_signing_async_job to merge MODEL_SYNC_MODEL_ID, MODEL_SYNC_MODEL_VERSION_ID and MODEL_SYNC_MODEL_ARTIFACT_ID (using IDs from signing_registered_model) into the environment_variables passed to create_async_upload_job (either by extending the signing_env_vars or by calling get_base_async_job_env_vars with explicit model/version/artifact overrides) so the job uses the intended registered model identifiers.
707-728:⚠️ Potential issue | 🟠 MajorClear the MinIO prefix before uploading the “unsigned” artifact.
Line 714 reuses a shared source prefix and Line 723-Line 726 never removes old objects. A stale
model.sigcan survive and invalidate this scenario by making unsigned flow consume pre-signed artifacts.Suggested fix
def upload_unsigned_model_to_minio( @@ run_minio_uploader_pod( @@ mc_commands=( + f"mc rm --recursive --force testminio/{bucket}/{source_key}/ || true && " f"echo 'test model content for native signing' > /work/model.onnx && " f"mc cp /work/model.onnx testminio/{bucket}/{source_key}/model.onnx && " f"mc ls testminio/{bucket}/{source_key}/ && " f"echo 'Unsigned model upload completed'" ), )As per coding guidelines, "** REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_registry/model_registry/python_client/signing/conftest.py` around lines 707 - 728, The test fixture upload_unsigned_model_to_minio reuses a shared SOURCE_AWS_KEY prefix but never clears existing objects, so stale artifacts like model.sig can make the upload appear signed; modify the run_minio_uploader_pod mc_commands sequence (in the upload_unsigned_model_to_minio fixture) to first remove any existing objects under testminio/{bucket}/{source_key}/ (e.g., use mc rm --recursive or equivalent) before creating and copying the new model.onnx, ensuring the prefix is cleaned (including model.sig) prior to the upload step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Around line 731-756: The identity_token_secret fixture leaves the temporary
token file created by generate_token (token variable, temp_base_folder) on disk
causing credential exposure; after reading token_content (and before yielding
the Secret in the Secret context) remove the temporary file and ensure removal
on error by wrapping the read and Secret creation in a try/finally (or use a
context that guarantees cleanup) and call os.remove(token) or
Path(token).unlink() in the finally so the temporary identity token is always
deleted; reference the identity_token_secret fixture, generate_token call, token
variable, and the Secret context when applying the fix.
---
Duplicate comments:
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Around line 759-823: The native_signing_async_job fixture builds
signing_env_vars but never injects the signing_registered_model IDs, so the
async job may target wrong registry entities; update native_signing_async_job to
merge MODEL_SYNC_MODEL_ID, MODEL_SYNC_MODEL_VERSION_ID and
MODEL_SYNC_MODEL_ARTIFACT_ID (using IDs from signing_registered_model) into the
environment_variables passed to create_async_upload_job (either by extending the
signing_env_vars or by calling get_base_async_job_env_vars with explicit
model/version/artifact overrides) so the job uses the intended registered model
identifiers.
- Around line 707-728: The test fixture upload_unsigned_model_to_minio reuses a
shared SOURCE_AWS_KEY prefix but never clears existing objects, so stale
artifacts like model.sig can make the upload appear signed; modify the
run_minio_uploader_pod mc_commands sequence (in the
upload_unsigned_model_to_minio fixture) to first remove any existing objects
under testminio/{bucket}/{source_key}/ (e.g., use mc rm --recursive or
equivalent) before creating and copying the new model.onnx, ensuring the prefix
is cleaned (including model.sig) prior to the upload step.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 89a8b00b-9e47-4f14-b10a-890da4550134
📒 Files selected for processing (1)
tests/model_registry/model_registry/python_client/signing/conftest.py
|
Status of building tag latest: success. |
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
4 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.
Pull Request
Summary
Related Issues
Please review and indicate how it has been tested
Additional Requirements
Summary by CodeRabbit