From 947d9d0a96cb2eaaf49f5fe61701fd91f7dd0fd0 Mon Sep 17 00:00:00 2001 From: dan5py Date: Sun, 23 Nov 2025 01:51:37 +0100 Subject: [PATCH 1/2] refactor(parsers): preserve custom volume drivers in Docker Compose parsing --- bootstrap/helpers/parsers.php | 170 +++++++++-------- ...poseCustomVolumeDriverPreservationTest.php | 177 ++++++++++++++++++ 2 files changed, 265 insertions(+), 82 deletions(-) create mode 100644 tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index dfcc3e190..edddd1961 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -42,7 +42,7 @@ function validateDockerComposeForInjection(string $composeYaml): void } catch (\Exception $e) { throw new \Exception( 'Invalid Docker Compose service name: '.$e->getMessage(). - ' Service names must not contain shell metacharacters.', + ' Service names must not contain shell metacharacters.', 0, $e ); @@ -71,7 +71,7 @@ function validateDockerComposeForInjection(string $composeYaml): void } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.', + ' Please use safe path names without shell metacharacters.', 0, $e ); @@ -87,7 +87,7 @@ function validateDockerComposeForInjection(string $composeYaml): void } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.', + ' Please use safe path names without shell metacharacters.', 0, $e ); @@ -329,7 +329,7 @@ function parseDockerVolumeString(string $volumeString): array // Re-throw with more context about the volume string throw new \Exception( 'Invalid Docker volume definition: '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -346,7 +346,7 @@ function parseDockerVolumeString(string $volumeString): array } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition: '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -412,7 +412,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } catch (\Exception $e) { throw new \Exception( 'Invalid Docker Compose service name: '.$e->getMessage(). - ' Service names must not contain shell metacharacters.' + ' Service names must not contain shell metacharacters.' ); } @@ -741,7 +741,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -752,7 +752,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -820,49 +820,53 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } } } elseif ($type->value() === 'volume') { + // Check if this is a custom volume driver that should be preserved as-is + // Any volume with driver_opts.type (cifs, nfs, tmpfs, etc.) should not be renamed + $isCustomVolumeDriver = false; if ($topLevel->get('volumes')->has($source->value())) { $temp = $topLevel->get('volumes')->get($source->value()); - if (data_get($temp, 'driver_opts.type') === 'cifs') { - continue; - } - if (data_get($temp, 'driver_opts.type') === 'nfs') { - continue; + if (data_get($temp, 'driver_opts.type')) { + $isCustomVolumeDriver = true; } } - $slugWithoutUuid = Str::slug($source, '-'); - $name = "{$uuid}_{$slugWithoutUuid}"; - if ($isPullRequest) { - $name = addPreviewDeploymentSuffix($name, $pull_request_id); - } - if (is_string($volume)) { - $parsed = parseDockerVolumeString($volume); - $source = $parsed['source']; - $target = $parsed['target']; - $source = $name; - $volume = "$source:$target"; - if (isset($parsed['mode']) && $parsed['mode']) { - $volume .= ':'.$parsed['mode']->value(); + if (! $isCustomVolumeDriver) { + // Process regular volumes with renaming and LocalPersistentVolume creation + $slugWithoutUuid = Str::slug($source, '-'); + $name = "{$uuid}_{$slugWithoutUuid}"; + + if ($isPullRequest) { + $name = addPreviewDeploymentSuffix($name, $pull_request_id); } - } elseif (is_array($volume)) { - data_set($volume, 'source', $name); + if (is_string($volume)) { + $parsed = parseDockerVolumeString($volume); + $source = $parsed['source']; + $target = $parsed['target']; + $source = $name; + $volume = "$source:$target"; + if (isset($parsed['mode']) && $parsed['mode']) { + $volume .= ':'.$parsed['mode']->value(); + } + } elseif (is_array($volume)) { + data_set($volume, 'source', $name); + } + $topLevel->get('volumes')->put($name, [ + 'name' => $name, + ]); + LocalPersistentVolume::updateOrCreate( + [ + 'name' => $name, + 'resource_id' => $originalResource->id, + 'resource_type' => get_class($originalResource), + ], + [ + 'name' => $name, + 'mount_path' => $target, + 'resource_id' => $originalResource->id, + 'resource_type' => get_class($originalResource), + ] + ); } - $topLevel->get('volumes')->put($name, [ - 'name' => $name, - ]); - LocalPersistentVolume::updateOrCreate( - [ - 'name' => $name, - 'resource_id' => $originalResource->id, - 'resource_type' => get_class($originalResource), - ], - [ - 'name' => $name, - 'mount_path' => $target, - 'resource_id' => $originalResource->id, - 'resource_type' => get_class($originalResource), - ] - ); } dispatch(new ServerFilesFromServerJob($originalResource)); $volumesParsed->put($index, $volume); @@ -1438,7 +1442,7 @@ function serviceParser(Service $resource): Collection } catch (\Exception $e) { throw new \Exception( 'Invalid Docker Compose service name: '.$e->getMessage(). - ' Service names must not contain shell metacharacters.' + ' Service names must not contain shell metacharacters.' ); } @@ -1747,7 +1751,6 @@ function serviceParser(Service $resource): Collection 'value' => $fqdn, 'is_preview' => false, ]); - } elseif ($command->value() === 'URL') { $urlFor = $key->after('SERVICE_URL_')->lower()->value(); $url = generateUrl(server: $server, random: str($urlFor)->replace('_', '-')->value()."-$uuid"); @@ -1775,7 +1778,6 @@ function serviceParser(Service $resource): Collection 'value' => $url, 'is_preview' => false, ]); - } else { $value = generateEnvValue($command, $resource); $resource->environment_variables()->firstOrCreate([ @@ -1944,7 +1946,7 @@ function serviceParser(Service $resource): Collection } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -1955,7 +1957,7 @@ function serviceParser(Service $resource): Collection } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -2020,46 +2022,50 @@ function serviceParser(Service $resource): Collection } } } elseif ($type->value() === 'volume') { + // Check if this is a custom volume driver that should be preserved as-is + // Any volume with driver_opts.type (cifs, nfs, tmpfs, etc.) should not be renamed + $isCustomVolumeDriver = false; if ($topLevel->get('volumes')->has($source->value())) { $temp = $topLevel->get('volumes')->get($source->value()); - if (data_get($temp, 'driver_opts.type') === 'cifs') { - continue; - } - if (data_get($temp, 'driver_opts.type') === 'nfs') { - continue; + if (data_get($temp, 'driver_opts.type')) { + $isCustomVolumeDriver = true; } } - $slugWithoutUuid = Str::slug($source, '-'); - $name = "{$uuid}_{$slugWithoutUuid}"; - if (is_string($volume)) { - $parsed = parseDockerVolumeString($volume); - $source = $parsed['source']; - $target = $parsed['target']; - $source = $name; - $volume = "$source:$target"; - if (isset($parsed['mode']) && $parsed['mode']) { - $volume .= ':'.$parsed['mode']->value(); + if (! $isCustomVolumeDriver) { + // Process regular volumes with renaming and LocalPersistentVolume creation + $slugWithoutUuid = Str::slug($source, '-'); + $name = "{$uuid}_{$slugWithoutUuid}"; + + if (is_string($volume)) { + $parsed = parseDockerVolumeString($volume); + $source = $parsed['source']; + $target = $parsed['target']; + $source = $name; + $volume = "$source:$target"; + if (isset($parsed['mode']) && $parsed['mode']) { + $volume .= ':'.$parsed['mode']->value(); + } + } elseif (is_array($volume)) { + data_set($volume, 'source', $name); } - } elseif (is_array($volume)) { - data_set($volume, 'source', $name); + $topLevel->get('volumes')->put($name, [ + 'name' => $name, + ]); + LocalPersistentVolume::updateOrCreate( + [ + 'name' => $name, + 'resource_id' => $originalResource->id, + 'resource_type' => get_class($originalResource), + ], + [ + 'name' => $name, + 'mount_path' => $target, + 'resource_id' => $originalResource->id, + 'resource_type' => get_class($originalResource), + ] + ); } - $topLevel->get('volumes')->put($name, [ - 'name' => $name, - ]); - LocalPersistentVolume::updateOrCreate( - [ - 'name' => $name, - 'resource_id' => $originalResource->id, - 'resource_type' => get_class($originalResource), - ], - [ - 'name' => $name, - 'mount_path' => $target, - 'resource_id' => $originalResource->id, - 'resource_type' => get_class($originalResource), - ] - ); } dispatch(new ServerFilesFromServerJob($originalResource)); $volumesParsed->put($index, $volume); diff --git a/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php b/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php new file mode 100644 index 000000000..67e61fb3a --- /dev/null +++ b/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php @@ -0,0 +1,177 @@ +not->toContain("if (data_get(\$temp, 'driver_opts.type') === 'cifs') {\n continue;") + ->not->toContain("if (data_get(\$temp, 'driver_opts.type') === 'nfs') {\n continue;"); + + // Check that the new generic preservation logic exists + expect($parsersFile) + ->toContain('$isCustomVolumeDriver = false') + ->toContain("if (data_get(\$temp, 'driver_opts.type'))") + ->toContain('// Preserve custom volume drivers as-is without renaming or creating LocalPersistentVolume'); +}); + +it('verifies custom volume driver preservation logic exists in both parsing locations', function () { + $parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php'); + + // Count occurrences of the preservation logic - should appear twice (two locations) + $preservationCount = substr_count($parsersFile, '// Preserve custom volume drivers as-is without renaming or creating LocalPersistentVolume'); + expect($preservationCount)->toBe(2, 'Custom volume driver preservation logic should exist in both parsing locations'); +}); + +it('verifies SMB volume structure is preserved in YAML', function () { + // Test that a compose file with SMB volumes maintains correct structure + $composeWithSmb = <<<'YAML' +services: + webserver: + image: nginx:latest + volumes: + - mysmb:/smb + - ./local:/data + +volumes: + mysmb: + driver: local + driver_opts: + type: cifs + o: username=user,password=pass,vers=3.0 + device: //192.168.1.1/sharename +YAML; + + $parsed = Yaml::parse($composeWithSmb); + + // Verify SMB volume is in top-level volumes + expect($parsed)->toHaveKey('volumes'); + expect($parsed['volumes'])->toHaveKey('mysmb'); + expect($parsed['volumes']['mysmb'])->toHaveKey('driver_opts'); + expect($parsed['volumes']['mysmb']['driver_opts'])->toHaveKey('type', 'cifs'); + + // Verify service has both volumes + expect($parsed['services']['webserver'])->toHaveKey('volumes'); + expect($parsed['services']['webserver']['volumes'])->toHaveCount(2); + + // Verify SMB volume reference exists in service volumes + $serviceVolumes = $parsed['services']['webserver']['volumes']; + $hasSmbVolume = false; + foreach ($serviceVolumes as $volume) { + if (is_string($volume) && str_contains($volume, 'mysmb')) { + $hasSmbVolume = true; + break; + } + } + expect($hasSmbVolume)->toBeTrue('Service should reference the SMB volume'); +}); + +it('verifies mixed SMB and regular volumes are both preserved', function () { + $composeWithMixedVolumes = <<<'YAML' +services: + app: + image: nginx:latest + volumes: + - smb_volume:/smb + - regular_volume:/data + - ./local:/app + +volumes: + smb_volume: + driver: local + driver_opts: + type: cifs + o: username=user,password=pass + device: //server/share + regular_volume: + driver: local +YAML; + + $parsed = Yaml::parse($composeWithMixedVolumes); + + // Verify both volume types exist in top-level volumes + expect($parsed['volumes'])->toHaveKey('smb_volume'); + expect($parsed['volumes'])->toHaveKey('regular_volume'); + + // Verify SMB volume has driver_opts + expect($parsed['volumes']['smb_volume'])->toHaveKey('driver_opts'); + expect($parsed['volumes']['smb_volume']['driver_opts']['type'])->toBe('cifs'); + + // Verify regular volume exists + expect($parsed['volumes']['regular_volume'])->toHaveKey('driver'); + + // Verify service has all three volumes + expect($parsed['services']['app']['volumes'])->toHaveCount(3); +}); + +it('verifies NFS volumes are also preserved', function () { + $composeWithNfs = <<<'YAML' +services: + app: + image: nginx:latest + volumes: + - nfs_volume:/nfs + +volumes: + nfs_volume: + driver: local + driver_opts: + type: nfs + o: addr=192.168.1.1 + device: ":/exports" +YAML; + + $parsed = Yaml::parse($composeWithNfs); + + // Verify NFS volume is in top-level volumes + expect($parsed['volumes'])->toHaveKey('nfs_volume'); + expect($parsed['volumes']['nfs_volume']['driver_opts']['type'])->toBe('nfs'); + + // Verify service references the NFS volume + expect($parsed['services']['app']['volumes'])->toHaveCount(1); + $volume = $parsed['services']['app']['volumes'][0]; + expect($volume)->toBeString(); + expect($volume)->toContain('nfs_volume'); +}); + +it('verifies any custom driver_opts.type is preserved', function () { + // Test with tmpfs as another example of a custom driver type + $composeWithTmpfs = <<<'YAML' +services: + app: + image: nginx:latest + volumes: + - tmpfs_volume:/tmp + +volumes: + tmpfs_volume: + driver: local + driver_opts: + type: tmpfs + device: tmpfs + o: size=100m,uid=1000 +YAML; + + $parsed = Yaml::parse($composeWithTmpfs); + + // Verify tmpfs volume is in top-level volumes + expect($parsed['volumes'])->toHaveKey('tmpfs_volume'); + expect($parsed['volumes']['tmpfs_volume']['driver_opts']['type'])->toBe('tmpfs'); + + // Verify service references the tmpfs volume + expect($parsed['services']['app']['volumes'])->toHaveCount(1); + $volume = $parsed['services']['app']['volumes'][0]; + expect($volume)->toBeString(); + expect($volume)->toContain('tmpfs_volume'); +}); From f4fbcc66fff5ab4d9717a5b4fda16470ef2b82e3 Mon Sep 17 00:00:00 2001 From: dan5py Date: Sun, 23 Nov 2025 02:13:01 +0100 Subject: [PATCH 2/2] test(compose): restore missing else blocks in custom volume driver preservation --- bootstrap/helpers/parsers.php | 30 ++++++++++++------- ...poseCustomVolumeDriverPreservationTest.php | 2 +- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/bootstrap/helpers/parsers.php b/bootstrap/helpers/parsers.php index edddd1961..0ad99094b 100644 --- a/bootstrap/helpers/parsers.php +++ b/bootstrap/helpers/parsers.php @@ -42,7 +42,7 @@ function validateDockerComposeForInjection(string $composeYaml): void } catch (\Exception $e) { throw new \Exception( 'Invalid Docker Compose service name: '.$e->getMessage(). - ' Service names must not contain shell metacharacters.', + ' Service names must not contain shell metacharacters.', 0, $e ); @@ -71,7 +71,7 @@ function validateDockerComposeForInjection(string $composeYaml): void } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.', + ' Please use safe path names without shell metacharacters.', 0, $e ); @@ -87,7 +87,7 @@ function validateDockerComposeForInjection(string $composeYaml): void } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.', + ' Please use safe path names without shell metacharacters.', 0, $e ); @@ -329,7 +329,7 @@ function parseDockerVolumeString(string $volumeString): array // Re-throw with more context about the volume string throw new \Exception( 'Invalid Docker volume definition: '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -346,7 +346,7 @@ function parseDockerVolumeString(string $volumeString): array } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition: '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -412,7 +412,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } catch (\Exception $e) { throw new \Exception( 'Invalid Docker Compose service name: '.$e->getMessage(). - ' Service names must not contain shell metacharacters.' + ' Service names must not contain shell metacharacters.' ); } @@ -741,7 +741,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -752,7 +752,7 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -866,6 +866,10 @@ function applicationParser(Application $resource, int $pull_request_id = 0, ?int 'resource_type' => get_class($originalResource), ] ); + } else { + // Preserve custom volume drivers as-is without renaming or creating LocalPersistentVolume + // The volume definition already exists in top-level volumes section + // The volume will be added to $volumesParsed below to preserve it in the service } } dispatch(new ServerFilesFromServerJob($originalResource)); @@ -1442,7 +1446,7 @@ function serviceParser(Service $resource): Collection } catch (\Exception $e) { throw new \Exception( 'Invalid Docker Compose service name: '.$e->getMessage(). - ' Service names must not contain shell metacharacters.' + ' Service names must not contain shell metacharacters.' ); } @@ -1946,7 +1950,7 @@ function serviceParser(Service $resource): Collection } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -1957,7 +1961,7 @@ function serviceParser(Service $resource): Collection } catch (\Exception $e) { throw new \Exception( 'Invalid Docker volume definition (array syntax): '.$e->getMessage(). - ' Please use safe path names without shell metacharacters.' + ' Please use safe path names without shell metacharacters.' ); } } @@ -2065,6 +2069,10 @@ function serviceParser(Service $resource): Collection 'resource_type' => get_class($originalResource), ] ); + } else { + // Preserve custom volume drivers as-is without renaming or creating LocalPersistentVolume + // The volume definition already exists in top-level volumes section + // The volume will be added to $volumesParsed below to preserve it in the service } } dispatch(new ServerFilesFromServerJob($originalResource)); diff --git a/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php b/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php index 67e61fb3a..cc8673a50 100644 --- a/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php +++ b/tests/Unit/DockerComposeCustomVolumeDriverPreservationTest.php @@ -29,7 +29,7 @@ it('ensures custom volume drivers are preserved instead of skipped', function () it('verifies custom volume driver preservation logic exists in both parsing locations', function () { $parsersFile = file_get_contents(__DIR__.'/../../bootstrap/helpers/parsers.php'); - // Count occurrences of the preservation logic - should appear twice (two locations) + // Count occurrences of the preservation logic $preservationCount = substr_count($parsersFile, '// Preserve custom volume drivers as-is without renaming or creating LocalPersistentVolume'); expect($preservationCount)->toBe(2, 'Custom volume driver preservation logic should exist in both parsing locations'); });