Make nodetype node selector label optional #2182
Conversation
|
WalkthroughThe change updates the construction of the ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/supervisor/src/workloadManager/kubernetes.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
| ...(env.KUBERNETES_WORKER_NODETYPE_LABEL | ||
| ? { | ||
| nodeSelector: { | ||
| nodetype: env.KUBERNETES_WORKER_NODETYPE_LABEL, | ||
| }, | ||
| } | ||
| : {}), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against whitespace-only env vars
The truthiness check will pass for any non-empty string, including a string that contains only whitespace.
That would result in a nodeSelector whose value is just spaces, yielding a pod that never schedules.
Consider trimming first:
- ...(env.KUBERNETES_WORKER_NODETYPE_LABEL
+ ...(env.KUBERNETES_WORKER_NODETYPE_LABEL?.trim()If the trimmed string is empty, the selector is omitted – exactly the behaviour intended by this PR.
Optionally cache the trimmed value in a local const to avoid repeating the lookup.
📝 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.
| ...(env.KUBERNETES_WORKER_NODETYPE_LABEL | |
| ? { | |
| nodeSelector: { | |
| nodetype: env.KUBERNETES_WORKER_NODETYPE_LABEL, | |
| }, | |
| } | |
| : {}), | |
| }; | |
| ...(env.KUBERNETES_WORKER_NODETYPE_LABEL?.trim() | |
| ? { | |
| nodeSelector: { | |
| nodetype: env.KUBERNETES_WORKER_NODETYPE_LABEL, | |
| }, | |
| } | |
| : {}), | |
| }; |
🤖 Prompt for AI Agents
In apps/supervisor/src/workloadManager/kubernetes.ts around lines 231 to 238,
the code checks the environment variable KUBERNETES_WORKER_NODETYPE_LABEL for
truthiness but does not handle cases where it contains only whitespace. To fix
this, trim the environment variable value first and check if the trimmed string
is non-empty before setting the nodeSelector. Cache the trimmed value in a local
constant to avoid multiple lookups and ensure the nodeSelector is only set when
the trimmed value is valid.
When
KUBERNETES_WORKER_NODETYPE_LABELis set to an empty string, runs will now be scheduled on any node. This mostly preserves backwards compatibility, except where this has already been set to an empty string.