mirror of
https://github.com/tiennm99/coolify.git
synced 2026-04-17 17:21:04 +00:00
feat: add validation for YAML parsing, integer parameters, and Docker Compose custom fields
This commit adds comprehensive validation improvements and DRY principles for handling Coolify's custom Docker Compose extensions. ## Changes ### 1. Created Reusable stripCoolifyCustomFields() Function - Added shared helper in bootstrap/helpers/docker.php - Removes all Coolify custom fields (exclude_from_hc, content, isDirectory, is_directory) - Handles both long syntax (arrays) and short syntax (strings) for volumes - Well-documented with comprehensive docblock - Follows DRY principle for consistent field stripping ### 2. Fixed Docker Compose Modal Validation - Updated validateComposeFile() to use stripCoolifyCustomFields() - Now removes ALL custom fields before Docker validation (previously only removed content) - Fixes validation errors when using templates with custom fields (e.g., traccar.yaml) - Users can now validate compose files with Coolify extensions in UI ### 3. Enhanced YAML Validation in CalculatesExcludedStatus - Added proper exception handling with ParseException vs generic Exception - Added structure validation (checks if parsed result and services are arrays) - Comprehensive logging with context (error message, line number, snippet) - Maintains safe fallback behavior (returns empty collection on error) ### 4. Added Integer Validation to ContainerStatusAggregator - Validates maxRestartCount parameter in both aggregateFromStrings() and aggregateFromContainers() - Corrects negative values to 0 with warning log - Logs warnings for suspiciously high values (> 1000) - Prevents logic errors in crash loop detection ### 5. Comprehensive Unit Tests - tests/Unit/StripCoolifyCustomFieldsTest.php (NEW) - 9 tests, 43 assertions - tests/Unit/ContainerStatusAggregatorTest.php - Added 6 tests for integer validation - tests/Unit/ExcludeFromHealthCheckTest.php - Added 4 tests for YAML validation - All tests passing with proper Log facade mocking ### 6. Documentation - Added comprehensive Docker Compose extensions documentation to .ai/core/deployment-architecture.md - Documents all custom fields: exclude_from_hc, content, isDirectory/is_directory - Includes examples, use cases, implementation details, and test references - Updated .ai/README.md with navigation links to new documentation ## Benefits - Better UX: Users can validate compose files with custom fields - Better Debugging: Comprehensive logging for errors - Better Code Quality: DRY principle with reusable validation - Better Reliability: Prevents logic errors from invalid parameters - Better Maintainability: Easy to add new custom fields in future 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Andras Bacsai
@@ -1,6 +1,7 @@
|
||||
<?php
|
||||
|
||||
use App\Services\ContainerStatusAggregator;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
|
||||
beforeEach(function () {
|
||||
$this->aggregator = new ContainerStatusAggregator;
|
||||
@@ -461,3 +462,79 @@ describe('state priority enforcement', function () {
|
||||
expect($result)->toBe('running:unknown');
|
||||
});
|
||||
});
|
||||
|
||||
describe('maxRestartCount validation', function () {
|
||||
test('negative maxRestartCount is corrected to 0 in aggregateFromStrings', function () {
|
||||
// Mock the Log facade to avoid "facade root not set" error in unit tests
|
||||
Log::shouldReceive('warning')->once();
|
||||
|
||||
$statuses = collect(['exited']);
|
||||
|
||||
// With negative value, should be treated as 0 (no restarts)
|
||||
$result = $this->aggregator->aggregateFromStrings($statuses, maxRestartCount: -5);
|
||||
|
||||
// Should return exited:unhealthy (not degraded) since corrected to 0
|
||||
expect($result)->toBe('exited:unhealthy');
|
||||
});
|
||||
|
||||
test('negative maxRestartCount is corrected to 0 in aggregateFromContainers', function () {
|
||||
// Mock the Log facade to avoid "facade root not set" error in unit tests
|
||||
Log::shouldReceive('warning')->once();
|
||||
|
||||
$containers = collect([
|
||||
[
|
||||
'State' => [
|
||||
'Status' => 'exited',
|
||||
'ExitCode' => 1,
|
||||
],
|
||||
],
|
||||
]);
|
||||
|
||||
// With negative value, should be treated as 0 (no restarts)
|
||||
$result = $this->aggregator->aggregateFromContainers($containers, maxRestartCount: -10);
|
||||
|
||||
// Should return exited:unhealthy (not degraded) since corrected to 0
|
||||
expect($result)->toBe('exited:unhealthy');
|
||||
});
|
||||
|
||||
test('zero maxRestartCount works correctly', function () {
|
||||
$statuses = collect(['exited']);
|
||||
|
||||
$result = $this->aggregator->aggregateFromStrings($statuses, maxRestartCount: 0);
|
||||
|
||||
// Zero is valid default - no crash loop detection
|
||||
expect($result)->toBe('exited:unhealthy');
|
||||
});
|
||||
|
||||
test('positive maxRestartCount works correctly', function () {
|
||||
$statuses = collect(['exited']);
|
||||
|
||||
$result = $this->aggregator->aggregateFromStrings($statuses, maxRestartCount: 5);
|
||||
|
||||
// Positive value enables crash loop detection
|
||||
expect($result)->toBe('degraded:unhealthy');
|
||||
});
|
||||
|
||||
test('crash loop detection still functions after validation', function () {
|
||||
$statuses = collect(['exited']);
|
||||
|
||||
// Test with various positive restart counts
|
||||
expect($this->aggregator->aggregateFromStrings($statuses, maxRestartCount: 1))
|
||||
->toBe('degraded:unhealthy');
|
||||
|
||||
expect($this->aggregator->aggregateFromStrings($statuses, maxRestartCount: 100))
|
||||
->toBe('degraded:unhealthy');
|
||||
|
||||
expect($this->aggregator->aggregateFromStrings($statuses, maxRestartCount: 999))
|
||||
->toBe('degraded:unhealthy');
|
||||
});
|
||||
|
||||
test('default maxRestartCount parameter works', function () {
|
||||
$statuses = collect(['exited']);
|
||||
|
||||
// Call without specifying maxRestartCount (should default to 0)
|
||||
$result = $this->aggregator->aggregateFromStrings($statuses);
|
||||
|
||||
expect($result)->toBe('exited:unhealthy');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -104,3 +104,51 @@ it('ensures UI handles excluded status in service heading buttons', function ()
|
||||
->toContain('str($service->status)->contains(\'degraded\')')
|
||||
->toContain('str($service->status)->contains(\'exited\')');
|
||||
});
|
||||
|
||||
/**
|
||||
* Unit tests for YAML validation in CalculatesExcludedStatus trait
|
||||
*/
|
||||
it('ensures YAML validation has proper exception handling for parse errors', function () {
|
||||
$traitFile = file_get_contents(__DIR__.'/../../app/Traits/CalculatesExcludedStatus.php');
|
||||
|
||||
// Verify that ParseException is imported and caught separately from generic Exception
|
||||
expect($traitFile)
|
||||
->toContain('use Symfony\Component\Yaml\Exception\ParseException')
|
||||
->toContain('use Illuminate\Support\Facades\Log')
|
||||
->toContain('} catch (ParseException $e) {')
|
||||
->toContain('} catch (\Exception $e) {');
|
||||
});
|
||||
|
||||
it('ensures YAML validation logs parse errors with context', function () {
|
||||
$traitFile = file_get_contents(__DIR__.'/../../app/Traits/CalculatesExcludedStatus.php');
|
||||
|
||||
// Verify that parse errors are logged with useful context (error message, line, snippet)
|
||||
expect($traitFile)
|
||||
->toContain('Log::warning(\'Failed to parse Docker Compose YAML for health check exclusions\'')
|
||||
->toContain('\'error\' => $e->getMessage()')
|
||||
->toContain('\'line\' => $e->getParsedLine()')
|
||||
->toContain('\'snippet\' => $e->getSnippet()');
|
||||
});
|
||||
|
||||
it('ensures YAML validation logs unexpected errors', function () {
|
||||
$traitFile = file_get_contents(__DIR__.'/../../app/Traits/CalculatesExcludedStatus.php');
|
||||
|
||||
// Verify that unexpected errors are logged with error level
|
||||
expect($traitFile)
|
||||
->toContain('Log::error(\'Unexpected error parsing Docker Compose YAML\'')
|
||||
->toContain('\'trace\' => $e->getTraceAsString()');
|
||||
});
|
||||
|
||||
it('ensures YAML validation checks structure after parsing', function () {
|
||||
$traitFile = file_get_contents(__DIR__.'/../../app/Traits/CalculatesExcludedStatus.php');
|
||||
|
||||
// Verify that parsed result is validated to be an array
|
||||
expect($traitFile)
|
||||
->toContain('if (! is_array($dockerCompose)) {')
|
||||
->toContain('Log::warning(\'Docker Compose YAML did not parse to array\'');
|
||||
|
||||
// Verify that services is validated to be an array
|
||||
expect($traitFile)
|
||||
->toContain('if (! is_array($services)) {')
|
||||
->toContain('Log::warning(\'Docker Compose services is not an array\'');
|
||||
});
|
||||
|
||||
229
tests/Unit/StripCoolifyCustomFieldsTest.php
Normal file
229
tests/Unit/StripCoolifyCustomFieldsTest.php
Normal file
@@ -0,0 +1,229 @@
|
||||
<?php
|
||||
|
||||
use function PHPUnit\Framework\assertEquals;
|
||||
|
||||
test('removes exclude_from_hc from service level', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'web' => [
|
||||
'image' => 'nginx:latest',
|
||||
'exclude_from_hc' => true,
|
||||
'ports' => ['80:80'],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
assertEquals('nginx:latest', $result['services']['web']['image']);
|
||||
assertEquals(['80:80'], $result['services']['web']['ports']);
|
||||
expect($result['services']['web'])->not->toHaveKey('exclude_from_hc');
|
||||
});
|
||||
|
||||
test('removes content from volume level', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'app' => [
|
||||
'image' => 'php:8.4',
|
||||
'volumes' => [
|
||||
[
|
||||
'type' => 'bind',
|
||||
'source' => './config.xml',
|
||||
'target' => '/app/config.xml',
|
||||
'content' => '<?xml version="1.0"?><config></config>',
|
||||
],
|
||||
],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
expect($result['services']['app']['volumes'][0])->toHaveKeys(['type', 'source', 'target']);
|
||||
expect($result['services']['app']['volumes'][0])->not->toHaveKey('content');
|
||||
});
|
||||
|
||||
test('removes isDirectory from volume level', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'app' => [
|
||||
'image' => 'node:20',
|
||||
'volumes' => [
|
||||
[
|
||||
'type' => 'bind',
|
||||
'source' => './data',
|
||||
'target' => '/app/data',
|
||||
'isDirectory' => true,
|
||||
],
|
||||
],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
expect($result['services']['app']['volumes'][0])->toHaveKeys(['type', 'source', 'target']);
|
||||
expect($result['services']['app']['volumes'][0])->not->toHaveKey('isDirectory');
|
||||
});
|
||||
|
||||
test('removes is_directory from volume level', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'app' => [
|
||||
'image' => 'python:3.12',
|
||||
'volumes' => [
|
||||
[
|
||||
'type' => 'bind',
|
||||
'source' => './logs',
|
||||
'target' => '/var/log/app',
|
||||
'is_directory' => true,
|
||||
],
|
||||
],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
expect($result['services']['app']['volumes'][0])->toHaveKeys(['type', 'source', 'target']);
|
||||
expect($result['services']['app']['volumes'][0])->not->toHaveKey('is_directory');
|
||||
});
|
||||
|
||||
test('removes all custom fields together', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'web' => [
|
||||
'image' => 'nginx:latest',
|
||||
'exclude_from_hc' => true,
|
||||
'volumes' => [
|
||||
[
|
||||
'type' => 'bind',
|
||||
'source' => './config.xml',
|
||||
'target' => '/etc/nginx/config.xml',
|
||||
'content' => '<config></config>',
|
||||
'isDirectory' => false,
|
||||
],
|
||||
[
|
||||
'type' => 'bind',
|
||||
'source' => './data',
|
||||
'target' => '/var/www/data',
|
||||
'is_directory' => true,
|
||||
],
|
||||
],
|
||||
],
|
||||
'worker' => [
|
||||
'image' => 'worker:latest',
|
||||
'exclude_from_hc' => true,
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
// Verify service-level custom fields removed
|
||||
expect($result['services']['web'])->not->toHaveKey('exclude_from_hc');
|
||||
expect($result['services']['worker'])->not->toHaveKey('exclude_from_hc');
|
||||
|
||||
// Verify volume-level custom fields removed
|
||||
expect($result['services']['web']['volumes'][0])->not->toHaveKey('content');
|
||||
expect($result['services']['web']['volumes'][0])->not->toHaveKey('isDirectory');
|
||||
expect($result['services']['web']['volumes'][1])->not->toHaveKey('is_directory');
|
||||
|
||||
// Verify standard fields preserved
|
||||
assertEquals('nginx:latest', $result['services']['web']['image']);
|
||||
assertEquals('worker:latest', $result['services']['worker']['image']);
|
||||
});
|
||||
|
||||
test('preserves standard Docker Compose fields', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'db' => [
|
||||
'image' => 'postgres:16',
|
||||
'environment' => [
|
||||
'POSTGRES_DB' => 'mydb',
|
||||
'POSTGRES_USER' => 'user',
|
||||
],
|
||||
'ports' => ['5432:5432'],
|
||||
'volumes' => [
|
||||
'db-data:/var/lib/postgresql/data',
|
||||
],
|
||||
'healthcheck' => [
|
||||
'test' => ['CMD', 'pg_isready'],
|
||||
'interval' => '5s',
|
||||
],
|
||||
'restart' => 'unless-stopped',
|
||||
'networks' => ['backend'],
|
||||
],
|
||||
],
|
||||
'networks' => [
|
||||
'backend' => [
|
||||
'driver' => 'bridge',
|
||||
],
|
||||
],
|
||||
'volumes' => [
|
||||
'db-data' => null,
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
// All standard fields should be preserved
|
||||
expect($result)->toHaveKeys(['services', 'networks', 'volumes']);
|
||||
expect($result['services']['db'])->toHaveKeys([
|
||||
'image', 'environment', 'ports', 'volumes',
|
||||
'healthcheck', 'restart', 'networks',
|
||||
]);
|
||||
assertEquals('postgres:16', $result['services']['db']['image']);
|
||||
assertEquals(['5432:5432'], $result['services']['db']['ports']);
|
||||
});
|
||||
|
||||
test('handles missing services gracefully', function () {
|
||||
$yaml = [
|
||||
'version' => '3.8',
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
expect($result)->toBe($yaml);
|
||||
});
|
||||
|
||||
test('handles missing volumes in service gracefully', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'app' => [
|
||||
'image' => 'nginx:latest',
|
||||
'exclude_from_hc' => true,
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
expect($result['services']['app'])->not->toHaveKey('exclude_from_hc');
|
||||
expect($result['services']['app'])->not->toHaveKey('volumes');
|
||||
assertEquals('nginx:latest', $result['services']['app']['image']);
|
||||
});
|
||||
|
||||
test('handles traccar.yaml example with multiline content', function () {
|
||||
$yaml = [
|
||||
'services' => [
|
||||
'traccar' => [
|
||||
'image' => 'traccar/traccar:latest',
|
||||
'volumes' => [
|
||||
[
|
||||
'type' => 'bind',
|
||||
'source' => './srv/traccar/conf/traccar.xml',
|
||||
'target' => '/opt/traccar/conf/traccar.xml',
|
||||
'content' => "<?xml version='1.0' encoding='UTF-8'?>\n<!DOCTYPE properties SYSTEM 'http://java.sun.com/dtd/properties.dtd'>\n<properties>\n <entry key='config.default'>./conf/default.xml</entry>\n</properties>",
|
||||
],
|
||||
],
|
||||
],
|
||||
],
|
||||
];
|
||||
|
||||
$result = stripCoolifyCustomFields($yaml);
|
||||
|
||||
expect($result['services']['traccar']['volumes'][0])->toHaveKeys(['type', 'source', 'target']);
|
||||
expect($result['services']['traccar']['volumes'][0])->not->toHaveKey('content');
|
||||
assertEquals('./srv/traccar/conf/traccar.xml', $result['services']['traccar']['volumes'][0]['source']);
|
||||
});
|
||||
Reference in New Issue
Block a user