refactor(jobs): extract container resolution logic for deployment commands

andrasbacsai · andrasbacsai · commit 811ee5d327da · 2026-03-25T14:09:07.000+01:00
Extract common container selection logic into resolveCommandContainer() method
that handles both single and multi-container app scenarios. This consolidates
duplicated code from run_pre_deployment_command() and run_post_deployment_command()
while improving error messaging and test coverage.
diff --git a/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php
@@ -3989,43 +3989,88 @@ private function validateContainerName(string $value): string
         return $value;
     }
 
+    /**
+     * Resolve which container to execute a deployment command in.
+     *
+     * For single-container apps, returns the sole container.
+     * For multi-container apps, matches by the user-specified container name.
+     * If no container name is specified for multi-container apps, logs available containers and returns null.
+     */
+    private function resolveCommandContainer(Collection $containers, ?string $specifiedContainerName, string $commandType): ?array
+    {
+        if ($containers->count() === 0) {
+            return null;
+        }
+
+        if ($containers->count() === 1) {
+            return $containers->first();
+        }
+
+        // Multi-container: require a container name to be specified
+        if (empty($specifiedContainerName)) {
+            $available = $containers->map(fn ($c) => data_get($c, 'Names'))->implode(', ');
+            $this->application_deployment_queue->addLogEntry(
+                "{$commandType} command: Multiple containers found but no container name specified. Available: {$available}"
+            );
+
+            return null;
+        }
+
+        // Multi-container: match by specified name prefix
+        $prefix = $specifiedContainerName.'-'.$this->application->uuid;
+        foreach ($containers as $container) {
+            $containerName = data_get($container, 'Names');
+            if (str_starts_with($containerName, $prefix)) {
+                return $container;
+            }
+        }
+
+        // No match found — log available containers to help the user debug
+        $available = $containers->map(fn ($c) => data_get($c, 'Names'))->implode(', ');
+        $this->application_deployment_queue->addLogEntry(
+            "{$commandType} command: Container '{$specifiedContainerName}' not found. Available: {$available}"
+        );
+
+        return null;
+    }
+
     private function run_pre_deployment_command()
     {
         if (empty($this->application->pre_deployment_command)) {
             return;
         }
         $containers = getCurrentApplicationContainerStatus($this->server, $this->application->id, $this->pull_request_id);
         if ($containers->count() == 0) {
+            $this->application_deployment_queue->addLogEntry('Pre-deployment command: No running containers found. Skipping.');
+
             return;
         }
         $this->application_deployment_queue->addLogEntry('Executing pre-deployment command (see debug log for output/errors).');
 
-        foreach ($containers as $container) {
-            $containerName = data_get($container, 'Names');
-            if ($containerName) {
-                $this->validateContainerName($containerName);
-            }
-            if ($containers->count() == 1 || str_starts_with($containerName, $this->application->pre_deployment_command_container.'-'.$this->application->uuid)) {
-                // Security: pre_deployment_command is intentionally treated as arbitrary shell input.
-                // Users (team members with deployment access) need full shell flexibility to run commands
-                // like "php artisan migrate", "npm run build", etc. inside their own application containers.
-                // The trust boundary is at the application/team ownership level — only authenticated team
-                // members can set these commands, and execution is scoped to the application's own container.
-                // The single-quote escaping here prevents breaking out of the sh -c wrapper, but does not
-                // restrict the command itself. Container names are validated separately via validateContainerName().
-                $cmd = "sh -c '".str_replace("'", "'\''", $this->application->pre_deployment_command)."'";
-                $exec = "docker exec {$containerName} {$cmd}";
-                $this->execute_remote_command(
-                    [
-                        'command' => $exec,
-                        'hidden' => true,
-                    ],
-                );
+        $container = $this->resolveCommandContainer($containers, $this->application->pre_deployment_command_container, 'Pre-deployment');
+        if ($container === null) {
+            throw new DeploymentException('Pre-deployment command: Could not find a valid container. Is the container name correct?');
+        }
 
-                return;
-            }
+        $containerName = data_get($container, 'Names');
+        if ($containerName) {
+            $this->validateContainerName($containerName);
         }
-        throw new DeploymentException('Pre-deployment command: Could not find a valid container. Is the container name correct?');
+        // Security: pre_deployment_command is intentionally treated as arbitrary shell input.
+        // Users (team members with deployment access) need full shell flexibility to run commands
+        // like "php artisan migrate", "npm run build", etc. inside their own application containers.
+        // The trust boundary is at the application/team ownership level — only authenticated team
+        // members can set these commands, and execution is scoped to the application's own container.
+        // The single-quote escaping here prevents breaking out of the sh -c wrapper, but does not
+        // restrict the command itself. Container names are validated separately via validateContainerName().
+        $cmd = "sh -c '".str_replace("'", "'\''", $this->application->pre_deployment_command)."'";
+        $exec = "docker exec {$containerName} {$cmd}";
+        $this->execute_remote_command(
+            [
+                'command' => $exec,
+                'hidden' => true,
+            ],
+        );
     }
 
     private function run_post_deployment_command()
@@ -4037,36 +4082,40 @@ private function run_post_deployment_command()
         $this->application_deployment_queue->addLogEntry('Executing post-deployment command (see debug log for output).');
 
         $containers = getCurrentApplicationContainerStatus($this->server, $this->application->id, $this->pull_request_id);
-        foreach ($containers as $container) {
-            $containerName = data_get($container, 'Names');
-            if ($containerName) {
-                $this->validateContainerName($containerName);
-            }
-            if ($containers->count() == 1 || str_starts_with($containerName, $this->application->post_deployment_command_container.'-'.$this->application->uuid)) {
-                // Security: post_deployment_command is intentionally treated as arbitrary shell input.
-                // See the equivalent comment in run_pre_deployment_command() for the full security rationale.
-                $cmd = "sh -c '".str_replace("'", "'\''", $this->application->post_deployment_command)."'";
-                $exec = "docker exec {$containerName} {$cmd}";
-                try {
-                    $this->execute_remote_command(
-                        [
-                            'command' => $exec,
-                            'hidden' => true,
-                            'save' => 'post-deployment-command-output',
-                        ],
-                    );
-                } catch (Exception $e) {
-                    $post_deployment_command_output = $this->saved_outputs->get('post-deployment-command-output');
-                    if ($post_deployment_command_output) {
-                        $this->application_deployment_queue->addLogEntry('Post-deployment command failed.');
-                        $this->application_deployment_queue->addLogEntry($post_deployment_command_output, 'stderr');
-                    }
-                }
+        if ($containers->count() == 0) {
+            $this->application_deployment_queue->addLogEntry('Post-deployment command: No running containers found. Skipping.');
 
-                return;
+            return;
+        }
+
+        $container = $this->resolveCommandContainer($containers, $this->application->post_deployment_command_container, 'Post-deployment');
+        if ($container === null) {
+            throw new DeploymentException('Post-deployment command: Could not find a valid container. Is the container name correct?');
+        }
+
+        $containerName = data_get($container, 'Names');
+        if ($containerName) {
+            $this->validateContainerName($containerName);
+        }
+        // Security: post_deployment_command is intentionally treated as arbitrary shell input.
+        // See the equivalent comment in run_pre_deployment_command() for the full security rationale.
+        $cmd = "sh -c '".str_replace("'", "'\''", $this->application->post_deployment_command)."'";
+        $exec = "docker exec {$containerName} {$cmd}";
+        try {
+            $this->execute_remote_command(
+                [
+                    'command' => $exec,
+                    'hidden' => true,
+                    'save' => 'post-deployment-command-output',
+                ],
+            );
+        } catch (Exception $e) {
+            $post_deployment_command_output = $this->saved_outputs->get('post-deployment-command-output');
+            if ($post_deployment_command_output) {
+                $this->application_deployment_queue->addLogEntry('Post-deployment command failed.');
+                $this->application_deployment_queue->addLogEntry($post_deployment_command_output, 'stderr');
             }
         }
-        throw new DeploymentException('Post-deployment command: Could not find a valid container. Is the container name correct?');
     }
 
     /**
diff --git a/tests/Feature/DeploymentCommandContainerResolutionTest.php b/tests/Feature/DeploymentCommandContainerResolutionTest.php
@@ -0,0 +1,116 @@
+<?php
+
+use App\Jobs\ApplicationDeploymentJob;
+use App\Models\Application;
+use App\Models\ApplicationDeploymentQueue;
+
+function createJobWithProperties(string $uuid): object
+{
+    $ref = new ReflectionClass(ApplicationDeploymentJob::class);
+    $instance = $ref->newInstanceWithoutConstructor();
+
+    $app = Mockery::mock(Application::class)->makePartial();
+    $app->uuid = $uuid;
+
+    $queue = Mockery::mock(ApplicationDeploymentQueue::class)->makePartial();
+    $queue->shouldReceive('addLogEntry')->andReturnNull();
+
+    $appProp = $ref->getProperty('application');
+    $appProp->setAccessible(true);
+    $appProp->setValue($instance, $app);
+
+    $queueProp = $ref->getProperty('application_deployment_queue');
+    $queueProp->setAccessible(true);
+    $queueProp->setValue($instance, $queue);
+
+    return $instance;
+}
+
+function invokeResolve(object $instance, $containers, ?string $specifiedName, string $type): ?array
+{
+    $ref = new ReflectionClass(ApplicationDeploymentJob::class);
+    $method = $ref->getMethod('resolveCommandContainer');
+    $method->setAccessible(true);
+
+    return $method->invoke($instance, $containers, $specifiedName, $type);
+}
+
+describe('resolveCommandContainer', function () {
+    test('returns null when no containers exist', function () {
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, collect([]), 'web', 'Pre-deployment');
+
+        expect($result)->toBeNull();
+    });
+
+    test('returns the sole container when only one exists', function () {
+        $container = ['Names' => 'web-abc123', 'Labels' => ''];
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, collect([$container]), null, 'Pre-deployment');
+
+        expect($result)->toBe($container);
+    });
+
+    test('returns the sole container regardless of specified name when only one exists', function () {
+        $container = ['Names' => 'web-abc123', 'Labels' => ''];
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, collect([$container]), 'wrong-name', 'Pre-deployment');
+
+        expect($result)->toBe($container);
+    });
+
+    test('returns null when no container name specified for multi-container app', function () {
+        $containers = collect([
+            ['Names' => 'web-abc123', 'Labels' => ''],
+            ['Names' => 'worker-abc123', 'Labels' => ''],
+        ]);
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, $containers, null, 'Pre-deployment');
+
+        expect($result)->toBeNull();
+    });
+
+    test('returns null when empty string container name for multi-container app', function () {
+        $containers = collect([
+            ['Names' => 'web-abc123', 'Labels' => ''],
+            ['Names' => 'worker-abc123', 'Labels' => ''],
+        ]);
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, $containers, '', 'Pre-deployment');
+
+        expect($result)->toBeNull();
+    });
+
+    test('matches correct container by specified name in multi-container app', function () {
+        $containers = collect([
+            ['Names' => 'web-abc123', 'Labels' => ''],
+            ['Names' => 'worker-abc123', 'Labels' => ''],
+        ]);
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, $containers, 'worker', 'Pre-deployment');
+
+        expect($result)->toBe(['Names' => 'worker-abc123', 'Labels' => '']);
+    });
+
+    test('returns null when specified container name does not match any container', function () {
+        $containers = collect([
+            ['Names' => 'web-abc123', 'Labels' => ''],
+            ['Names' => 'worker-abc123', 'Labels' => ''],
+        ]);
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, $containers, 'nonexistent', 'Pre-deployment');
+
+        expect($result)->toBeNull();
+    });
+
+    test('matches container with PR suffix', function () {
+        $containers = collect([
+            ['Names' => 'web-abc123-pr-42', 'Labels' => ''],
+            ['Names' => 'worker-abc123-pr-42', 'Labels' => ''],
+        ]);
+        $instance = createJobWithProperties('abc123');
+        $result = invokeResolve($instance, $containers, 'web', 'Pre-deployment');
+
+        expect($result)->toBe(['Names' => 'web-abc123-pr-42', 'Labels' => '']);
+    });
+});
