feat(orchestrator): GCP Cloud Run and Azure ACI providers#778
Closed
frostebite wants to merge 5 commits intomainfrom
Closed
feat(orchestrator): GCP Cloud Run and Azure ACI providers#778frostebite wants to merge 5 commits intomainfrom
frostebite wants to merge 5 commits intomainfrom
Conversation
…iders Add two new cloud provider implementations for the orchestrator, both marked as experimental: - **GCP Cloud Run Jobs** (`providerStrategy: gcp-cloud-run`): Executes Unity builds as Cloud Run Jobs with GCS FUSE for large artifact storage. Supports configurable machine types, service accounts, and VPC connectors. 7 new inputs (gcpProject, gcpRegion, gcpBucket, gcpMachineType, gcpDiskSizeGb, gcpServiceAccount, gcpVpcConnector). - **Azure Container Instances** (`providerStrategy: azure-aci`): Executes Unity builds as ACI containers with Azure File Shares (Premium FileStorage) for large artifact storage up to 100 TiB. Supports configurable CPU/memory, VNet integration, and subscription targeting. 9 new inputs (azureResourceGroup, azureLocation, azureStorageAccount, azureFileShareName, azureSubscriptionId, azureCpu, azureMemoryGb, azureDiskSizeGb, azureSubnetId). Both providers use their respective CLIs (gcloud, az) for infrastructure management and support garbage collection of old build resources. No tests included as these require real cloud infrastructure to validate. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Warning Rate limit exceeded
⌛ 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: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds experimental GCP Cloud Run and Azure ACI providers: new action inputs, Input getters, BuildParameters fields, provider registration, and two provider implementations with orchestration, execution, logging, and garbage-collection logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator
participant Provider as GCP_CloudRun
participant GCloudCLI as gcloud
participant GCS as GCS_Bucket
Orchestrator->>Provider: setupWorkflow(buildGuid, params, ...)
Provider->>GCloudCLI: verify gcloud & APIs, create bucket if needed
Provider->>GCS: ensure bucket / prepare storage
Orchestrator->>Provider: runTaskInWorkflow(image, commands, mounts, env, secrets)
Provider->>GCloudCLI: create/update Cloud Run Job, submit job
GCloudCLI->>Provider: job started / jobName
Provider->>GCloudCLI: streamJobLogs(jobName)
GCloudCLI->>Provider: log lines / status
Provider->>Orchestrator: return final logs / status
sequenceDiagram
participant Orchestrator
participant Provider as Azure_ACI
participant AzCLI as az
participant Storage as Azure_Storage
Orchestrator->>Provider: setupWorkflow(buildGuid, params, ...)
Provider->>AzCLI: verify az CLI, ensure resource group
Provider->>Storage: create storage account / file share / container as needed
Orchestrator->>Provider: runTaskInWorkflow(image, commands, mounts, env, secrets)
Provider->>AzCLI: create container group with volumes/subnet/service details
AzCLI->>Provider: container started (name)
Provider->>AzCLI: poll logs/state until completion
AzCLI->>Provider: logs / final state
Provider->>Orchestrator: return final logs / status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
|
|
Both providers now support four storage backends via gcpStorageType / azureStorageType: GCP Cloud Run: - gcs-fuse: Mount GCS bucket as POSIX filesystem (unlimited, best for large sequential I/O) - gcs-copy: Copy artifacts in/out via gsutil (simpler, no FUSE overhead) - nfs: Filestore NFS mount (true POSIX, good random I/O, up to 100 TiB) - in-memory: tmpfs (fastest, volatile, up to 32 GiB) Azure ACI: - azure-files: SMB file share mount (up to 100 TiB, premium throughput) - blob-copy: Copy artifacts in/out via az storage blob (no mount overhead) - azure-files-nfs: NFS 4.1 file share mount (true POSIX, no SMB lock overhead) - in-memory: emptyDir tmpfs (fastest, volatile, limited by container memory) New inputs: gcpStorageType, gcpFilestoreIp, gcpFilestoreShare, azureStorageType, azureBlobContainer. Constructor validates storage config and warns on missing prerequisites (e.g. NFS requires VPC connector/subnet). Co-Authored-By: Claude Opus 4.6 <[email protected]>
frostebite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/providers/azure-aci/index.ts`:
- Around line 64-68: The code currently only warns when this.resourceGroup is
empty (OrchestratorLogger.logWarning) but later always injects
`--resource-group` with an empty value into the Azure CLI command (causing
runtime failure); update the setup to fail fast by validating
`this.resourceGroup` and throwing or returning an error when it's missing
(replace the warning with a throw or call to OrchestratorLogger.logError and
abort) and update the command-construction logic that adds `--resource-group`
(the code that builds the az container create/run command) to only include the
`--resource-group` flag when `this.resourceGroup` is non-empty so no empty
`--resource-group ""` is passed to the CLI.
- Around line 153-159: runTaskInWorkflow ignores the workingdir parameter when
constructing the container command; update the command builder inside
runTaskInWorkflow to run from the provided workingdir by either setting the
container's WorkingDir property in the ACI container definition or prefixing the
commands with a safe "cd <workingdir> &&" (ensure proper shell escaping), and
apply the same change to the other command-construction site referenced around
lines 203-204 so both places honor the workingdir argument.
- Around line 71-76: Several provider methods (including setupWorkflow) declare
parameters required by the interface but not used, causing no-unused-vars ESLint
failures; fix by explicitly marking those parameters as used—either rename each
unused parameter to start with an underscore (e.g., buildGuid -> _buildGuid) or
add a single-line usage like void buildGuid for each unused parameter in the
method body. Apply this to setupWorkflow and the other affected provider methods
referenced in the comment so the interface is satisfied without lint errors.
- Around line 282-287: The current completion check treats provisioningState ===
'Succeeded' as finished even when the container hasn't reported a terminal exit
code; update the logic in the block that inspects
containerState/provisioningState (where
state.containers?.[0]?.instanceView?.currentState and exitCode are read) to only
consider the run completed when either the container's currentState.state ===
'Terminated' or an explicit exitCode is present (exitCode !== undefined); if
neither is true, do not log success or throw based on provisioningState and
continue waiting/polling. Ensure the error path still throws when exitCode is
defined and non‑zero, and only log `[Azure ACI] Container completed
successfully` after confirming termination/exitCode.
- Around line 356-361: The createdAt parsing and deletion condition are wrong:
stop using container.properties?.provisioningState as a timestamp and only
delete containers that are both terminated and older than cutoffDate; update the
createdAt calculation to parse container.tags?.createdAt (fallback to 0) and
change the if check from "if (state === 'Terminated' || createdAt < cutoffDate)"
to require both state === 'Terminated' and createdAt < cutoffDate (i.e., "state
=== 'Terminated' && createdAt < cutoffDate") so only terminated containers older
than cutoffDate are garbage-collected.
- Line 48: The current fallback chain makes the literal 'eastus' unreachable
because Input.region is always non-empty; change the assignment of this.location
to prefer buildParameters.azureLocation, but only use Input.region when it is
not the framework default/empty, otherwise fall back to 'eastus' — update the
expression that sets this.location (currently using
buildParameters.azureLocation || Input.region || 'eastus') to explicitly check
Input.region for a non-default value before using it so 'eastus' can actually be
selected when no real region is provided.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts`:
- Around line 113-119: The runTaskInWorkflow function accepts a workingdir but
never applies it when overriding the container command; update runTaskInWorkflow
(and the similar block at the other occurrence) so the executed command uses the
provided workingdir — either by setting the container's workingDir/workDir field
if supported (container.workdir or workingDir) or by prefixing the commands with
a safe cd (e.g., "cd <workingdir> && <commands>") before passing them to the
command override; locate the override that currently only passes commands and
modify it to incorporate workingdir using the function name runTaskInWorkflow
and the matching second occurrence mentioned in the comment.
- Around line 65-70: The setupWorkflow method (and other provider methods
flagged by ESLint) declare parameters/local variables that are never used,
causing no-unused-vars errors; fix by either removing truly unnecessary
parameters or renaming unused parameters to start with an underscore (e.g.,
buildGuid -> _buildGuid, defaultSecretsArray -> _defaultSecretsArray) or
reference them in a no-op way (e.g., void _buildGuid) so ESLint recognizes them
as intentionally unused; apply the same change consistently to the other
provider methods mentioned in the review so lint passes.
- Around line 47-48: The code captures this.machineType and this.diskSizeGb but
still hardcodes '--cpu=4' and '--memory=16Gi' when building the Cloud Run job
flags; update the job creation logic (the code that currently injects '--cpu=4'
and '--memory=16Gi') to derive cpu and memory from this.machineType (e.g., parse
'e2-standard-4' to cpu=4 and memory=16Gi or map known machine types) and replace
the hardcoded flags with '--cpu=<derivedCpu>' and '--memory=<derivedMemory>';
also replace/augment the disk-related flag(s) to use this.diskSizeGb (e.g.,
'--disk-size=<this.diskSizeGb>GB' or the Cloud Run equivalent) and apply the
same change to the second occurrence noted in the file so user-provided
gcpMachineType and gcpDiskSizeGb actually affect execution.
- Line 45: The current assignment this.region = buildParameters.gcpRegion ||
Input.region || 'us-central1' makes the 'us-central1' fallback unreachable
because Input.region already has a default; change the expression to use nullish
coalescing so the explicit fallback only applies if both are null/undefined,
e.g. set this.region = buildParameters.gcpRegion ?? Input.region ??
'us-central1', updating the assignment in the constructor (referencing
this.region, buildParameters.gcpRegion and Input.region) so the final
'us-central1' fallback can be reached when neither source provides a value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9001c05-db57-4f4e-a4b1-a7d9130ea519
📒 Files selected for processing (7)
action.ymlsrc/model/build-parameters.tssrc/model/input.tssrc/model/orchestrator/orchestrator.tssrc/model/orchestrator/providers/azure-aci/index.tssrc/model/orchestrator/providers/gcp-cloud-run/index.tssrc/model/orchestrator/providers/provider-loader.ts
| constructor(buildParameters: BuildParameters) { | ||
| this.buildParameters = buildParameters; | ||
| this.resourceGroup = buildParameters.azureResourceGroup || process.env.AZURE_RESOURCE_GROUP || ''; | ||
| this.location = buildParameters.azureLocation || Input.region || 'eastus'; |
There was a problem hiding this comment.
Azure location fallback chain makes the 'eastus' default unreachable.
Line 48 uses buildParameters.azureLocation || Input.region || 'eastus', but Input.region is always non-empty by default, so 'eastus' is never used.
Proposed fix
- this.location = buildParameters.azureLocation || Input.region || 'eastus';
+ this.location = buildParameters.azureLocation || Input.getInput('region') || 'eastus';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` at line 48, The current
fallback chain makes the literal 'eastus' unreachable because Input.region is
always non-empty; change the assignment of this.location to prefer
buildParameters.azureLocation, but only use Input.region when it is not the
framework default/empty, otherwise fall back to 'eastus' — update the expression
that sets this.location (currently using buildParameters.azureLocation ||
Input.region || 'eastus') to explicitly check Input.region for a non-default
value before using it so 'eastus' can actually be selected when no real region
is provided.
Comment on lines
+64
to
+68
| if (!this.resourceGroup) { | ||
| OrchestratorLogger.logWarning( | ||
| '[Azure ACI] No resource group specified. Set azureResourceGroup input or AZURE_RESOURCE_GROUP env var.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing azureResourceGroup is only warned, then fails later at runtime.
At Line 64-68 this is just a warning, but Line 208 always passes --resource-group, including an empty string. Fail fast during setup to avoid opaque failures.
Proposed fix
async setupWorkflow(
buildGuid: string,
buildParameters: BuildParameters,
branchName: string,
defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[],
) {
+ if (!this.resourceGroup) {
+ throw new Error('[Azure ACI] azureResourceGroup is required for providerStrategy=azure-aci');
+ }
+
OrchestratorLogger.log(`[Azure ACI] Setting up workflow for build ${buildGuid}`);Also applies to: 206-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 64 - 68,
The code currently only warns when this.resourceGroup is empty
(OrchestratorLogger.logWarning) but later always injects `--resource-group` with
an empty value into the Azure CLI command (causing runtime failure); update the
setup to fail fast by validating `this.resourceGroup` and throwing or returning
an error when it's missing (replace the warning with a throw or call to
OrchestratorLogger.logError and abort) and update the command-construction logic
that adds `--resource-group` (the code that builds the az container create/run
command) to only include the `--resource-group` flag when `this.resourceGroup`
is non-empty so no empty `--resource-group ""` is passed to the CLI.
Comment on lines
+71
to
+76
| async setupWorkflow( | ||
| buildGuid: string, | ||
| buildParameters: BuildParameters, | ||
| branchName: string, | ||
| defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], | ||
| ) { |
There was a problem hiding this comment.
ESLint blockers: unused parameters/locals in provider methods.
no-unused-vars errors at these signatures/locals will fail linting. The interface requires the parameters, but they still need explicit handling in implementation.
Proposed fix
async setupWorkflow(
buildGuid: string,
buildParameters: BuildParameters,
branchName: string,
defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[],
) {
+ void buildParameters;
+ void branchName;
+ void defaultSecretsArray;
OrchestratorLogger.log(`[Azure ACI] Setting up workflow for build ${buildGuid}`);
ResourceTracking.logAllocationSummary('azure-aci setup');
@@
- const keyJson = await OrchestratorSystem.Run(
- `az storage account keys list --account-name "${this.storageAccount}" --resource-group "${this.resourceGroup}" --output json`,
- false,
- true,
- );
- const keys = JSON.parse(keyJson);
- const storageKey = keys[0]?.value || '';
+ await OrchestratorSystem.Run(
+ `az storage account keys list --account-name "${this.storageAccount}" --resource-group "${this.resourceGroup}" --output json`,
+ false,
+ true,
+ );
@@
async cleanupWorkflow(
buildParameters: BuildParameters,
branchName: string,
defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[],
) {
+ void buildParameters;
+ void branchName;
+ void defaultSecretsArray;
OrchestratorLogger.log(`[Azure ACI] Cleaning up workflow`);
@@
async garbageCollect(
filter: string,
previewOnly: boolean,
olderThan: Number,
fullCache: boolean,
baseDependencies: boolean,
): Promise<string> {
+ void filter;
+ void fullCache;
+ void baseDependencies;
OrchestratorLogger.log(`[Azure ACI] Garbage collecting old container groups`);Also applies to: 129-136, 323-336
🧰 Tools
🪛 ESLint
[error] 73-73: 'buildParameters' is defined but never used.
(no-unused-vars)
[error] 74-74: 'branchName' is defined but never used.
(no-unused-vars)
[error] 75-75: 'defaultSecretsArray' is defined but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 71 - 76,
Several provider methods (including setupWorkflow) declare parameters required
by the interface but not used, causing no-unused-vars ESLint failures; fix by
explicitly marking those parameters as used—either rename each unused parameter
to start with an underscore (e.g., buildGuid -> _buildGuid) or add a single-line
usage like void buildGuid for each unused parameter in the method body. Apply
this to setupWorkflow and the other affected provider methods referenced in the
comment so the interface is satisfied without lint errors.
Comment on lines
+153
to
+159
| async runTaskInWorkflow( | ||
| buildGuid: string, | ||
| image: string, | ||
| commands: string, | ||
| mountdir: string, | ||
| workingdir: string, | ||
| environment: OrchestratorEnvironmentVariable[], |
There was a problem hiding this comment.
workingdir is ignored when building the container command.
The method receives workingdir but the command only runs commands. This can execute scripts from the wrong directory.
Proposed fix
- const commandFlag = commands ? `--command-line "/bin/sh -c '${commands.replace(/'/g, "'\\''")}'\"` : '';
+ const commandToRun = commands
+ ? `cd '${workingdir.replace(/'/g, "'\\''")}' && ${commands}`
+ : '';
+ const commandFlag = commandToRun
+ ? `--command-line "/bin/sh -c '${commandToRun.replace(/'/g, "'\\''")}'"`
+ : '';Also applies to: 203-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 153 - 159,
runTaskInWorkflow ignores the workingdir parameter when constructing the
container command; update the command builder inside runTaskInWorkflow to run
from the provided workingdir by either setting the container's WorkingDir
property in the ACI container definition or prefixing the commands with a safe
"cd <workingdir> &&" (ensure proper shell escaping), and apply the same change
to the other command-construction site referenced around lines 203-204 so both
places honor the workingdir argument.
Comment on lines
+282
to
+287
| if (containerState === 'Terminated' || provisioningState === 'Succeeded') { | ||
| const exitCode = state.containers?.[0]?.instanceView?.currentState?.exitCode; | ||
| if (exitCode !== undefined && exitCode !== 0) { | ||
| throw new Error(`[Azure ACI] Container exited with code ${exitCode}`); | ||
| } | ||
| OrchestratorLogger.log(`[Azure ACI] Container completed successfully`); |
There was a problem hiding this comment.
Completion check can report success before the container has exited.
Line 282 treats provisioningState === 'Succeeded' as completion even if no terminal exit code exists yet. This can return early with incomplete/incorrect success.
Proposed fix
- if (containerState === 'Terminated' || provisioningState === 'Succeeded') {
+ if (containerState === 'Terminated') {
const exitCode = state.containers?.[0]?.instanceView?.currentState?.exitCode;
if (exitCode !== undefined && exitCode !== 0) {
throw new Error(`[Azure ACI] Container exited with code ${exitCode}`);
}
OrchestratorLogger.log(`[Azure ACI] Container completed successfully`);📝 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.
Suggested change
| if (containerState === 'Terminated' || provisioningState === 'Succeeded') { | |
| const exitCode = state.containers?.[0]?.instanceView?.currentState?.exitCode; | |
| if (exitCode !== undefined && exitCode !== 0) { | |
| throw new Error(`[Azure ACI] Container exited with code ${exitCode}`); | |
| } | |
| OrchestratorLogger.log(`[Azure ACI] Container completed successfully`); | |
| if (containerState === 'Terminated') { | |
| const exitCode = state.containers?.[0]?.instanceView?.currentState?.exitCode; | |
| if (exitCode !== undefined && exitCode !== 0) { | |
| throw new Error(`[Azure ACI] Container exited with code ${exitCode}`); | |
| } | |
| OrchestratorLogger.log(`[Azure ACI] Container completed successfully`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 282 - 287,
The current completion check treats provisioningState === 'Succeeded' as
finished even when the container hasn't reported a terminal exit code; update
the logic in the block that inspects containerState/provisioningState (where
state.containers?.[0]?.instanceView?.currentState and exitCode are read) to only
consider the run completed when either the container's currentState.state ===
'Terminated' or an explicit exitCode is present (exitCode !== undefined); if
neither is true, do not log success or throw based on provisioningState and
continue waiting/polling. Ensure the error path still throws when exitCode is
defined and non‑zero, and only log `[Azure ACI] Container completed
successfully` after confirming termination/exitCode.
Comment on lines
+356
to
+361
| const createdAt = new Date(container.tags?.createdAt || container.properties?.provisioningState || 0); | ||
| const state = container.containers?.[0]?.instanceView?.currentState?.state || ''; | ||
|
|
||
| // Delete terminated containers older than the threshold | ||
| if (state === 'Terminated' || createdAt < cutoffDate) { | ||
| if (previewOnly) { |
There was a problem hiding this comment.
Garbage-collection age filtering is currently incorrect.
Line 356 parses provisioningState as a date, and Line 360 uses state === 'Terminated' || createdAt < cutoffDate, which deletes all terminated containers regardless of age.
Proposed fix
- const createdAt = new Date(container.tags?.createdAt || container.properties?.provisioningState || 0);
+ const createdAtRaw = container.tags?.createdAt;
+ const createdAt = createdAtRaw ? new Date(createdAtRaw) : undefined;
+ const isOlderThanCutoff = createdAt ? createdAt < cutoffDate : false;
const state = container.containers?.[0]?.instanceView?.currentState?.state || '';
@@
- if (state === 'Terminated' || createdAt < cutoffDate) {
+ if (state === 'Terminated' && isOlderThanCutoff) {📝 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.
Suggested change
| const createdAt = new Date(container.tags?.createdAt || container.properties?.provisioningState || 0); | |
| const state = container.containers?.[0]?.instanceView?.currentState?.state || ''; | |
| // Delete terminated containers older than the threshold | |
| if (state === 'Terminated' || createdAt < cutoffDate) { | |
| if (previewOnly) { | |
| const createdAtRaw = container.tags?.createdAt; | |
| const createdAt = createdAtRaw ? new Date(createdAtRaw) : undefined; | |
| const isOlderThanCutoff = createdAt ? createdAt < cutoffDate : false; | |
| const state = container.containers?.[0]?.instanceView?.currentState?.state || ''; | |
| // Delete terminated containers older than the threshold | |
| if (state === 'Terminated' && isOlderThanCutoff) { | |
| if (previewOnly) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 356 - 361,
The createdAt parsing and deletion condition are wrong: stop using
container.properties?.provisioningState as a timestamp and only delete
containers that are both terminated and older than cutoffDate; update the
createdAt calculation to parse container.tags?.createdAt (fallback to 0) and
change the if check from "if (state === 'Terminated' || createdAt < cutoffDate)"
to require both state === 'Terminated' and createdAt < cutoffDate (i.e., "state
=== 'Terminated' && createdAt < cutoffDate") so only terminated containers older
than cutoffDate are garbage-collected.
| constructor(buildParameters: BuildParameters) { | ||
| this.buildParameters = buildParameters; | ||
| this.project = buildParameters.gcpProject || process.env.GOOGLE_CLOUD_PROJECT || process.env.GCLOUD_PROJECT || ''; | ||
| this.region = buildParameters.gcpRegion || Input.region || 'us-central1'; |
There was a problem hiding this comment.
Region fallback logic makes 'us-central1' unreachable.
Line 45 uses buildParameters.gcpRegion || Input.region || 'us-central1'. Since Input.region has a default, the explicit 'us-central1' fallback is effectively dead.
Proposed fix
- this.region = buildParameters.gcpRegion || Input.region || 'us-central1';
+ this.region = buildParameters.gcpRegion || Input.getInput('region') || 'us-central1';📝 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.
Suggested change
| this.region = buildParameters.gcpRegion || Input.region || 'us-central1'; | |
| this.region = buildParameters.gcpRegion || Input.getInput('region') || 'us-central1'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` at line 45, The
current assignment this.region = buildParameters.gcpRegion || Input.region ||
'us-central1' makes the 'us-central1' fallback unreachable because Input.region
already has a default; change the expression to use nullish coalescing so the
explicit fallback only applies if both are null/undefined, e.g. set this.region
= buildParameters.gcpRegion ?? Input.region ?? 'us-central1', updating the
assignment in the constructor (referencing this.region,
buildParameters.gcpRegion and Input.region) so the final 'us-central1' fallback
can be reached when neither source provides a value.
Comment on lines
+47
to
+48
| this.machineType = buildParameters.gcpMachineType || 'e2-standard-4'; | ||
| this.diskSizeGb = Number.parseInt(buildParameters.gcpDiskSizeGb || '100', 10); |
There was a problem hiding this comment.
Configured GCP sizing inputs are currently ignored at execution time.
gcpMachineType and gcpDiskSizeGb are captured, but job creation still hardcodes --cpu=4 and --memory=16Gi. User input has no effect here.
Also applies to: 157-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 47 -
48, The code captures this.machineType and this.diskSizeGb but still hardcodes
'--cpu=4' and '--memory=16Gi' when building the Cloud Run job flags; update the
job creation logic (the code that currently injects '--cpu=4' and
'--memory=16Gi') to derive cpu and memory from this.machineType (e.g., parse
'e2-standard-4' to cpu=4 and memory=16Gi or map known machine types) and replace
the hardcoded flags with '--cpu=<derivedCpu>' and '--memory=<derivedMemory>';
also replace/augment the disk-related flag(s) to use this.diskSizeGb (e.g.,
'--disk-size=<this.diskSizeGb>GB' or the Cloud Run equivalent) and apply the
same change to the second occurrence noted in the file so user-provided
gcpMachineType and gcpDiskSizeGb actually affect execution.
Comment on lines
+65
to
+70
| async setupWorkflow( | ||
| buildGuid: string, | ||
| buildParameters: BuildParameters, | ||
| branchName: string, | ||
| defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], | ||
| ) { |
There was a problem hiding this comment.
ESLint blockers: unused parameters/local variables in provider methods.
These no-unused-vars errors are actionable and should be resolved to keep lint green.
Proposed fix
async setupWorkflow(
buildGuid: string,
buildParameters: BuildParameters,
branchName: string,
defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[],
) {
+ void buildParameters;
+ void branchName;
+ void defaultSecretsArray;
OrchestratorLogger.log(`[GCP Cloud Run] Setting up workflow for build ${buildGuid}`);
@@
- const version = await OrchestratorSystem.Run('gcloud --version', false, true);
+ await OrchestratorSystem.Run('gcloud --version', false, true);
@@
async cleanupWorkflow(
buildParameters: BuildParameters,
branchName: string,
defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[],
) {
+ void buildParameters;
+ void branchName;
+ void defaultSecretsArray;
OrchestratorLogger.log(`[GCP Cloud Run] Cleaning up workflow`);
@@
async garbageCollect(
filter: string,
previewOnly: boolean,
olderThan: Number,
fullCache: boolean,
baseDependencies: boolean,
): Promise<string> {
+ void filter;
+ void fullCache;
+ void baseDependencies;
OrchestratorLogger.log(`[GCP Cloud Run] Garbage collecting old jobs`);Also applies to: 76-76, 252-267
🧰 Tools
🪛 ESLint
[error] 67-67: 'buildParameters' is defined but never used.
(no-unused-vars)
[error] 68-68: 'branchName' is defined but never used.
(no-unused-vars)
[error] 69-69: 'defaultSecretsArray' is defined but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 65 -
70, The setupWorkflow method (and other provider methods flagged by ESLint)
declare parameters/local variables that are never used, causing no-unused-vars
errors; fix by either removing truly unnecessary parameters or renaming unused
parameters to start with an underscore (e.g., buildGuid -> _buildGuid,
defaultSecretsArray -> _defaultSecretsArray) or reference them in a no-op way
(e.g., void _buildGuid) so ESLint recognizes them as intentionally unused; apply
the same change consistently to the other provider methods mentioned in the
review so lint passes.
Comment on lines
+113
to
+119
| async runTaskInWorkflow( | ||
| buildGuid: string, | ||
| image: string, | ||
| commands: string, | ||
| mountdir: string, | ||
| workingdir: string, | ||
| environment: OrchestratorEnvironmentVariable[], |
There was a problem hiding this comment.
workingdir is accepted but not applied to the executed command.
The command override only passes commands; it never changes to workingdir, which can break build steps expecting a specific cwd.
Proposed fix
if (commands) {
+ const commandWithWorkingDir = workingdir ? `cd ${workingdir} && ${commands}` : commands;
const updateCmd = [
'gcloud run jobs update',
jobName,
`--region=${this.region}`,
`--command="/bin/sh"`,
- `--args="-c,${commands}"`,
+ `--args="-c,${commandWithWorkingDir}"`,
projectFlag,
'--quiet',
]Also applies to: 186-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 113 -
119, The runTaskInWorkflow function accepts a workingdir but never applies it
when overriding the container command; update runTaskInWorkflow (and the similar
block at the other occurrence) so the executed command uses the provided
workingdir — either by setting the container's workingDir/workDir field if
supported (container.workdir or workingDir) or by prefixing the commands with a
safe cd (e.g., "cd <workingdir> && <commands>") before passing them to the
command override; locate the override that currently only passes commands and
modify it to incorporate workingdir using the function name runTaskInWorkflow
and the matching second occurrence mentioned in the comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/model/orchestrator/providers/gcp-cloud-run/index.ts (1)
423-429:listWorkflowandwatchWorkflowthrow instead of providing implementations.Same as Azure provider - these are required by
ProviderInterfacebut throw errors. Acceptable for experimental status, but ensure orchestrator code doesn't call these methods forgcp-cloud-runprovider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 423 - 429, The current methods listWorkflow and watchWorkflow in the gcp-cloud-run provider throw errors; replace them with safe no-op implementations that satisfy ProviderInterface so callers won't throw: implement listWorkflow(): Promise<ProviderWorkflow[]> { return Promise.resolve([]); } and async watchWorkflow(): Promise<string> { return Promise.resolve(''); } (use the exact method names listWorkflow and watchWorkflow), and/or add caller-side guards in orchestrator code to avoid invoking these methods for the 'gcp-cloud-run' provider (check provider id or instanceof) so orchestration paths handle experimental providers without relying on these methods.src/model/orchestrator/providers/azure-aci/index.ts (1)
544-550:listWorkflowandwatchWorkflowthrow instead of providing implementations.These are required by
ProviderInterfacebut throw errors. This is acceptable for an experimental provider, but any orchestrator code calling these methods will get runtime exceptions. Ensure the orchestrator avoids calling these methods when using theazure-aciprovider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 544 - 550, The azure-aci provider currently throws in listWorkflow() and watchWorkflow() which causes runtime errors when orchestrator code calls them; add a capability flag (e.g., supportsWorkflows = false) to the azure-aci provider implementation in index.ts and update the ProviderInterface contract to include this boolean, then change orchestrator call sites to check provider.supportsWorkflows before invoking listWorkflow() or watchWorkflow(); alternatively, if changing the orchestrator is not feasible, make listWorkflow() return an empty ProviderWorkflow[] and watchWorkflow() return an empty string/promise-resolved noop so callers get safe defaults instead of exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/model/orchestrator/providers/azure-aci/index.ts`:
- Around line 287-299: The 'azure-files-nfs' switch branch in the Azure ACI
provider (case 'azure-files-nfs' in index.ts) is invalid because ACI does not
support NFS; remove the duplicate SMB logic and replace it with explicit error
handling: in the 'azure-files-nfs' case (within the same method that calls
this.getStorageKey()), log or throw a clear error (or return a rejected Promise)
stating "NFS mounts are not supported on Azure Container Instances; use Azure
Files (SMB) or a different provider" and ensure no storage credentials are
requested (remove calls to this.getStorageKey() in that branch) so callers
receive an immediate, clear failure instead of attempting an unsupported mount.
---
Nitpick comments:
In `@src/model/orchestrator/providers/azure-aci/index.ts`:
- Around line 544-550: The azure-aci provider currently throws in listWorkflow()
and watchWorkflow() which causes runtime errors when orchestrator code calls
them; add a capability flag (e.g., supportsWorkflows = false) to the azure-aci
provider implementation in index.ts and update the ProviderInterface contract to
include this boolean, then change orchestrator call sites to check
provider.supportsWorkflows before invoking listWorkflow() or watchWorkflow();
alternatively, if changing the orchestrator is not feasible, make listWorkflow()
return an empty ProviderWorkflow[] and watchWorkflow() return an empty
string/promise-resolved noop so callers get safe defaults instead of exceptions.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts`:
- Around line 423-429: The current methods listWorkflow and watchWorkflow in the
gcp-cloud-run provider throw errors; replace them with safe no-op
implementations that satisfy ProviderInterface so callers won't throw: implement
listWorkflow(): Promise<ProviderWorkflow[]> { return Promise.resolve([]); } and
async watchWorkflow(): Promise<string> { return Promise.resolve(''); } (use the
exact method names listWorkflow and watchWorkflow), and/or add caller-side
guards in orchestrator code to avoid invoking these methods for the
'gcp-cloud-run' provider (check provider id or instanceof) so orchestration
paths handle experimental providers without relying on these methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0858b46b-330f-4477-9137-835a5e4d832d
📒 Files selected for processing (5)
action.ymlsrc/model/build-parameters.tssrc/model/input.tssrc/model/orchestrator/providers/azure-aci/index.tssrc/model/orchestrator/providers/gcp-cloud-run/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/model/input.ts
Comment on lines
+287
to
+299
| case 'azure-files-nfs': { | ||
| // ACI NFS mount uses a YAML deployment template; for CLI we use the same | ||
| // azure-file-volume flags but the share must be NFS-enabled and | ||
| // the container must be in a VNet | ||
| const storageKey = await this.getStorageKey(); | ||
| if (!storageKey) return ''; | ||
| return [ | ||
| `--azure-file-volume-account-name "${this.storageAccount}"`, | ||
| `--azure-file-volume-account-key "${storageKey}"`, | ||
| `--azure-file-volume-share-name "${this.fileShareName}"`, | ||
| `--azure-file-volume-mount-path "${mountdir}"`, | ||
| ].join(' '); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Azure Container Instances NFS file share mount authentication storage account key
💡 Result:
Azure Container Instances (ACI) can’t mount an NFS share, so there’s no “NFS mount authenticated with a storage account key” option in ACI. ACI’s built-in file-share volume support is Azure Files over SMB, and it requires the storage account name + file share name + storage account access key. [1], [2]
If you specifically need Azure Files NFS, note that NFS shares don’t provide user/key-based authentication; access is effectively controlled via network restrictions (for example, private endpoint/service endpoint rules). [3]
🏁 Script executed:
# Search for NFS-related code and tests in the repository
fd -t f "\.ts$" -x rg -l "nfs|NFS" {} \;Repository: game-ci/unity-builder
Length of output: 176
🏁 Script executed:
# Look at the surrounding code context for the buildVolumeFlags method
cat -n src/model/orchestrator/providers/azure-aci/index.ts | sed -n '270,310p'Repository: game-ci/unity-builder
Length of output: 1970
azure-files-nfs case is unsupported in Azure Container Instances.
Azure Container Instances does not support NFS file share mounts at all—only Azure Files over SMB protocol. The current implementation (lines 287-299) duplicates the SMB case and will fail at runtime when users attempt to use the azure-files-nfs storage type.
Either remove this unsupported case or replace it with proper error handling that explains why NFS is not available in ACI.
🧰 Tools
🪛 ESLint
[error] 293-298: Expected blank line before this statement.
(padding-line-between-statements)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 287 - 299,
The 'azure-files-nfs' switch branch in the Azure ACI provider (case
'azure-files-nfs' in index.ts) is invalid because ACI does not support NFS;
remove the duplicate SMB logic and replace it with explicit error handling: in
the 'azure-files-nfs' case (within the same method that calls
this.getStorageKey()), log or throw a clear error (or return a rejected Promise)
stating "NFS mounts are not supported on Azure Container Instances; use Azure
Files (SMB) or a different provider" and ensure no storage credentials are
requested (remove calls to this.getStorageKey() in that branch) so callers
receive an immediate, clear failure instead of attempting an unsupported mount.
Comment on lines
+343
to
+355
| // For blob-copy, wrap the user command with copy-in/copy-out steps | ||
| let effectiveCommands = commands; | ||
| if (this.storageType === 'blob-copy' && this.storageAccount && commands) { | ||
| effectiveCommands = [ | ||
| `az storage blob download-batch --destination "${mountdir}" --source "${this.blobContainer}" --account-name "${this.storageAccount}" 2>/dev/null || true`, | ||
| commands, | ||
| `az storage blob upload-batch --source "${mountdir}" --destination "${this.blobContainer}" --account-name "${this.storageAccount}" --overwrite`, | ||
| ].join(' && '); | ||
| } | ||
|
|
||
| const commandFlag = effectiveCommands | ||
| ? `--command-line "/bin/sh -c '${effectiveCommands.replace(/'/g, "'\\''")}'"` | ||
| : ''; |
There was a problem hiding this comment.
blob-copy mode requires Azure CLI inside the container image.
The blob-copy storage type wraps user commands with az storage blob download-batch and upload-batch commands that execute inside the container. This requires the container image to have Azure CLI installed and authenticated, which may not be the case for standard Unity build images.
Consider documenting this requirement or implementing an alternative approach (e.g., using a sidecar or init container for artifact staging).
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <[email protected]>
frostebite
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (9)
src/model/orchestrator/providers/gcp-cloud-run/index.ts (3)
219-225:⚠️ Potential issue | 🟠 Major
workingdiris accepted but not applied to executed job command.Line 224 receives
workingdir, but Line 301 only sendseffectiveCommands. Builds depending on cwd can fail.Proposed fix
if (effectiveCommands) { + const commandWithWorkingDir = workingdir + ? `cd '${workingdir.replace(/'/g, "'\\''")}' && ${effectiveCommands}` + : effectiveCommands; + const updateCmd = [ 'gcloud run jobs update', jobName, `--region=${this.region}`, '--command="/bin/sh"', - `--args="-c,${effectiveCommands}"`, + `--args="-c,${commandWithWorkingDir.replace(/"/g, '\\"')}"`, projectFlag, '--quiet', ]Also applies to: 295-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 219 - 225, The runTaskInWorkflow function accepts a workingdir but never applies it to the executed command; update the construction of effectiveCommands (the value sent to Cloud Run in the section around where effectiveCommands is built/used, referenced in runTaskInWorkflow and lines ~295-302) to honor workingdir by prepending a directory change (e.g., "cd <workingdir> && ") when workingdir is non-empty, or alternatively set the container's working directory in the job spec if available; ensure all places that send effectiveCommands use this updated value so commands run in the intended cwd.
114-119:⚠️ Potential issue | 🟠 MajorLint blockers: unused interface parameters are currently failing
no-unused-vars.Line 116–118, Line 359–361, and Line 370–371 should be marked intentionally unused to keep CI green.
Proposed fix
async setupWorkflow( buildGuid: string, buildParameters: BuildParameters, branchName: string, defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], ) { + void buildParameters; + void branchName; + void defaultSecretsArray; OrchestratorLogger.log(`[GCP Cloud Run] Setting up workflow for build ${buildGuid}`); @@ async cleanupWorkflow( buildParameters: BuildParameters, branchName: string, defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], ) { + void buildParameters; + void branchName; + void defaultSecretsArray; OrchestratorLogger.log('[GCP Cloud Run] Cleaning up workflow'); @@ async garbageCollect( filter: string, previewOnly: boolean, olderThan: Number, fullCache: boolean, baseDependencies: boolean, ): Promise<string> { + void filter; + void fullCache; + void baseDependencies; OrchestratorLogger.log('[GCP Cloud Run] Garbage collecting old jobs');Also applies to: 358-372
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 114 - 119, The listed lint errors come from unused function parameters; update the signatures of setupWorkflow (buildGuid, buildParameters, branchName, defaultSecretsArray) by prefixing intentionally unused parameters with an underscore (e.g., _buildGuid, _buildParameters, _branchName, _defaultSecretsArray) so TS/ESLint treats them as used, and apply the same underscore-prefix fix to the other affected functions in this file (the other parameter lists around lines mentioned) to silence no-unused-vars.
61-62:⚠️ Potential issue | 🟠 MajorConfigured compute sizing is ignored at execution time.
Line 61 captures
gcpMachineType, but Line 267–268 hardcodes CPU/memory, so user sizing inputs don’t take effect.Proposed fix
+ private resolveComputeResources(): { cpu: number; memoryGi: number } { + const lastToken = this.machineType.split('-').at(-1) || ''; + const parsedCpu = Number.parseInt(lastToken, 10); + const cpu = Number.isFinite(parsedCpu) && parsedCpu > 0 ? parsedCpu : 4; + return { cpu, memoryGi: cpu * 4 }; + } @@ - // Create the Cloud Run Job + const { cpu, memoryGi } = this.resolveComputeResources(); + // Create the Cloud Run Job const createCmd = [ @@ - '--cpu=4', - '--memory=16Gi', + `--cpu=${cpu}`, + `--memory=${memoryGi}Gi`,Also applies to: 267-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts` around lines 61 - 62, The code reads buildParameters.gcpMachineType and buildParameters.gcpDiskSizeGb into this.machineType and this.diskSizeGb but later hardcodes CPU/memory (around the block that sets container CPU/memory at lines ~267–268), so user sizing is ignored; replace the hardcoded cpu/memory with values derived from this.machineType (or a small mapping from known GCP machine types to cpu/memory) and ensure any storage/ephemeral disk config uses this.diskSizeGb when configuring the Cloud Run revision/service; update the block that currently sets the cpu and memory to reference this.machineType (or the mapping) and use this.diskSizeGb for disk settings so buildParameters.gcpMachineType and gcpDiskSizeGb take effect.src/model/orchestrator/providers/azure-aci/index.ts (6)
488-492:⚠️ Potential issue | 🟠 MajorGarbage-collection age filter is incorrect and may delete unintended containers.
Line 488 treats
provisioningStateas a timestamp, and Line 491 uses||, so terminated containers are deleted regardless of age.Proposed fix
- const createdAt = new Date(container.tags?.createdAt || container.properties?.provisioningState || 0); + const createdAtRaw = container.tags?.createdAt; + const createdAt = createdAtRaw ? new Date(createdAtRaw) : undefined; + const isOlderThanCutoff = createdAt ? createdAt < cutoffDate : false; const state = container.containers?.[0]?.instanceView?.currentState?.state || ''; - if (state === 'Terminated' || createdAt < cutoffDate) { + if (state === 'Terminated' && isOlderThanCutoff) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 488 - 492, The GC condition is wrong: you treat container.properties?.provisioningState as a timestamp and use || so any Terminated container is deleted regardless of age; change creation-time extraction to parse a real timestamp (use container.tags?.createdAt parsed into a Date, falling back to a numeric/Date field such as container.properties?.createdTime or 0 — do not use provisioningState as a timestamp) and change the deletion condition to require both termination AND older-than-cutoff (i.e., state === 'Terminated' && createdAt < cutoffDate). Ensure createdAt is a Date/number parsed reliably before comparing to cutoffDate and keep previewOnly handling unchanged.
304-310:⚠️ Potential issue | 🟠 Major
workingdiris ignored when constructing the container command.Line 309 accepts
workingdir, but Line 343–345 executes onlyeffectiveCommands. This can break builds expecting a specific CWD.Proposed fix
- const commandFlag = effectiveCommands - ? `--command-line "/bin/sh -c '${effectiveCommands.replace(/'/g, "'\\''")}'"` + const commandToRun = + effectiveCommands && workingdir + ? `cd '${workingdir.replace(/'/g, "'\\''")}' && ${effectiveCommands}` + : effectiveCommands; + + const commandFlag = commandToRun + ? `--command-line "/bin/sh -c '${commandToRun.replace(/'/g, "'\\''")}'"` : '';Also applies to: 343-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 304 - 310, runTaskInWorkflow currently accepts workingdir but ignores it when building the container execution command; update the logic that builds/uses effectiveCommands so the container runs in the requested CWD. Inside runTaskInWorkflow, detect a non-empty workingdir and either (a) prefix effectiveCommands with a safe chdir (e.g. "cd <workingdir> && <effectiveCommands>") or (b) set the container execution/WorkingDir property if the Azure ACI API/SDK call used supports it—ensure you reference and modify the same place where effectiveCommands is executed so the container runs in workingdir rather than the default.
119-123:⚠️ Potential issue | 🟠 MajorMissing
azureResourceGroupshould hard-fail before command execution.Line 120 only warns, but Line 349 always passes
--resource-group, including empty values. This causes avoidable runtime failures.Proposed fix
async setupWorkflow( buildGuid: string, buildParameters: BuildParameters, branchName: string, defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], ) { + if (!this.resourceGroup) { + throw new Error('[Azure ACI] azureResourceGroup is required for providerStrategy=azure-aci'); + } + OrchestratorLogger.log(`[Azure ACI] Setting up workflow for build ${buildGuid}`);Also applies to: 347-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 119 - 123, The current check uses OrchestratorLogger.logWarning when this.resourceGroup is missing but code later always supplies `--resource-group` (causing runtime errors); change the behavior to hard-fail early by throwing an Error (or calling process.exit) when this.resourceGroup is falsy in the same validation block where OrchestratorLogger.logWarning is called, and also add a guard in the command-building path (the method that constructs the az CLI args where `--resource-group` is appended) to never append `--resource-group` with an empty value—use the thrown error to abort before command assembly or omit the flag only when a valid this.resourceGroup exists.
126-131:⚠️ Potential issue | 🟠 MajorLint blockers: intentionally-unused interface parameters need explicit handling.
no-unused-varserrors at Line 128–130, Line 456–458, and Line 467–468 will fail CI.Proposed fix
async setupWorkflow( buildGuid: string, buildParameters: BuildParameters, branchName: string, defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], ) { + void buildParameters; + void branchName; + void defaultSecretsArray; OrchestratorLogger.log(`[Azure ACI] Setting up workflow for build ${buildGuid}`); @@ async cleanupWorkflow( buildParameters: BuildParameters, branchName: string, defaultSecretsArray: { ParameterKey: string; EnvironmentVariable: string; ParameterValue: string }[], ) { + void buildParameters; + void branchName; + void defaultSecretsArray; OrchestratorLogger.log('[Azure ACI] Cleaning up workflow'); @@ async garbageCollect( filter: string, previewOnly: boolean, olderThan: Number, fullCache: boolean, baseDependencies: boolean, ): Promise<string> { + void filter; + void fullCache; + void baseDependencies; OrchestratorLogger.log('[Azure ACI] Garbage collecting old container groups');Also applies to: 455-469
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 126 - 131, The lint failures are due to intentionally-unused parameters; rename those parameters to start with an underscore (e.g., buildParameters -> _buildParameters, branchName -> _branchName, defaultSecretsArray -> _defaultSecretsArray) in async setupWorkflow(buildGuid, buildParameters, branchName, defaultSecretsArray) and do the same for the other functions referenced around lines 455–469 so ESLint recognizes them as intentionally unused; alternatively, explicitly reference them with void (e.g., void _param) if you prefer to keep names visible—update all occurrences of the unique function/method signatures in this file to use the underscore-prefixed names or void usage to resolve no-unused-vars.
13-16:⚠️ Potential issue | 🔴 Critical
azure-files-nfsshould fail fast instead of attempting a mount path that won’t work in ACI.Line 275–286 reuses SMB-style Azure Files flags for
azure-files-nfs, which is misleading and will fail at runtime for users selecting that mode.Does Azure Container Instances support mounting Azure Files NFS shares (NFS 4.1), or only SMB Azure File share mounts via account key?Proposed fix
- case 'azure-files-nfs': { - // ACI NFS mount uses a YAML deployment template; for CLI we use the same - // azure-file-volume flags but the share must be NFS-enabled and - // the container must be in a VNet - const storageKey = await this.getStorageKey(); - if (!storageKey) return ''; - return [ - `--azure-file-volume-account-name "${this.storageAccount}"`, - `--azure-file-volume-account-key "${storageKey}"`, - `--azure-file-volume-share-name "${this.fileShareName}"`, - `--azure-file-volume-mount-path "${mountdir}"`, - ].join(' '); - } + case 'azure-files-nfs': + throw new Error( + '[Azure ACI] NFS mounts are not supported on Azure Container Instances; use azure-files (SMB), blob-copy, or a different provider.', + );Also applies to: 275-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 13 - 16, The code path that builds Azure Files mount flags in src/model/orchestrator/providers/azure-aci/index.ts currently reuses SMB-style options for the "azure-files-nfs" mode; instead, detect the "azure-files-nfs" string early (where azureStorageAccount / azureFileShareName / azureSubnetId are handled) and fail fast with a clear error/exception rather than constructing SMB mount flags or attempting a mount. Update the branch that handles Azure Files mounts to: if mode === "azure-files-nfs" throw/return an explicit unsupported/invalid-configuration error (with a helpful message) so callers don’t proceed, and keep the existing SMB flag construction only for the SMB mode (e.g., the code path that references azureFileShareName and SMB mount options).
418-423:⚠️ Potential issue | 🟠 MajorCompletion detection is too permissive and can report success early.
At Line 418,
provisioningState === 'Succeeded'is treated as completion even when no terminal exit status is present.Proposed fix
- if (containerState === 'Terminated' || provisioningState === 'Succeeded') { + const exitCode = state.containers?.[0]?.instanceView?.currentState?.exitCode; + if (containerState === 'Terminated' || exitCode !== undefined) { - const exitCode = state.containers?.[0]?.instanceView?.currentState?.exitCode; if (exitCode !== undefined && exitCode !== 0) { throw new Error(`[Azure ACI] Container exited with code ${exitCode}`); } OrchestratorLogger.log('[Azure ACI] Container completed successfully');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/orchestrator/providers/azure-aci/index.ts` around lines 418 - 423, The current completion check treats provisioningState === 'Succeeded' as a terminal success even if the container has no terminal instance view; update the logic in the block that references containerState, provisioningState and state.containers?.[0]?.instanceView to only treat the operation as completed when either containerState === 'Terminated' OR provisioningState === 'Succeeded' AND a terminal container state/exitCode is present (e.g., check state.containers?.[0]?.instanceView?.currentState?.state === 'Terminated' and/or existence of exitCode). Move the success log and the non-zero exitCode throw to run only after confirming a terminal instanceView is present; if provisioningState is 'Succeeded' but no terminal instanceView yet, do not log success or throw and instead wait/continue polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/model/orchestrator/providers/azure-aci/index.ts`:
- Around line 488-492: The GC condition is wrong: you treat
container.properties?.provisioningState as a timestamp and use || so any
Terminated container is deleted regardless of age; change creation-time
extraction to parse a real timestamp (use container.tags?.createdAt parsed into
a Date, falling back to a numeric/Date field such as
container.properties?.createdTime or 0 — do not use provisioningState as a
timestamp) and change the deletion condition to require both termination AND
older-than-cutoff (i.e., state === 'Terminated' && createdAt < cutoffDate).
Ensure createdAt is a Date/number parsed reliably before comparing to cutoffDate
and keep previewOnly handling unchanged.
- Around line 304-310: runTaskInWorkflow currently accepts workingdir but
ignores it when building the container execution command; update the logic that
builds/uses effectiveCommands so the container runs in the requested CWD. Inside
runTaskInWorkflow, detect a non-empty workingdir and either (a) prefix
effectiveCommands with a safe chdir (e.g. "cd <workingdir> &&
<effectiveCommands>") or (b) set the container execution/WorkingDir property if
the Azure ACI API/SDK call used supports it—ensure you reference and modify the
same place where effectiveCommands is executed so the container runs in
workingdir rather than the default.
- Around line 119-123: The current check uses OrchestratorLogger.logWarning when
this.resourceGroup is missing but code later always supplies `--resource-group`
(causing runtime errors); change the behavior to hard-fail early by throwing an
Error (or calling process.exit) when this.resourceGroup is falsy in the same
validation block where OrchestratorLogger.logWarning is called, and also add a
guard in the command-building path (the method that constructs the az CLI args
where `--resource-group` is appended) to never append `--resource-group` with an
empty value—use the thrown error to abort before command assembly or omit the
flag only when a valid this.resourceGroup exists.
- Around line 126-131: The lint failures are due to intentionally-unused
parameters; rename those parameters to start with an underscore (e.g.,
buildParameters -> _buildParameters, branchName -> _branchName,
defaultSecretsArray -> _defaultSecretsArray) in async setupWorkflow(buildGuid,
buildParameters, branchName, defaultSecretsArray) and do the same for the other
functions referenced around lines 455–469 so ESLint recognizes them as
intentionally unused; alternatively, explicitly reference them with void (e.g.,
void _param) if you prefer to keep names visible—update all occurrences of the
unique function/method signatures in this file to use the underscore-prefixed
names or void usage to resolve no-unused-vars.
- Around line 13-16: The code path that builds Azure Files mount flags in
src/model/orchestrator/providers/azure-aci/index.ts currently reuses SMB-style
options for the "azure-files-nfs" mode; instead, detect the "azure-files-nfs"
string early (where azureStorageAccount / azureFileShareName / azureSubnetId are
handled) and fail fast with a clear error/exception rather than constructing SMB
mount flags or attempting a mount. Update the branch that handles Azure Files
mounts to: if mode === "azure-files-nfs" throw/return an explicit
unsupported/invalid-configuration error (with a helpful message) so callers
don’t proceed, and keep the existing SMB flag construction only for the SMB mode
(e.g., the code path that references azureFileShareName and SMB mount options).
- Around line 418-423: The current completion check treats provisioningState ===
'Succeeded' as a terminal success even if the container has no terminal instance
view; update the logic in the block that references containerState,
provisioningState and state.containers?.[0]?.instanceView to only treat the
operation as completed when either containerState === 'Terminated' OR
provisioningState === 'Succeeded' AND a terminal container state/exitCode is
present (e.g., check state.containers?.[0]?.instanceView?.currentState?.state
=== 'Terminated' and/or existence of exitCode). Move the success log and the
non-zero exitCode throw to run only after confirming a terminal instanceView is
present; if provisioningState is 'Succeeded' but no terminal instanceView yet,
do not log success or throw and instead wait/continue polling.
In `@src/model/orchestrator/providers/gcp-cloud-run/index.ts`:
- Around line 219-225: The runTaskInWorkflow function accepts a workingdir but
never applies it to the executed command; update the construction of
effectiveCommands (the value sent to Cloud Run in the section around where
effectiveCommands is built/used, referenced in runTaskInWorkflow and lines
~295-302) to honor workingdir by prepending a directory change (e.g., "cd
<workingdir> && ") when workingdir is non-empty, or alternatively set the
container's working directory in the job spec if available; ensure all places
that send effectiveCommands use this updated value so commands run in the
intended cwd.
- Around line 114-119: The listed lint errors come from unused function
parameters; update the signatures of setupWorkflow (buildGuid, buildParameters,
branchName, defaultSecretsArray) by prefixing intentionally unused parameters
with an underscore (e.g., _buildGuid, _buildParameters, _branchName,
_defaultSecretsArray) so TS/ESLint treats them as used, and apply the same
underscore-prefix fix to the other affected functions in this file (the other
parameter lists around lines mentioned) to silence no-unused-vars.
- Around line 61-62: The code reads buildParameters.gcpMachineType and
buildParameters.gcpDiskSizeGb into this.machineType and this.diskSizeGb but
later hardcodes CPU/memory (around the block that sets container CPU/memory at
lines ~267–268), so user sizing is ignored; replace the hardcoded cpu/memory
with values derived from this.machineType (or a small mapping from known GCP
machine types to cpu/memory) and ensure any storage/ephemeral disk config uses
this.diskSizeGb when configuring the Cloud Run revision/service; update the
block that currently sets the cpu and memory to reference this.machineType (or
the mapping) and use this.diskSizeGb for disk settings so
buildParameters.gcpMachineType and gcpDiskSizeGb take effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a6c2c8c-5813-47da-a625-cf8c9d8a05b6
⛔ Files ignored due to path filters (2)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (3)
action.ymlsrc/model/orchestrator/providers/azure-aci/index.tssrc/model/orchestrator/providers/gcp-cloud-run/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- action.yml
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #778 +/- ##
==========================================
- Coverage 31.25% 29.50% -1.75%
==========================================
Files 84 86 +2
Lines 4563 5016 +453
Branches 1103 1257 +154
==========================================
+ Hits 1426 1480 +54
- Misses 3137 3536 +399
🚀 New features to boost your workflow:
|
This was referenced Mar 5, 2026
frostebite
added
enhancement
This was referenced Mar 5, 2026
Co-Authored-By: Claude Opus 4.6 <[email protected]>
The orchestrator-develop branch no longer exists. Update all fallback clone commands and test fixtures to use main instead. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
|
Closing — all orchestrator code has been extracted to the standalone Content from this PR (GCP Cloud Run and Azure ACI providers) is fully present in the orchestrator repo. See PR #819 for the extraction. |
12 tasks
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
Two new experimental orchestrator providers for Google Cloud Platform and Microsoft Azure, each with four configurable storage backends.
Depends on: #777 (CLI provider protocol, submodule profiles, local caching, LFS agents, git hooks)
GCP Cloud Run Jobs (
providerStrategy: gcp-cloud-run)Executes Unity builds as Cloud Run Jobs. Storage via
gcpStorageType:gcs-fuse(default)gcs-copynfsin-memoryInputs:
gcpProject,gcpRegion,gcpStorageType,gcpBucket,gcpFilestoreIp,gcpFilestoreShare,gcpMachineType,gcpDiskSizeGb,gcpServiceAccount,gcpVpcConnectorAzure Container Instances (
providerStrategy: azure-aci)Executes Unity builds as Azure Container Instances. Storage via
azureStorageType:azure-files(default)blob-copyazure-files-nfsin-memoryInputs:
azureResourceGroup,azureLocation,azureStorageType,azureStorageAccount,azureBlobContainer,azureFileShareName,azureSubscriptionId,azureCpu,azureMemoryGb,azureDiskSizeGb,azureSubnetIdUsage
Both providers are experimental. No tests (require real cloud infrastructure).
Test plan
Documentation
🤖 Generated with Claude Code
Summary by CodeRabbit
Tracking: