replacing patch way to ResourceEditor#519
Conversation
Signed-off-by: Milind Waykole <mwaykole@redhat.com>
📝 WalkthroughWalkthroughReplaced JSON Patch updates with ResourceEditor-based updates in a KEDA scaling CPU test. The test now converts the InferenceService to dict, conditionally injects external authenticationRef settings into autoScaling.metrics, and applies the change via ResourceEditor. The rest of the test flow remains unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/verified', '/wip', '/build-push-pr-image', '/lgtm', '/cherry-pick', '/hold'} |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/model_serving/model_server/keda/test_isvc_keda_scaling_cpu.py (2)
85-97: RBAC/regression risk: patch now uses the unprivileged client.Previously the JSON patch used admin_client; ResourceEditor operates with the client of the resource key you pass. Since stressed_ovms_keda_inference_service originates from the unprivileged_client fixture (see conftest), this change may fail under stricter RBAC.
If admin privileges are required here, patch via an admin-scoped resource object:
- ResourceEditor( - patches={ - stressed_ovms_keda_inference_service: { + admin_isvc = InferenceService( + name=stressed_ovms_keda_inference_service.name, + namespace=stressed_ovms_keda_inference_service.namespace, + client=admin_client, + ) + ResourceEditor( + patches={ + admin_isvc: { "spec": { "predictor": { "autoScaling": { "metrics": metrics, } } } } } ).update()Follow-up:
- Confirm the unprivileged SA has patch permission on InferenceServices; otherwise adopt the admin-scoped edit above.
76-84: Optional: Avoid relying on metrics[0] ordering.If multiple metrics are configured, indexing [0] is brittle. Consider updating the first metric that has an external block:
Example (outside-of-diff illustration):
"""
for m in metrics:
if isinstance(m, dict) and isinstance(m.get("external"), dict):
m["external"]["authModes"] = "bearer"
m["external"]["authenticationRef"] = {"name": "inference-prometheus-auth"}
break
"""This makes the code resilient to ordering changes from upstream helpers.
To be safe, please verify whether multiple metrics could be present in this test’s auto scaling config (create_keda_auto_scaling_config). If yes, prefer the looped approach above.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/model_serving/model_server/keda/test_isvc_keda_scaling_cpu.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dbasunag
PR: opendatahub-io/opendatahub-tests#504
File: tests/model_registry/upgrade/conftest.py:20-30
Timestamp: 2025-08-11T12:14:34.102Z
Learning: In tests/model_registry/upgrade/conftest.py, when using ResourceEditor to patch DataScienceCluster resources with a partial components dictionary like `{DscComponents.MODELREGISTRY: {"managementState": ...}}`, the patch is applied correctly without overwriting other components in spec.components. ResourceEditor performs the appropriate merge operation for this resource type.
🧬 Code Graph Analysis (1)
tests/model_serving/model_server/keda/test_isvc_keda_scaling_cpu.py (1)
tests/model_serving/model_server/keda/conftest.py (1)
stressed_ovms_keda_inference_service(165-194)
🔇 Additional comments (2)
tests/model_serving/model_server/keda/test_isvc_keda_scaling_cpu.py (2)
2-2: Good switch to ResourceEditor.Importing and using ResourceEditor improves readability and aligns with our test infra patterns for patching resources.
79-83: KEDA Prometheus trigger metadata fields are correctConfirmed that in the Prometheus scaler spec:
authModesandauthenticationRefare sibling fields.authModesaccepts a comma-separated string (e.g."bearer").The current test snippet correctly sets:
metrics[0]["external"]["authenticationRef"] = { "authModes": "bearer", "authenticationRef": {"name": "inference-prometheus-auth"}, }…which matches the official KEDA documentation. No changes required.
| if metrics and isinstance(metrics[0], dict) and metrics[0].get("external") is not None: | ||
| metrics[0].setdefault("external", {})["authenticationRef"] = { | ||
| "authModes": "bearer", | ||
| "authenticationRef": {"name": "inference-prometheus-auth"}, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: authModes incorrectly nested under authenticationRef.
In the KEDA trigger spec, authModes and authenticationRef are sibling fields (under external), not nested. The current structure will produce an invalid spec.
Apply this diff to fix the structure and harden the type check:
-if metrics and isinstance(metrics[0], dict) and metrics[0].get("external") is not None:
- metrics[0].setdefault("external", {})["authenticationRef"] = {
- "authModes": "bearer",
- "authenticationRef": {"name": "inference-prometheus-auth"},
- }
+if metrics and isinstance(metrics[0], dict) and isinstance(metrics[0].get("external"), dict):
+ external = metrics[0]["external"]
+ external["authModes"] = "bearer"
+ external["authenticationRef"] = {"name": "inference-prometheus-auth"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if metrics and isinstance(metrics[0], dict) and metrics[0].get("external") is not None: | |
| metrics[0].setdefault("external", {})["authenticationRef"] = { | |
| "authModes": "bearer", | |
| "authenticationRef": {"name": "inference-prometheus-auth"}, | |
| } | |
| if metrics and isinstance(metrics[0], dict) and isinstance(metrics[0].get("external"), dict): | |
| external = metrics[0]["external"] | |
| external["authModes"] = "bearer" | |
| external["authenticationRef"] = {"name": "inference-prometheus-auth"} |
🤖 Prompt for AI Agents
In tests/model_serving/model_server/keda/test_isvc_keda_scaling_cpu.py around
lines 79 to 83, the code nests authModes under authenticationRef and doesn't
robustly check external's type; update the block to first verify metrics and
that metrics[0].get("external") is a dict, then set external["authModes"] =
["bearer"] and external["authenticationRef"] = {"name":
"inference-prometheus-auth"} as sibling keys under external (not nested),
ensuring the correct list value for authModes and avoiding invalid spec shape.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if metrics and isinstance(metrics[0], dict) and metrics[0].get("external") is not None: | ||
| metrics[0].setdefault("external", {})["authenticationRef"] = { | ||
| "authModes": "bearer", | ||
| "authenticationRef": {"name": "inference-prometheus-auth"}, | ||
| } |
rhods-ci-bot
removed
Verified
|
Status of building tag latest: success. |
4 participants
Summary by CodeRabbit