Skip to content

replacing patch way to ResourceEditor #519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dbasunag merged 4 commits into from
Aug 13, 2025
Merged

replacing patch way to ResourceEditor #519

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e37d3a0
run pre-commit for fixng check
mwaykole Aug 13, 2025
2f6c8ff
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 13, 2025
ac263ca
Merge branch 'main' into kedafix
mwaykole Aug 13, 2025
702349f
Merge branch 'main' into kedafix
dbasunag Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions tests/model_serving/model_server/keda/test_isvc_keda_scaling_cpu.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import pytest
from ocp_resources.resource import ResourceEditor
from simple_logger.logger import get_logger
from typing import Any, Generator
from kubernetes.dynamic import DynamicClient
Expand Down Expand Up @@ -72,19 +73,28 @@ def test_ovms_keda_scaling_verify_scaledobject(
"""Test KEDA ScaledObject configuration and run inference multiple times to trigger scaling."""

if is_jira_open(jira_id="RHOAIENG-31386", admin_client=admin_client):
patch_operations = [
{
"op": "add",
"path": "/spec/predictor/autoScaling/metrics/0/external/authenticationRef",
"value": {"authModes": "bearer", "authenticationRef": {"name": "inference-prometheus-auth"}},
isvc_dict = stressed_ovms_keda_inference_service.instance.to_dict()
metrics = isvc_dict.get("spec", {}).get("predictor", {}).get("autoScaling", {}).get("metrics", [])

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"},
}
Comment on lines +79 to 83
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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.

Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag Aug 13, 2025

Choose a reason for hiding this comment

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

@mwaykole check if this makes sense.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

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

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!

]
admin_client.resources.get(api_version="v1beta1", kind="InferenceService").patch(
name=stressed_ovms_keda_inference_service.name,
namespace=stressed_ovms_keda_inference_service.namespace,
body=patch_operations,
content_type="application/json-patch+json",
)

ResourceEditor(
patches={
stressed_ovms_keda_inference_service: {
"spec": {
"predictor": {
"autoScaling": {
"metrics": metrics,
}
}
}
}
}
).update()

verify_keda_scaledobject(
client=unprivileged_client,
Expand Down
Loading