Skip to content

Replace dyn_client with client in all get calls#980

Merged
dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag:mr_use_client
Jan 7, 2026
Merged

Replace dyn_client with client in all get calls#980
dbasunag merged 2 commits intoopendatahub-io:mainfrom
dbasunag:mr_use_client

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Jan 5, 2026

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • Tests
    • Standardized keyword argument usage across multiple test utilities and fixtures to improve consistency and maintainability within the test suite.
    • Updated test helper calls to use the new unified argument naming for internal API interactions; behavior and test outcomes remain unchanged with no user-facing impact.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 5, 2026

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/build-push-pr-image', '/hold', '/cherry-pick', '/lgtm', '/verified', '/wip'}

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 5, 2026

📝 Walkthrough

Walkthrough

Keyword argument refactor: multiple test utilities and fixtures updated to pass the Kubernetes client as client= instead of dyn_client= in various .get() calls (Pod, Service, Route, ConfigMap, Deployment). No function signatures or return types were changed.

Changes

Cohort / File(s) Summary
Model Registry Core Utils
tests/model_registry/utils.py
Updated three Pod/Service lookups (get_mr_service_by_label, wait_for_new_running_mr_pod, get_model_catalog_pod) to use client= instead of dyn_client=
Model Catalog Utilities
tests/model_registry/model_catalog/utils.py, tests/model_registry/model_catalog/catalog_config/utils.py, tests/model_registry/model_catalog/conftest.py
Replaced dyn_client= with client= in Route/resource .get() calls
Image Validation & SCC Utils
tests/model_registry/image_validation/conftest.py, tests/model_registry/scc/utils.py
Changed Pod.get(..., dyn_client=admin_client) to Pod.get(..., client=admin_client)
REST API Tests & Utilities
tests/model_registry/model_registry/rest_api/test_model_registry_rest_api.py, tests/model_registry/model_registry/rest_api/test_multiple_mr.py, tests/model_registry/model_registry/rest_api/utils.py
Refactored .get() calls for Pod, ConfigMap, and Deployment to use client= instead of dyn_client=
Async Job Utils
tests/model_registry/model_registry/async_job/utils.py
Updated Pod.get invocation to use client= keyword

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: replacing dyn_client with client parameter names across multiple get() calls throughout the test files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c57878 and ca16462.

📒 Files selected for processing (1)
  • tests/model_registry/model_catalog/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/model_registry/model_catalog/conftest.py (2)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (57-58)
tests/conftest.py (1)
  • admin_client (79-80)
🔇 Additional comments (1)
tests/model_registry/model_catalog/conftest.py (1)

398-401: Route.get() parameter change is correct and consistent across the codebase.

The refactoring from dyn_client to client parameter has been successfully completed. openshift-python-wrapper>=11.0.99 supports the client parameter, and all Route.get() calls in the codebase consistently use this parameter name with no remaining dyn_client usages.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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/scc/utils.py (1)

98-104: This change is incomplete and will cause runtime failures.

While the update from dyn_client to client in Pod.get() is correct, the parameter rename is incomplete across the codebase. A search reveals 40+ remaining dyn_client= usages in:

  • utilities/operator_utils.py, mariadb_utils.py, jira.py, kueue_utils.py, infra.py, inference_utils.py, general.py
  • conftest.py and test fixture files throughout tests/

If the method signatures have been changed from dyn_client= to client= globally, these remaining calls will fail at runtime. All occurrences must be updated consistently.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a1ebdcd and d2565dc.

📒 Files selected for processing (9)
  • tests/model_registry/async_job/utils.py
  • tests/model_registry/conftest.py
  • tests/model_registry/image_validation/conftest.py
  • tests/model_registry/model_catalog/utils.py
  • tests/model_registry/rest_api/test_model_registry_rest_api.py
  • tests/model_registry/rest_api/test_multiple_mr.py
  • tests/model_registry/rest_api/utils.py
  • tests/model_registry/scc/utils.py
  • tests/model_registry/utils.py
🧰 Additional context used
🧬 Code graph analysis (9)
tests/model_registry/model_catalog/utils.py (2)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (76-77)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/rest_api/utils.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/rest_api/test_model_registry_rest_api.py (2)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (76-77)
tests/model_registry/image_validation/conftest.py (1)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/conftest.py (1)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/async_job/utils.py (1)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/rest_api/test_multiple_mr.py (2)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (76-77)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/utils.py (2)
tests/conftest.py (1)
  • admin_client (79-80)
tests/model_registry/conftest.py (1)
  • model_registry_namespace (76-77)
🔇 Additional comments (11)
tests/model_registry/async_job/utils.py (1)

18-23: LGTM!

The keyword argument update from dyn_client to client is correct and aligns with the updated API parameter naming.

tests/model_registry/rest_api/utils.py (1)

267-268: LGTM!

The keyword argument change from dyn_client to client in the Deployment.get call is correct and maintains the function's behavior.

tests/model_registry/model_catalog/utils.py (2)

30-32: LGTM!

The keyword argument update from dyn_client to client in the Route.get call is correct.


47-51: LGTM!

The keyword argument update from dyn_client to client in the kind.get call is correct and maintains the validation logic.

tests/model_registry/image_validation/conftest.py (1)

29-34: LGTM!

The keyword argument update from dyn_client to client in the Pod.get call is correct.

tests/model_registry/rest_api/test_model_registry_rest_api.py (1)

161-161: LGTM! Parameter name updated correctly.

The keyword argument change from dyn_client to client aligns with the updated Pod.get API signature.

tests/model_registry/rest_api/test_multiple_mr.py (1)

62-62: LGTM! Parameter name updated correctly.

The keyword argument change from dyn_client to client aligns with the updated ConfigMap.get API signature.

tests/model_registry/utils.py (3)

67-67: LGTM! Parameter name updated correctly.

The keyword argument change from dyn_client to client in Service.get aligns with the updated API signature.


249-249: LGTM! Parameter name updated correctly.

The keyword argument change from dyn_client to client in Pod.get aligns with the updated API signature.


666-666: LGTM! Parameter name updated correctly.

The keyword argument change from dyn_client to client in Pod.get aligns with the updated API signature.

tests/model_registry/conftest.py (1)

636-636: Parameter change verified correctly. The keyword argument in Route.get() has been successfully updated from dyn_client to client at line 636. Verification confirms no remaining dyn_client usages in the model_registry test files.

Copy link
Copy Markdown
Contributor

@fege fege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@dbasunag dbasunag merged commit 6266b8e into opendatahub-io:main Jan 7, 2026
9 checks passed
@dbasunag dbasunag deleted the mr_use_client branch January 7, 2026 12:00
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 7, 2026

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

mwaykole pushed a commit to mwaykole/opendatahub-tests that referenced this pull request Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants