fix: add support for Strimzi v1.0+ api#534
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughClusterLoader now probes and caches the StrimziPodSet API version before listing custom resources, using v1 when available and falling back to v1beta2 when needed. The listing path uses the detected version, and tests cover resolution, fallback, namespace probing, and retry behavior. ChangesStrimziPodSet API version detection
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
2af9ffd to
12f4956
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_strimzipodset_api_version.py (1)
27-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the core fallback logic; consider adding a caching/memoization test.
The three tests cover v1 success, v1→v1beta2 fallback, and both-unavailable→None well. Missing: a test asserting that calling
_resolve_strimzipodset_api_version()twice on the sameloaderonly performs the underlying API calls once (verifying_strimzipodset_api_version_checkedmemoization actually takes effect), and a test for the namespaced (config.namespaces = ["ns1"]) code path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_strimzipodset_api_version.py` around lines 27 - 74, Add tests around loader._resolve_strimzipodset_api_version() to cover the memoized cache and the namespaced path. Specifically, verify that calling the method twice on the same loader only triggers the underlying custom_objects list_cluster_custom_object calls once by asserting _strimzipodset_api_version_checked prevents повторных API requests, and add a case with config.namespaces set (for example a single namespace) that exercises the list_namespaced_custom_object branch instead of the cluster-scoped call.robusta_krr/core/integrations/kubernetes/__init__.py (1)
492-501: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMinor: probe only checks the first configured namespace; synthetic exception is a bit indirect.
Only
self.namespaces[0]is probed when namespaces are explicitly configured — if that particular namespace is inaccessible via RBAC while others aren't, version detection could fail unnecessarily. Also, raising a syntheticApiException(status=404, ...)just to reuse thecontinuebranch (line 493) works but reads a bit indirectly; an explicitcontinuewith its own log line would be clearer.Low priority given this is a narrow edge case with a safe fallback (mark kind unavailable).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@robusta_krr/core/integrations/kubernetes/__init__.py` around lines 492 - 501, The version probe in the Kubernetes integration only checks self.namespaces[0] and uses a synthetic ApiException to flow into the fallback path, which is too indirect. Update the probing logic in the relevant namespace loop so it iterates through all configured namespaces until one succeeds, and on the no-namespaces case use an explicit continue/fallback path instead of raising a fake exception. Keep the behavior in the Kubernetes custom object probe code (the list_namespaced_custom_object call) the same otherwise, and add a clear log message when a namespace is skipped or unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@robusta_krr/core/integrations/kubernetes/__init__.py`:
- Around line 476-515: In _resolve_strimzipodset_api_version, non-404 probe
failures are being cached as None or can escape and fail the scan; update the
probing logic for the StrimziPodSet API versions so transient non-404
ApiException cases still try the next version instead of breaking, and broaden
handling to catch client/network exceptions raised by the Kubernetes client.
Keep the discovery behavior aligned with __list_hpa / _try_list_hpa so only
missing resources are treated as unavailable while other errors are logged and
safely degraded without poisoning _strimzipodset_api_version_checked for the
rest of the run.
- Around line 476-527: The StrimziPodSet API probe in
_resolve_strimzipodset_api_version() is being executed synchronously on the
event loop via _list_strimzipodsets() and list_scannable_objects(), which can
block other work. Make _list_strimzipodsets() async and run the custom_objects
list_cluster_custom_object/list_namespaced_custom_object probe inside the
executor, following the same pattern used by _list_namespaced_or_global_objects,
_list_jobs_for_cronjobs, and list_pods. Keep
_resolve_strimzipodset_api_version() synchronous so callers and tests that
invoke it directly still work.
---
Nitpick comments:
In `@robusta_krr/core/integrations/kubernetes/__init__.py`:
- Around line 492-501: The version probe in the Kubernetes integration only
checks self.namespaces[0] and uses a synthetic ApiException to flow into the
fallback path, which is too indirect. Update the probing logic in the relevant
namespace loop so it iterates through all configured namespaces until one
succeeds, and on the no-namespaces case use an explicit continue/fallback path
instead of raising a fake exception. Keep the behavior in the Kubernetes custom
object probe code (the list_namespaced_custom_object call) the same otherwise,
and add a clear log message when a namespace is skipped or unavailable.
In `@tests/test_strimzipodset_api_version.py`:
- Around line 27-74: Add tests around
loader._resolve_strimzipodset_api_version() to cover the memoized cache and the
namespaced path. Specifically, verify that calling the method twice on the same
loader only triggers the underlying custom_objects list_cluster_custom_object
calls once by asserting _strimzipodset_api_version_checked prevents повторных
API requests, and add a case with config.namespaces set (for example a single
namespace) that exercises the list_namespaced_custom_object branch instead of
the cluster-scoped call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7513a6de-80dc-47e1-a9f7-70a7fae7a98a
📒 Files selected for processing (2)
robusta_krr/core/integrations/kubernetes/__init__.pytests/test_strimzipodset_api_version.py
12f4956 to
fb86f0d
Compare
KRR’s StrimziPodSet support was hardcoded to
core.strimzi.io/v1beta2. That worked when the feature landed, but Strimzi 1.0 droppedv1beta2.This probes
v1first, falls back tov1beta2for older operators, and caches whichever version works for the rest of the scan.