fix: teardown issue where unprivileged user cannot delete namespace#443
Merged
mwaykole merged 8 commits intoopendatahub-io:mainfrom Jul 17, 2025
Merged
fix: teardown issue where unprivileged user cannot delete namespace#443mwaykole merged 8 commits intoopendatahub-io:mainfrom
mwaykole merged 8 commits intoopendatahub-io:mainfrom
Conversation
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
Contributor
📝 WalkthroughSummary by CodeRabbit
## Summary by CodeRabbit
* **Refactor**
* Standardized the use of admin and unprivileged clients in test fixtures and utilities.
* Updated several test fixtures to explicitly require both admin and unprivileged clients where applicable.
* Improved clarity and consistency in namespace and project creation logic during test setup.
## Walkthrough
The changes standardize the usage of the `admin_client` keyword argument across multiple test fixtures and the `create_ns` utility. Function signatures and calls are updated to explicitly require and pass `admin_client` instead of the ambiguous `client`, clarifying the distinction between admin and unprivileged clients in namespace and project creation logic.
## Changes
| Files/Paths | Change Summary |
|--------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------|
| tests/conftest.py, tests/model_explainability/trustyai_service/service/conftest.py, | Updated calls to `create_ns` to use `admin_client` instead of `client`; fixture signatures updated where required. |
| tests/model_registry/negative_tests/conftest.py, tests/model_serving/model_server/upgrade/conftest.py | Changed `client` to `admin_client` in `create_ns` calls within fixtures. |
| tests/model_serving/model_server/private_endpoint/conftest.py, tests/rag/conftest.py | Fixture signatures updated to accept `admin_client`; `create_ns` calls updated to use both clients. |
| utilities/infra.py | Refactored `create_ns` to require `admin_client`, clarified client usage, and updated teardown/cleanup logic. |
## Possibly related PRs
- opendatahub-io/opendatahub-tests#346: Modifies pytest fixtures to replace or add the `admin_client` parameter in place of a generic `client` parameter when calling namespace or resource creation utilities, closely related to client usage updates in test fixtures.
## Suggested labels
`Verified`
## Suggested reviewers
- dbasunag
- sheltoncyril📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
ModelExplainability
ModelRegistry
ModelServing
Utilities
size/m
labels
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/build-push-pr-image', '/cherry-pick', '/verified', '/hold', '/lgtm'} |
Raghul-M
previously approved these changes
Jul 16, 2025
rnetser
previously requested changes
Jul 16, 2025
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
![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)
utilities/infra.py (1)
140-166: Control flow restructuring addresses the teardown issue effectively.The restructured logic correctly separates namespace creation and cleanup responsibilities:
- When no
unprivileged_client: usesadmin_clientfor all operations- When
unprivileged_clientprovided: uses it for creation butadmin_clientfor cleanupThis addresses the PR objective by ensuring privileged operations (teardown) always use admin privileges.
Two minor issues to address:
- Line 163: Fix the typo in comment:
- # cleanup must be done with admin admin_client + # cleanup must be done with admin_client
- Line 155: Consider using the passed
admin_clientfor consistency:- ns = Namespace(client=get_client(), name=name) + ns = Namespace(client=admin_client, name=name)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/infra.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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#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: 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.
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.
utilities/infra.py (2)
Learnt from: jiripetrlik
PR: opendatahub-io/opendatahub-tests#335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The `wait_for_replicas()` method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
Learnt from: jiripetrlik
PR: opendatahub-io/opendatahub-tests#335
File: tests/rag/test_rag.py:0-0
Timestamp: 2025-06-19T14:32:17.658Z
Learning: The `wait_for_replicas()` method from ocp_resources.deployment.Deployment raises an exception when timeout is reached rather than returning a boolean value, so it doesn't need explicit return value checking in tests.
🔇 Additional comments (2)
utilities/infra.py (2)
97-97: Documentation updated correctly.The docstring parameter description has been updated to reflect the new parameter name.
74-74: Allcreate_nscallers updated to useadmin_client– no further action required.A search for
create_ns(across the codebase shows onlyadmin_client=(andunprivileged_client=) usages, with no remainingclient=parameters. The breaking-change update is fully applied.
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
rnetser
reviewed
Jul 16, 2025
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
Contributor
|
/lgtm |
Raghul-M
approved these changes
Jul 16, 2025
israel-hdez
dismissed
rnetser’s stale review
Dismissing rnester review to unblock merging, because she already approved.
|
Status of building tag latest: success. |
This was referenced Jul 23, 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
5 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.
No description provided.