fix: minor bug fix in clean up and wait for TAS operator#1192
Merged
dbasunag merged 3 commits intoopendatahub-io:mainfrom Mar 10, 2026
Merged
fix: minor bug fix in clean up and wait for TAS operator#1192dbasunag merged 3 commits intoopendatahub-io:mainfrom
dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/build-push-pr-image', '/verified', '/wip', '/cherry-pick', '/lgtm'} |
Contributor
📝 WalkthroughWalkthroughSwitched Securesign readiness checks in test fixtures from custom polling/TimeoutSampler to condition-based waits; removed the Changes
Security findings (actionable)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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: 2
🧹 Nitpick comments (1)
tests/model_registry/model_registry/python_client/signing/conftest.py (1)
203-203: Pass the readiness timeout explicitly.The docstring still promises a 5-minute wait budget, but Line 203 now depends on
wait_for_condition(...)defaults. Keep the timeout explicit here so hangs and timeout regressions do not depend on library behavior.Suggested fix
- securesign.wait_for_condition(condition="Ready", status="True") + securesign.wait_for_condition( + condition="Ready", + status="True", + timeout=Timeout.TIMEOUT_5MIN, + )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` at line 203, The call to securesign.wait_for_condition currently relies on its default timeout, contradicting the docstring promise of a 5-minute readiness budget; update the call to securesign.wait_for_condition(condition="Ready", status="True", timeout=300) (or equivalent 5-minute param name if different) so the timeout is passed explicitly and no longer depends on library defaults; modify the invocation in conftest.py where securesign.wait_for_condition is called to include the explicit 300-second timeout.
🤖 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 154-155: The Namespace for SECURESIGN_NAMESPACE is only
referenced, not created, causing wait_for_status(Namespace.Status.ACTIVE) to
time out; construct it with ensure_exists=True so the namespace is created
before waiting (i.e., update Namespace(name=SECURESIGN_NAMESPACE) to
Namespace(name=SECURESIGN_NAMESPACE, ensure_exists=True) and keep the subsequent
wait_for_status call).
- Around line 120-123: The Namespace deletion is currently asynchronous and
namespace creation was removed; update the deletion call to ns.delete(wait=True)
to avoid a Terminating race, and restore creation by constructing the Namespace
with the admin client and ensure_exists=True (or use the context manager
pattern) so the namespace is created before waiting: e.g., use
Namespace(client=admin_client, name=SECURESIGN_NAMESPACE, ensure_exists=True) or
with Namespace(client=admin_client, name=SECURESIGN_NAMESPACE) as ns, then call
ns.wait_for_status(status=Namespace.Status.ACTIVE); ensure you reference
Namespace, SECURESIGN_NAMESPACE, ns.delete(wait=True), and
ns.wait_for_status(...) in the fix.
---
Nitpick comments:
In `@tests/model_registry/model_registry/python_client/signing/conftest.py`:
- Line 203: The call to securesign.wait_for_condition currently relies on its
default timeout, contradicting the docstring promise of a 5-minute readiness
budget; update the call to securesign.wait_for_condition(condition="Ready",
status="True", timeout=300) (or equivalent 5-minute param name if different) so
the timeout is passed explicitly and no longer depends on library defaults;
modify the invocation in conftest.py where securesign.wait_for_condition is
called to include the explicit 300-second timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c9bc60c0-6bee-412a-8a04-1dffbe0a3736
📒 Files selected for processing (2)
tests/model_registry/model_registry/python_client/signing/conftest.pytests/model_registry/model_registry/python_client/signing/utils.py
💤 Files with no reviewable changes (1)
- tests/model_registry/model_registry/python_client/signing/utils.py
dbasunag
changed the title
dbasunag
commented
Mar 10, 2026
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/conftest.py`:
- Around line 120-123: The comment above the Namespace teardown is incorrect: it
reads "Ensure namespace exists for Securesign" but the code is deleting the
namespace; update the comment to accurately describe the teardown (e.g., "Delete
Securesign namespace if it exists") or remove it; locate the Namespace
instantiation (Namespace(name=SECURESIGN_NAMESPACE)) and the conditional using
ns.exists and ns.delete(wait=True) and change the comment to reflect deletion
rather than creation.
- Around line 154-155: The wait_for_status call on Namespace lacks an explicit
timeout which can cause indefinite hangs; update the Namespace instantiation
usage by calling ns.wait_for_status(status=Namespace.Status.ACTIVE,
timeout=Timeout.TIMEOUT_2MIN) so it matches the pattern used in
utilities/infra.py and prevents waiting forever (refer to Namespace,
wait_for_status, Namespace.Status.ACTIVE, and Timeout.TIMEOUT_2MIN to locate the
change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 73194bc5-0e17-44ea-a497-45609059d19c
📒 Files selected for processing (1)
tests/model_registry/model_registry/python_client/signing/conftest.py
mwaykole
approved these changes
Mar 10, 2026
|
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
How it has been tested
Additional Requirements
Summary by CodeRabbit