[MR updates] use client arg for resource calls#983
[MR updates] use client arg for resource calls#983dbasunag merged 1 commit intoopendatahub-io:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR updates test fixture and utility functions to consistently pass an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![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/conftest.py (1)
570-570: Remove unusedoriginal_userparameter.The
original_userparameter is declared in the function signature but never used in the function body. Consider removing it to clean up the API.🔎 Proposed fix
@pytest.fixture(scope="module") def created_htpasswd_secret( - is_byoidc: bool, admin_client: DynamicClient, original_user: str, user_credentials_rbac: dict[str, str] + is_byoidc: bool, admin_client: DynamicClient, user_credentials_rbac: dict[str, str] ) -> Generator[UserTestSession | None, None, None]:Based on static analysis hints.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/conftest.pytests/model_registry/scc/conftest.pytests/model_registry/scc/utils.pytests/model_registry/upgrade/model_registry/test_model_registry_upgrade.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_registry/scc/conftest.py (2)
tests/conftest.py (1)
admin_client(79-80)tests/model_registry/conftest.py (1)
model_registry_namespace(76-77)
tests/model_registry/scc/utils.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/conftest.py (2)
utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)tests/conftest.py (2)
admin_client(79-80)original_user(819-822)
🪛 Ruff (0.14.10)
tests/model_registry/conftest.py
570-570: Unused function argument: original_user
(ARG001)
🔇 Additional comments (9)
tests/model_registry/upgrade/model_registry/test_model_registry_upgrade.py (1)
112-112: LGTM! Client parameter correctly added to CustomResourceDefinition.The explicit client parameter aligns with the broader pattern of centralizing DynamicClient usage across resource constructors.
tests/model_registry/conftest.py (5)
91-93: LGTM! ModelRegistry correctly receives client parameter.The explicit client parameter follows the established pattern for post-upgrade resource instantiation.
135-161: LGTM! All post-upgrade resources correctly instantiated with client parameter.The consistent pattern of passing
client=admin_clientto Secret, PersistentVolumeClaim, Service, ConfigMap, and Deployment constructors aligns with the PR's objective.
251-253: LGTM! Namespace instantiation correctly updated with client parameter.Both Namespace instantiations (custom namespace creation and original namespace restoration) now properly receive the admin_client parameter.
Also applies to: 270-270
586-586: LGTM! Secret constructor correctly receives client parameter.The explicit client parameter ensures proper API context for secret creation in the openshift-config namespace.
607-607: LGTM! OAuth constructor correctly receives client parameter.The explicit client parameter aligns with the broader pattern of centralizing DynamicClient usage.
tests/model_registry/scc/utils.py (1)
93-93: LGTM! Deployment constructor correctly receives client parameter.The explicit client parameter ensures proper API context when verifying deployment existence.
tests/model_registry/scc/conftest.py (2)
35-36: LGTM! Fixture correctly updated to accept and use admin_client.The explicit client parameter ensures proper API context when retrieving namespace annotations for SCC validation.
44-52: LGTM! Fixture correctly updated to accept and use admin_client.The explicit client parameter ensures proper API context when instantiating the Deployment resource for testing.
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/hold', '/wip', '/lgtm', '/cherry-pick', '/build-push-pr-image', '/verified'} |
dbasunag
force-pushed
the
update_client_part1
branch
from
c7a177f to
24dd1fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_registry/conftest.py (1)
62-134: Inconsistent client usage in cleanup block.Lines 88 and 105-107 correctly use
client=admin_clientfor Namespace instantiation. However, the finally block at lines 122 and 124 instantiates Namespace objects without the client parameter, relying on default client behavior. This inconsistency could lead to issues if the default client differs fromadmin_client.🔧 Proposed fix to use consistent client parameter
finally: resource_editor.restore() - Namespace(name=py_config["model_registry_namespace"]).delete(wait=True) + Namespace(client=admin_client, name=py_config["model_registry_namespace"]).delete(wait=True) # create the original namespace object again, so that we can wait for it to be created first - original_namespace = Namespace(name=original_namespace_name, wait_for_resource=True) + original_namespace = Namespace(client=admin_client, name=original_namespace_name, wait_for_resource=True) original_namespace.wait_for_status(status=Namespace.Status.ACTIVE)
🤖 Fix all issues with AI agents
In @tests/model_registry/conftest.py:
- Around line 204-206: The function signature for created_htpasswd_secret
includes an unused parameter original_user; remove original_user from the
parameter list and update any call sites to stop passing that argument (or
adjust callers accordingly), ensuring the function created_htpasswd_secret now
accepts only is_byoidc: bool, admin_client: DynamicClient,
user_credentials_rbac: dict[str, str] and that type hints and any references to
original_user inside tests or fixtures are removed/updated.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/model_registry/conftest.pytests/model_registry/model_registry/upgrade/test_model_registry_upgrade.pytests/model_registry/scc/conftest.pytests/model_registry/scc/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_registry/scc/conftest.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/model_registry/model_registry/upgrade/test_model_registry_upgrade.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/scc/utils.py (1)
tests/conftest.py (1)
admin_client(79-80)
tests/model_registry/conftest.py (2)
tests/conftest.py (3)
admin_client(79-80)is_byoidc(393-397)original_user(819-822)utilities/resources/model_registry_modelregistry_opendatahub_io.py (1)
ModelRegistry(9-94)
🪛 Ruff (0.14.10)
tests/model_registry/conftest.py
205-205: Unused function argument: original_user
(ARG001)
🔇 Additional comments (6)
tests/model_registry/model_registry/upgrade/test_model_registry_upgrade.py (1)
112-112: LGTM!The addition of
client=admin_clientto the CustomResourceDefinition instantiation is consistent with the PR objective and correctly uses the provided admin_client parameter.tests/model_registry/scc/utils.py (1)
93-93: LGTM!The addition of
client=admin_clientto the Deployment instantiation is consistent with the PR objective and correctly uses the provided admin_client parameter.tests/model_registry/conftest.py (4)
220-228: LGTM!The addition of
client=admin_clientto the Secret instantiation is consistent with the PR objective.
242-242: LGTM!The addition of
client=admin_clientto the OAuth instantiation is consistent with the PR objective.
297-299: LGTM!The addition of
client=admin_clientto the ModelRegistry instantiation in the post-upgrade path is consistent with the PR objective.
340-368: LGTM!The addition of
client=admin_clientto all resource instantiations (Secret, PersistentVolumeClaim, Service, ConfigMap, Deployment) in the post-upgrade path is consistent with the PR objective and correctly structured.
|
Status of building tag latest: success. |
4 participants
Description
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.