chore: move all placement tag helpers to core #2403
Conversation
|
WalkthroughIntroduces a new PlacementTagProcessor in packages/core/src/v3/serverOnly/placementTags.ts to convert placement tags into Kubernetes nodeSelector entries, with configurable enabled flag and prefix. Exports it via serverOnly index and adds a placementTag helper there. Removes the prior placementTag helper from packages/core/src/v3/schemas/runEngine.ts. Updates internal-packages/run-engine dequeueSystem to import placementTag from the serverOnly submodule. Refactors apps/supervisor KubernetesWorkloadManager to instantiate and use PlacementTagProcessor for nodeSelector generation, removing its local PlacementConfig, getter, and warning method. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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
🧹 Nitpick comments (3)
packages/core/src/v3/utils/placementTags.ts (3)
4-7: Prefer type aliases over interfaces in TS (repo convention)In this repo we prefer types over interfaces. Consider switching PlacementConfig to a type alias for consistency.
Apply:
-export interface PlacementConfig { +export type PlacementConfig = { enabled: boolean; prefix: string; -} +}
28-36: Optional: avoid unschedulable pods when tag has no values; warn on conflicting valuesSetting a nodeSelector value to an empty string usually won’t match any nodes, potentially making pods unschedulable. Also, silently overwriting an existing nodeSelector value for the same key can be surprising.
Apply:
- for (const tag of placementTags) { + for (const tag of placementTags) { const labelKey = `${this.config.prefix}/${tag.key}`; // Print warnings (if any) this.printTagWarnings(tag); // For now we only support single values via nodeSelector - nodeSelector[labelKey] = tag.values?.[0] ?? ""; + const candidateValue = tag.values?.[0]; + + // If no usable value, skip setting the label to avoid unschedulable pods + if (candidateValue === undefined || candidateValue === null || candidateValue === "") { + continue; + } + + // Warn if we're overwriting a different existing value + if (labelKey in nodeSelector && nodeSelector[labelKey] !== candidateValue) { + this.logger.warn("Overriding existing nodeSelector value for placement tag", { + key: labelKey, + previous: nodeSelector[labelKey], + next: candidateValue, + }); + } + + nodeSelector[labelKey] = candidateValue; }Note: If you adopt the normalizedPrefix fix above, update the labelKey line accordingly.
1-64: Add unit tests for core conversion behaviorRecommend adding focused tests for:
- disabled=true returns existing selector untouched
- no tags returns existing selector untouched
- single tag with single value maps to prefix/key correctly
- tags with empty values are skipped (if you adopt the optional change) or produce empty string if you keep current behavior
- multiple values emits a warning and uses the first
- merging with existing nodeSelector (no overwrite, overwrite warning)
- blank and trailing-slash prefix normalization
I can scaffold these tests if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/supervisor/src/workloadManager/kubernetes.ts(3 hunks)packages/core/src/v3/index.ts(1 hunks)packages/core/src/v3/schemas/runEngine.ts(0 hunks)packages/core/src/v3/utils/placementTags.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/v3/schemas/runEngine.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/src/v3/index.tspackages/core/src/v3/utils/placementTags.tsapps/supervisor/src/workloadManager/kubernetes.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/index.tspackages/core/src/v3/utils/placementTags.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/core/src/v3/index.ts (1)
72-72: Public re-export for placement tags looks goodRe-exporting the placement tag utilities from the v3 barrel keeps consumers off deep imports. No concerns.
packages/core/src/v3/utils/placementTags.ts (4)
21-23: Early-return behavior matches previous logicReturning the existing nodeSelector when disabled or when no tags are provided maintains the previous behavior path. Looks good.
41-55: Structured warnings are appropriate and non-intrusiveUsing SimpleStructuredLogger with clear messages for “no values” and “multiple values” is a good balance. No changes needed.
62-64: placementTag helper is fine for ergonomicsHelper matches the current PlacementTag shape and simplifies callers. No issues.
1-64: No stale deep imports found — placementTag is re-exported and used correctlyI searched the repo. The helper remains at packages/core/src/v3/utils/placementTags.ts and is re-exported from packages/core/src/v3/index.ts. The only non-test usage found imports placementTag from "@trigger.dev/core/v3" (no direct deep imports to the old helper path).
Files of interest:
- packages/core/src/v3/utils/placementTags.ts — placementTag definition (export).
- packages/core/src/v3/index.ts:72 — re-exports "./utils/placementTags.js".
- internal-packages/run-engine/src/engine/systems/dequeueSystem.ts — imports placementTag from "@trigger.dev/core/v3" (import at ~line 4; usage at ~line 477).
apps/supervisor/src/workloadManager/kubernetes.ts (4)
8-8: Good: using the new PlacementTagProcessor from coreImport via the v3 barrel avoids deep imports and keeps the surface area consistent.
21-21: Private processor field: good encapsulationKeeps placement logic centralized and testable.
25-28: Confirm env types for processor configEnsure env.PLACEMENT_TAGS_ENABLED is a boolean and env.PLACEMENT_TAGS_PREFIX is a string (not undefined). If env is stringly-typed, coerce/parse here before passing to the processor.
If needed, consider:
this.placementTagProcessor = new PlacementTagProcessor({ enabled: Boolean(env.PLACEMENT_TAGS_ENABLED), prefix: env.PLACEMENT_TAGS_PREFIX ?? "", });Note: With the normalizedPrefix fix in core, empty or trailing slashes are handled gracefully.
41-44: Delegation to processor simplifies podSpec constructionThe new call cleanly merges existing nodeSelector with placement-generated labels and centralizes warnings. Assuming the core fix for prefix normalization, this should be robust.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)
4-5: Use a type-only import for DequeuedMessage; keep RetryOptions as a value importDequeuedMessage is only used for typing (via satisfies). This avoids emitting unnecessary runtime imports and aligns with existing style (Line 1 already uses import type).
-import { DequeuedMessage, RetryOptions } from "@trigger.dev/core/v3"; +import type { DequeuedMessage } from "@trigger.dev/core/v3"; +import { RetryOptions } from "@trigger.dev/core/v3"; import { placementTag } from "@trigger.dev/core/v3/serverOnly";packages/core/src/v3/serverOnly/placementTags.ts (4)
4-7: Prefer type alias over interface in corePer repo guidelines, we usually use types over interfaces in TypeScript.
-export interface PlacementConfig { - enabled: boolean; - prefix: string; -} +export type PlacementConfig = { + enabled: boolean; + prefix: string; +};
25-36: Normalize prefix and avoid generating invalid label keys; skip empty values instead of assigning ""
- If prefix is empty or contains a trailing slash,
${prefix}/${key}can produce invalid keys like/keyor a double slash.- Assigning an empty string for a nodeSelector value can unintentionally render pods unschedulable. Better to skip tags with no usable value.
- // Convert placement tags to nodeSelector labels - for (const tag of placementTags) { - const labelKey = `${this.config.prefix}/${tag.key}`; - - // Print warnings (if any) - this.printTagWarnings(tag); - - // For now we only support single values via nodeSelector - nodeSelector[labelKey] = tag.values?.[0] ?? ""; - } + // Convert placement tags to nodeSelector labels + const normalizedPrefix = this.#normalizePrefix(this.config.prefix); + for (const tag of placementTags) { + const labelKey = normalizedPrefix ? `${normalizedPrefix}/${tag.key}` : tag.key; + + // Print warnings (if any) + this.printTagWarnings(tag); + + // For now we only support single values via nodeSelector + const firstValue = tag.values?.[0]; + if (typeof firstValue !== "string" || firstValue.length === 0) { + // Skip tags with no usable value to avoid creating a selector that never matches + continue; + } + nodeSelector[labelKey] = firstValue; + }Add the helper (outside the selected range, near other privates):
// Private helper to normalize/clean the prefix #normalizePrefix(prefix: string): string { const p = (prefix ?? "").trim().replace(/\/+$/, ""); return p.length > 0 ? p : ""; }
41-49: Reduce potential PII in logs; log minimal tag metadataPassing the full tag object to logs could leak values. Prefer logging key and counts. Keep warnings but trim payload.
private printTagWarnings(tag: PlacementTag) { if (!tag.values || tag.values.length === 0) { // No values provided - this.logger.warn("Placement tag has no values, using empty string", tag); + this.logger.warn("Placement tag has no values; skipping", { + key: tag.key, + valuesCount: tag.values?.length ?? 0, + }); } else if (tag.values.length > 1) { // Multiple values provided - this.logger.warn("Placement tag has multiple values, only using first one", tag); + this.logger.warn("Placement tag has multiple values; only using first", { + key: tag.key, + valuesCount: tag.values.length, + }); } }
12-13: Optional: validate config on construction to catch misconfiguration earlyWhen enabled but prefix is empty/invalid, warn once at construction time to aid debugging.
I can add a small zod schema for PlacementConfig and validate here. Want me to follow up with a patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/supervisor/src/workloadManager/kubernetes.ts(3 hunks)internal-packages/run-engine/src/engine/systems/dequeueSystem.ts(1 hunks)packages/core/src/v3/serverOnly/index.ts(1 hunks)packages/core/src/v3/serverOnly/placementTags.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/src/v3/serverOnly/index.tspackages/core/src/v3/serverOnly/placementTags.tsinternal-packages/run-engine/src/engine/systems/dequeueSystem.tsapps/supervisor/src/workloadManager/kubernetes.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/serverOnly/index.tspackages/core/src/v3/serverOnly/placementTags.ts
🧬 Code Graph Analysis (2)
packages/core/src/v3/serverOnly/placementTags.ts (1)
packages/core/src/v3/utils/structuredLogger.ts (1)
SimpleStructuredLogger(21-102)
apps/supervisor/src/workloadManager/kubernetes.ts (1)
packages/core/src/v3/serverOnly/placementTags.ts (1)
PlacementTagProcessor(9-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- 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 (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
🔇 Additional comments (5)
packages/core/src/v3/serverOnly/index.ts (1)
9-9: Expose server-only placement tags API via index: LGTMRe-export looks correct and keeps the server-only surface tidy.
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)
5-5: All placementTag imports point to serverOnly — no action requiredVerified: placementTag is defined under packages/core/src/v3/serverOnly and the only import of placementTag is from "@trigger.dev/core/v3/serverOnly". No imports of placementTag from "@trigger.dev/core/v3" (non-serverOnly) were found.
Files checked (relevant hits):
- packages/core/src/v3/serverOnly/placementTags.ts — export function placementTag(...) (definition)
- internal-packages/run-engine/src/engine/systems/dequeueSystem.ts — import { placementTag } from "@trigger.dev/core/v3/serverOnly"; (line 5) and usage at line 478: placementTags: [placementTag("paid", ...)]
packages/core/src/v3/serverOnly/placementTags.ts (1)
56-58: Helper is straightforward: LGTMThe minimal builder promotes consistent tag shape and pairs well with the processor.
apps/supervisor/src/workloadManager/kubernetes.ts (2)
8-8: Server-only import: LGTMCorrectly sources the processor from the server-only module.
41-44: Delegating nodeSelector generation to the processor: LGTMCentralizes logic and keeps K8s manager lean.
| private placementTagProcessor: PlacementTagProcessor; | ||
|
|
||
| constructor(private opts: WorkloadManagerOptions) { | ||
| this.k8s = createK8sApi(); | ||
| this.placementTagProcessor = new PlacementTagProcessor({ | ||
| enabled: env.PLACEMENT_TAGS_ENABLED, | ||
| prefix: env.PLACEMENT_TAGS_PREFIX, | ||
| }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make the processor field readonly; ensure env prefix is set when enabled
- Marking the field readonly communicates intent and prevents accidental reassignment.
- If PLACEMENT_TAGS_ENABLED is true but the prefix is missing or has a trailing slash, label keys could be malformed unless normalized in the processor (recommended). Validate or normalize centrally.
- private placementTagProcessor: PlacementTagProcessor;
+ private readonly placementTagProcessor: PlacementTagProcessor;To verify env wiring and defaults:
🏁 Script executed:
#!/bin/bash
# Check env.ts/js for PLACEMENT_TAGS_ENABLED/PLACEMENT_TAGS_PREFIX definitions and defaults
fd -t f -g '*env.*' apps | xargs -I{} rg -n -C2 'PLACEMENT_TAGS_(ENABLED|PREFIX)' {}Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Search: placementTagProcessor usages ----"
rg -n --hidden -S 'placementTagProcessor' || true
echo
echo "---- Search: PlacementTagProcessor symbols ----"
rg -n --hidden -S 'PlacementTagProcessor' || true
echo
echo "---- Search: PLACEMENT_TAGS_* env defs ----"
rg -n --hidden -S 'PLACEMENT_TAGS_(ENABLED|PREFIX)' || true
echo
echo "---- Show files that define PLACEMENT_TAGS_PREFIX (first 200 lines) ----"
files=$(rg -l --hidden -S 'PLACEMENT_TAGS_PREFIX' || true)
if [ -n "$files" ]; then
for f in $files; do
echo "---- File: $f ----"
sed -n '1,200p' "$f"
echo
done
else
echo "No files found defining PLACEMENT_TAGS_PREFIX"
fi
echo
echo "---- Show files that define PlacementTagProcessor (first 200 lines) ----"
files2=$(rg -l --hidden -S 'class PlacementTagProcessor|function PlacementTagProcessor|new PlacementTagProcessor' || true)
if [ -n "$files2" ]; then
for f in $files2; do
echo "---- File: $f ----"
sed -n '1,200p' "$f"
echo
done
else
echo "No files found with obvious PlacementTagProcessor definition; showing files that reference it instead"
rg -n --hidden -S 'PlacementTagProcessor' || true
fiLength of output: 24031
Make placementTagProcessor readonly and normalize/validate PLACEMENT_TAGS_PREFIX
Quick summary: I verified that apps/supervisor/src/env.ts provides a default PLACEMENT_TAGS_PREFIX ("node.cluster.x-k8s.io") but PlacementTagProcessor uses the prefix directly and does not normalize or validate it. Also the field in KubernetesWorkloadManager is not readonly. Recommend making the field readonly and normalizing/validating the prefix centrally.
Files to change:
- apps/supervisor/src/workloadManager/kubernetes.ts — mark the processor field readonly.
- packages/core/src/v3/serverOnly/placementTags.ts — normalize (trim trailing slashes) and validate prefix when enabled; use the normalized prefix when building label keys.
Suggested diffs:
Make field readonly in KubernetesWorkloadManager:
- private placementTagProcessor: PlacementTagProcessor;
+ private readonly placementTagProcessor: PlacementTagProcessor;Normalize/validate prefix in PlacementTagProcessor (outline):
-export class PlacementTagProcessor {
- private readonly logger = new SimpleStructuredLogger("placement-tag-processor");
-
- constructor(private readonly config: PlacementConfig) {}
+export class PlacementTagProcessor {
+ private readonly logger = new SimpleStructuredLogger("placement-tag-processor");
+ private readonly config: PlacementConfig;
+ private readonly prefix: string;
+
+ constructor(config: PlacementConfig) {
+ this.config = config;
+ // normalize prefix: remove trailing slashes
+ this.prefix = (config.prefix ?? "").replace(/\/+$/, "");
+ if (config.enabled && this.prefix.length === 0) {
+ throw new Error(
+ "PlacementTagProcessor: PLACEMENT_TAGS_PREFIX must be a non-empty string when PLACEMENT_TAGS_ENABLED is true"
+ );
+ }
+ }And replace label construction in convertToNodeSelector:
- const labelKey = `${this.config.prefix}/${tag.key}`;
+ const labelKey = `${this.prefix}/${tag.key}`;Note: apps/supervisor/src/env.ts currently defines:
- PLACEMENT_TAGS_ENABLED: BoolEnv.default(false)
- PLACEMENT_TAGS_PREFIX: z.string().default("node.cluster.x-k8s.io")
Even with that default, normalizing/validating is recommended because the prefix can be overridden via environment and a trailing slash or empty value would produce malformed label keys.
📝 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.
| private placementTagProcessor: PlacementTagProcessor; | |
| constructor(private opts: WorkloadManagerOptions) { | |
| this.k8s = createK8sApi(); | |
| this.placementTagProcessor = new PlacementTagProcessor({ | |
| enabled: env.PLACEMENT_TAGS_ENABLED, | |
| prefix: env.PLACEMENT_TAGS_PREFIX, | |
| }); | |
| private readonly placementTagProcessor: PlacementTagProcessor; | |
| constructor(private opts: WorkloadManagerOptions) { | |
| this.k8s = createK8sApi(); | |
| this.placementTagProcessor = new PlacementTagProcessor({ | |
| enabled: env.PLACEMENT_TAGS_ENABLED, | |
| prefix: env.PLACEMENT_TAGS_PREFIX, | |
| }); |
No description provided.