refactor: make orchestrator engine-agnostic at the typed surface#26
Conversation
Remove Unity-specific licensing fields from orchestrator's typed surfaces (BuildParameters, OrchestratorConfig, CLI input mapper, build/orchestrate yargs commands, cli-plugin adapter): - unitySerial - unityLicensingServer - skipActivation These were vestigial — BuildParameters.create() hardcoded them to empty strings, no orchestrator service read them for logic. They existed only to mirror unity-builder's BuildParameters shape, which is exactly the boundary violation: orchestrator's domain is dispatch + providers, not engine-specific licensing. The plugin contract (coreParams: Record<string, any>) is already opaque and engine-agnostic. The host (unity-builder today, @game-ci/cli in the future) passes its full BuildParameters object through; orchestrator reads only generic build context (targetPlatform, projectPath, etc.) and its plugin-owned config from env/inputs. Engine-specific keys ride in the dict untouched. No companion change needed in unity-builder: it continues to construct its own BuildParameters with whatever fields it wants and pass it as coreParams. The dict's index signature accepts everything. Documentation: - Tracking issue #25 lays out the full architecture, today/future state, and migration runway. - Code comments in plugin-lifecycle.ts, interfaces.ts, build-parameters.ts, build.ts, orchestrate.ts, input-mapper.ts, build-parameters-adapter.ts reference the issue and explain the boundary intent so the next contributor understands why these fields are not (and must not be) declared here. Out of scope (separate cleanups, noted in tracking issue): - cacheUnityInstallationOnMac / unityHubVersionOnMac in input-mapper (Mac runtime install caching, more entangled) - task-parameter-serializer.ts UNITY_SERIAL well-known-secret list (well-known-secrets generalization) - activate CLI command (Unity-specific legacy helper, leave as-is) Refs game-ci/unity-builder#739 and game-ci/unity-builder#838 (the user- facing fix that motivated this boundary cleanup).
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ 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 |
1 participant
Summary
Removes Unity-specific licensing fields from orchestrator's typed surfaces. Orchestrator's domain is dispatch + providers, not engine-specific licensing — the fields removed here were vestigial leftovers from the original carve-out from unity-builder.
Tracking issue: #25 (full architecture, today/future state, migration runway)
Supersedes: #24 (closed). That PR mirrored
unityLicensingToolsetacross orchestrator's typed surfaces. The architectural review (#25) concluded orchestrator shouldn't carry any Unity-specific licensing field, so adding the toolset here was the wrong direction. User-facing toolset support remains live in game-ci/unity-builder#838 and game-ci/cli#50; orchestrator forwards the value opaquely via the existingcoreParams: Record<string, any>plugin contract.Motivated by game-ci/unity-builder#739 — adding one Unity field today touches ~13 places across three repos. After this refactor, adding a Unity-licensing field touches zero orchestrator files.
What changes
Removed from typed surfaces only:
unitySerialunityLicensingServerskipActivation(Plus
unityLicensingToolset— never lands now that #24 is closed.)Sites updated:
src/model/build-parameters.tscreate()defaults removed; doc-block expanded with engine-agnostic notesrc/model/orchestrator/interfaces.ts(OrchestratorConfig)src/cli/input-mapper.tsskipActivation,unityLicensingServer); index signature[key: string]: unknowncontinues to accept anything else hosts passsrc/cli/commands/build.ts--skip-activation,--unity-licensing-serveryargs options removed; comment points users to env vars / engine-specific host CLIsrc/cli/commands/orchestrate.ts--skip-activationremovedsrc/cli-plugin/build-parameters-adapter.tssrc/model/orchestrator/orchestrator.tsupdateStatusWithBuildParametersannotated (still scrubs by key name; safe via BP's index signature)src/plugin-lifecycle.tssrc/cli/__tests__/commands.test.tsWhy no companion PR is needed in unity-builder or cli
The plugin contract (
coreParams: Record<string, any>) is already opaque. unity-builder constructs its ownBuildParameters(withunityLicensingServer,unityLicensingToolset, etc.) and passes it as the dict. Orchestrator simply stops typing those keys — they ride through the index signature untouched, get forwarded to the build container as env vars bytask-parameter-serializer.ts, and reach Unity. No host-side change required.Are these changes opt-in / backwards compatible?
Mixed — the structure is honest:
--skip-activation/--unity-licensing-serverdirectlySKIP_ACTIVATION,UNITY_LICENSING_SERVER) or invoke the engine-specific host CLI (unity-builder action / @game-ci/cli)OrchestratorConfig.unityLicensingServeretc.The breaking-change blast radius is genuinely small (the fields were dead weight), but I'm flagging it honestly rather than calling this a no-op.
What's intentionally NOT in this PR
These are documented in #25 as separate, follow-up cleanups:
cacheUnityInstallationOnMac/unityHubVersionOnMacin input-mapper. More entangled with Mac runtime install caching. Separate PR.task-parameter-serializer.tsUNITY_SERIAL well-known-secret list. Belongs in a "well-known-secrets generalization" cleanup.activateCLI command. Genuinely Unity-specific, but removing it is breaking and the command isn't part of the plugin contract. Migrate later.platform-setup.ts. Naturally collapses in the future state where cli delegates to unity-builder.Sequencing with the open fix PRs
This PR is built off
origin/mainand is independent of the two remaining fix PRs (game-ci/unity-builder#838, game-ci/cli#50). Either PR can land first; merge order:No rebase or follow-up commit needed since #24 is closed and never lands its orchestrator-side toolset additions.
Test plan
yarn buildclean (tsc passes)yarn test:ci— 891 passed, 1 skipped, 0 failedcommands.test.tswas inverted (asserts the engine-specific flags are now undefined, with a comment explaining why)coreParamsopaque pass-through still delivers Unity license info to the container (cannot do locally — needs a reviewer with a working dispatch flow to confirm)🤖 Generated with Claude Code