-
Notifications
You must be signed in to change notification settings - Fork 0
fix: removed k8s pod config, now routing stuff through settings #115
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.
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
Conversation
📝 WalkthroughWalkthroughThe PR removes the K8sWorkerConfig dataclass and migrates Kubernetes worker configuration into the centralized Settings object. PodBuilder and KubernetesWorker now accept Settings, namespace and resource values are read from Settings, K8S_MAX_CONCURRENT_PODS was added, and provider wiring/imports were simplified. Changes
Sequence Diagram(s)(Skipped — changes are a refactor of configuration wiring and do not introduce a new multi-component control flow that requires visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/settings.py`:
- Around line 37-38: Validate the K8S_MAX_CONCURRENT_PODS setting before it's
used: ensure the integer K8S_MAX_CONCURRENT_PODS is strictly greater than 0
(since it's passed to asyncio.Semaphore), and raise a clear ValueError (or
fallback to a safe default) if the value is <= 0 or not an int; update the
settings initialization/validation logic near the K8S_MAX_CONCURRENT_PODS
declaration and any config-loading function that constructs the Semaphore so the
check runs at startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/tests/e2e/test_k8s_worker_create_pod.py">
<violation number="1" location="backend/tests/e2e/test_k8s_worker_create_pod.py:29">
P2: Using `MagicMock(wraps=test_settings)` does not preserve attribute values; it returns MagicMocks for attributes, which breaks KubernetesWorker/metrics initialization and can enable OTLP exporting with an invalid endpoint. Use a real settings copy with an updated field instead of a MagicMock.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|



Summary by cubic
Consolidated all Kubernetes worker config into Settings and removed K8sWorkerConfig. This centralizes pod resources, namespace, and concurrency and simplifies instantiation and tests.
Refactors
Migration
Written for commit 42c8f8d. Summary will update on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.