docs: orchestrator documentation — providers, build services, cloud#1
Closed
frostebite wants to merge 2 commits intomainfrom
Closed
docs: orchestrator documentation — providers, build services, cloud#1frostebite wants to merge 2 commits intomainfrom
frostebite wants to merge 2 commits intomainfrom
Conversation
Consolidates documentation from individual unity-builder PRs into the standalone orchestrator repository. Updated to reflect the standalone package architecture with both Action and CLI usage examples. Consolidates: - game-ci/unity-builder#780 (CLI provider protocol and provider plugin system) - game-ci/unity-builder#781 (submodule profiles, local caching, LFS agents, git hooks) - game-ci/unity-builder#782 (GCP Cloud Run and Azure ACI with multi-storage backends) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds three new documentation pages for build services, cloud providers, and provider plugins, and exposes five lazy-loaded enterprise service exports from the public API surface. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/build-services.md`:
- Around line 61-76: Docs reference a localCacheRoot input but the input model
(src/model/build-parameters.ts, the BuildParameters interface / parseInputs
logic around lines ~78-134) does not define or parse localCacheRoot; either add
localCacheRoot to the input model or remove it from the docs. Fix by extending
the BuildParameters type and the input parsing/validation code to include a
localCacheRoot field (with the same default/precedence behavior used in docs)
and ensure CLI/Action parsing reads it, or update docs/examples to stop
mentioning localCacheRoot if intentionally unsupported; reference the
BuildParameters interface and the parseInputs (or equivalent input parsing)
function in src/model/build-parameters.ts when implementing the change.
- Around line 103-108: The fenced code block in docs/build-services.md that
shows the git config commands (the lines starting with "git config
lfs.customtransfer.{name}.path", "git config lfs.customtransfer.{name}.args",
and "git config lfs.standalonetransferagent {name}") needs a language tag;
update the opening triple-backtick fence to ```bash so the block is explicitly
tagged as bash for linting and syntax highlighting.
In `@docs/cloud-providers.md`:
- Around line 41-50: The docs entry for gcpDiskSizeGb contradicts itself
(default 100 vs description "in-memory volume size (for in-memory, max 32)");
fix it by aligning the table with the actual validation used by the GCP Cloud
Run provider: examine the gcp-cloud-run provider code path that validates
gcpDiskSizeGb (the function/constant in that provider which enforces the max 32
in-memory rule) and either set the "Default" column to a valid default within
that range (e.g., 32) or update the description to reflect that larger defaults
are accepted if the provider allows 100; ensure the scope ("in-memory") and the
valid range match the provider validation and update the table cell for
gcpDiskSizeGb accordingly.
In `@docs/provider-plugins.md`:
- Around line 25-32: The fenced code block under the "Protocol" header is
missing a language tag which triggers docs lint MD040; update that block (the
triple-backtick fence containing "Invocation: <executable> <subcommand>" etc.)
to include a language tag such as text (i.e., change ``` to ```text) so the
linter accepts the non-executable snippet.
- Around line 147-160: Update the docs so the ProviderInterface section matches
the real contract: state that the TypeScript ProviderInterface applies to
dynamic TS providers (CLI providers expose equivalent subcommands instead), and
update the method signatures to use the real parameter name defaultSecretsArray
for setupWorkflow and cleanupWorkflow (referencing ProviderInterface,
setupWorkflow, cleanupWorkflow, defaultSecretsArray); ensure runTaskInWorkflow
and other method names remain unchanged and the doc text clarifies the CLI vs TS
provider shape difference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f90cc4d-8df9-4183-bd8f-76550db37115
📒 Files selected for processing (3)
docs/build-services.mddocs/cloud-providers.mddocs/provider-plugins.md
Comment on lines
+61
to
+76
| ### How it works | ||
|
|
||
| - Cache key: `{platform}-{version}-{branch}` (sanitized) | ||
| - Cache root: `localCacheRoot` > `$RUNNER_TEMP/game-ci-cache` > `.game-ci/cache` | ||
| - On restore: extracts `library-{key}.tar` / `lfs-{key}.tar` if they exist | ||
| - On save: creates tar archives of the Library and LFS folders | ||
| - Garbage collection removes cache entries that haven't been accessed recently | ||
|
|
||
| ### Inputs | ||
|
|
||
| | Input | Default | Description | | ||
| |-------|---------|-------------| | ||
| | `localCacheEnabled` | `false` | Enable filesystem caching | | ||
| | `localCacheRoot` | — | Cache directory override | | ||
| | `localCacheLibrary` | `true` | Cache Unity Library folder | | ||
| | `localCacheLfs` | `true` | Cache LFS objects | |
There was a problem hiding this comment.
localCacheRoot looks unsupported by the current inputs model.
This section documents localCacheRoot as an accepted input and even makes it part of the cache-root precedence, but src/model/build-parameters.ts:78-134 only shows localCacheEnabled, localCacheLibrary, and localCacheLfs for local caching. If localCacheRoot is not parsed anywhere else, the table and the CLI/Action examples below are misleading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/build-services.md` around lines 61 - 76, Docs reference a localCacheRoot
input but the input model (src/model/build-parameters.ts, the BuildParameters
interface / parseInputs logic around lines ~78-134) does not define or parse
localCacheRoot; either add localCacheRoot to the input model or remove it from
the docs. Fix by extending the BuildParameters type and the input
parsing/validation code to include a localCacheRoot field (with the same
default/precedence behavior used in docs) and ensure CLI/Action parsing reads
it, or update docs/examples to stop mentioning localCacheRoot if intentionally
unsupported; reference the BuildParameters interface and the parseInputs (or
equivalent input parsing) function in src/model/build-parameters.ts when
implementing the change.
Comment on lines
+103
to
+108
| Configures git to use a custom transfer agent via: | ||
| ``` | ||
| git config lfs.customtransfer.{name}.path <executable> | ||
| git config lfs.customtransfer.{name}.args <args> | ||
| git config lfs.standalonetransferagent {name} | ||
| ``` |
There was a problem hiding this comment.
Tag this fenced block as bash.
It is currently an unlabeled code fence, which is a docs-lint failure waiting to happen.
📝 Suggested doc fix
-```
+```bash
git config lfs.customtransfer.{name}.path <executable>
git config lfs.customtransfer.{name}.args <args>
git config lfs.standalonetransferagent {name}</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
Configures git to use a custom transfer agent via:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/build-services.md` around lines 103 - 108, The fenced code block in
docs/build-services.md that shows the git config commands (the lines starting
with "git config lfs.customtransfer.{name}.path", "git config
lfs.customtransfer.{name}.args", and "git config lfs.standalonetransferagent
{name}") needs a language tag; update the opening triple-backtick fence to
```bash so the block is explicitly tagged as bash for linting and syntax
highlighting.
Comment on lines
+41
to
+50
| | Input | Default | Description | | ||
| |-------|---------|-------------| | ||
| | `gcpProject` | `$GOOGLE_CLOUD_PROJECT` | GCP project ID | | ||
| | `gcpRegion` | `us-central1` | Cloud Run region | | ||
| | `gcpStorageType` | `gcs-fuse` | Storage backend (see above) | | ||
| | `gcpBucket` | — | GCS bucket name (for gcs-fuse, gcs-copy) | | ||
| | `gcpFilestoreIp` | — | Filestore IP address (for nfs) | | ||
| | `gcpFilestoreShare` | `/share1` | Filestore share name (for nfs) | | ||
| | `gcpMachineType` | `e2-standard-4` | Machine type | | ||
| | `gcpDiskSizeGb` | `100` | In-memory volume size (for in-memory, max 32) | |
There was a problem hiding this comment.
Clarify gcpDiskSizeGb; the default and description contradict each other.
The table says the default is 100, but the description says this is the in-memory volume size with a max of 32. Those two statements cannot both be true. Please align the description, scope, and valid range with src/model/orchestrator/providers/gcp-cloud-run/index.ts:53-65 before merging, otherwise users do not know what values are actually valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cloud-providers.md` around lines 41 - 50, The docs entry for
gcpDiskSizeGb contradicts itself (default 100 vs description "in-memory volume
size (for in-memory, max 32)"); fix it by aligning the table with the actual
validation used by the GCP Cloud Run provider: examine the gcp-cloud-run
provider code path that validates gcpDiskSizeGb (the function/constant in that
provider which enforces the max 32 in-memory rule) and either set the "Default"
column to a valid default within that range (e.g., 32) or update the description
to reflect that larger defaults are accepted if the provider allows 100; ensure
the scope ("in-memory") and the valid range match the provider validation and
update the table cell for gcpDiskSizeGb accordingly.
Comment on lines
+25
to
+32
| ### Protocol | ||
|
|
||
| ``` | ||
| Invocation: <executable> <subcommand> | ||
| Input: JSON on stdin | ||
| Output: JSON on stdout | ||
| Errors: stderr forwarded to orchestrator logs | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
This trips MD040 in docs lint. Use text here if you want to keep it non-executable.
📝 Suggested doc fix
-```
+```text
Invocation: <executable> <subcommand>
Input: JSON on stdin
Output: JSON on stdout
Errors: stderr forwarded to orchestrator logs</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
### Protocol
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/provider-plugins.md` around lines 25 - 32, The fenced code block under
the "Protocol" header is missing a language tag which triggers docs lint MD040;
update that block (the triple-backtick fence containing "Invocation:
<executable> <subcommand>" etc.) to include a language tag such as text (i.e.,
change ``` to ```text) so the linter accepts the non-executable snippet.
Comment on lines
+147
to
+160
| ## Provider Interface | ||
|
|
||
| All providers (built-in, CLI, or dynamic) implement the same 7-method interface: | ||
|
|
||
| ```typescript | ||
| interface ProviderInterface { | ||
| setupWorkflow(buildGuid, buildParameters, branchName, secrets): any; | ||
| runTaskInWorkflow(buildGuid, image, commands, mountdir, workingdir, environment, secrets): Promise<string>; | ||
| cleanupWorkflow(buildParameters, branchName, secrets): any; | ||
| garbageCollect(filter, previewOnly, olderThan, fullCache, baseDependencies): Promise<string>; | ||
| listResources(): Promise<ProviderResource[]>; | ||
| listWorkflow(): Promise<ProviderWorkflow[]>; | ||
| watchWorkflow(): Promise<string>; | ||
| } |
There was a problem hiding this comment.
Make this interface section match the real contract.
This section currently implies that CLI providers implement the TypeScript ProviderInterface directly, and the sample signatures for setupWorkflow / cleanupWorkflow do not match src/model/orchestrator/providers/provider-interface.ts:7-32 (defaultSecretsArray is part of the real contract). Anyone copying this into a TS provider will get the wrong shape. Tighten this to dynamic providers, or explicitly say CLI providers expose equivalent subcommands instead of the TS interface.
📝 Suggested doc fix
-## Provider Interface
-
-All providers (built-in, CLI, or dynamic) implement the same 7-method interface:
+## Provider Interface
+
+Dynamic providers loaded from a package/path must export a class that implements the TypeScript `ProviderInterface`. CLI providers expose equivalent operations through the CLI subcommands above.
```typescript
interface ProviderInterface {
- setupWorkflow(buildGuid, buildParameters, branchName, secrets): any;
+ setupWorkflow(buildGuid, buildParameters, branchName, defaultSecretsArray): any;
runTaskInWorkflow(buildGuid, image, commands, mountdir, workingdir, environment, secrets): Promise<string>;
- cleanupWorkflow(buildParameters, branchName, secrets): any;
+ cleanupWorkflow(buildParameters, branchName, defaultSecretsArray): any;
garbageCollect(filter, previewOnly, olderThan, fullCache, baseDependencies): Promise<string>;
listResources(): Promise<ProviderResource[]>;
listWorkflow(): Promise<ProviderWorkflow[]>;
watchWorkflow(): Promise<string>;
}</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
## Provider Interface
Dynamic providers loaded from a package/path must export a class that implements the TypeScript `ProviderInterface`. CLI providers expose equivalent operations through the CLI subcommands above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/provider-plugins.md` around lines 147 - 160, Update the docs so the
ProviderInterface section matches the real contract: state that the TypeScript
ProviderInterface applies to dynamic TS providers (CLI providers expose
equivalent subcommands instead), and update the method signatures to use the
real parameter name defaultSecretsArray for setupWorkflow and cleanupWorkflow
(referencing ProviderInterface, setupWorkflow, cleanupWorkflow,
defaultSecretsArray); ensure runTaskInWorkflow and other method names remain
unchanged and the doc text clarifies the CLI vs TS provider shape difference.
Adds missing exports for ChildWorkspaceService, LocalCacheService, SubmoduleProfileService, LfsAgentService, and GitHooksService. These are required by unity-builder's plugin interface (loadEnterpriseServices). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Closing — documentation should go in game-ci/documentation repo instead, not a docs folder here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combined documentation for the standalone
@game-ci/orchestratorpackage, updated to reflect the new architecture where orchestrator is a separate package loaded via unity-builder's plugin interface.All docs updated to show both GitHub Action usage (via unity-builder) and standalone CLI usage.
Consolidates
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit