From 36f8a58c281e461cf75750f6cf57ae5afd52a3ba Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 18 Nov 2025 09:46:04 +0100 Subject: [PATCH] refactor: move buildpack cleanup logic to model lifecycle hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move buildpack switching cleanup from Livewire component to Application model's boot lifecycle. This improves separation of concerns and ensures cleanup happens consistently regardless of how the buildpack change is triggered. Also clears Dockerfile-specific data when switching away from dockerfile buildpack. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- app/Livewire/Project/Application/General.php | 15 +- app/Models/Application.php | 30 +++ ...ile_data_for_non_dockerfile_buildpacks.php | 31 +++ .../ApplicationBuildpackCleanupTest.php | 183 ++++++++++++++++++ tests/Feature/BuildpackSwitchCleanupTest.php | 134 +++++++++++++ 5 files changed, 379 insertions(+), 14 deletions(-) create mode 100644 database/migrations/2025_11_18_083747_cleanup_dockerfile_data_for_non_dockerfile_buildpacks.php create mode 100644 tests/Feature/ApplicationBuildpackCleanupTest.php create mode 100644 tests/Feature/BuildpackSwitchCleanupTest.php diff --git a/app/Livewire/Project/Application/General.php b/app/Livewire/Project/Application/General.php index ce7d6b1b7..16733a298 100644 --- a/app/Livewire/Project/Application/General.php +++ b/app/Livewire/Project/Application/General.php @@ -622,6 +622,7 @@ public function updatedIsStatic($value) public function updatedBuildPack() { + $originalBuildPack = $this->application->getOriginal('build_pack'); // Check if user has permission to update try { $this->authorize('update', $this->application); @@ -641,8 +642,6 @@ public function updatedBuildPack() $this->application->settings->is_static = false; $this->application->settings->save(); } else { - $this->portsExposes = '3000'; - $this->application->ports_exposes = '3000'; $this->resetDefaultLabels(false); } if ($this->buildPack === 'dockercompose') { @@ -655,18 +654,6 @@ public function updatedBuildPack() } catch (\Illuminate\Auth\Access\AuthorizationException $e) { // User doesn't have update permission, just continue without saving } - } else { - // Clear Docker Compose specific data when switching away from dockercompose - if ($this->application->getOriginal('build_pack') === 'dockercompose') { - $this->application->docker_compose_domains = null; - $this->application->docker_compose_raw = null; - - // Remove SERVICE_FQDN_* and SERVICE_URL_* environment variables - $this->application->environment_variables()->where('key', 'LIKE', 'SERVICE_FQDN_%')->delete(); - $this->application->environment_variables()->where('key', 'LIKE', 'SERVICE_URL_%')->delete(); - $this->application->environment_variables_preview()->where('key', 'LIKE', 'SERVICE_FQDN_%')->delete(); - $this->application->environment_variables_preview()->where('key', 'LIKE', 'SERVICE_URL_%')->delete(); - } } if ($this->buildPack === 'static') { $this->portsExposes = '80'; diff --git a/app/Models/Application.php b/app/Models/Application.php index 5e2aaa347..306c9bd7a 100644 --- a/app/Models/Application.php +++ b/app/Models/Application.php @@ -176,6 +176,36 @@ protected static function booted() if (count($payload) > 0) { $application->forceFill($payload); } + + // Buildpack switching cleanup logic + if ($application->isDirty('build_pack')) { + $originalBuildPack = $application->getOriginal('build_pack'); + + // Clear Docker Compose specific data when switching away from dockercompose + if ($originalBuildPack === 'dockercompose') { + $application->docker_compose_domains = null; + $application->docker_compose_raw = null; + + // Remove SERVICE_FQDN_* and SERVICE_URL_* environment variables + $application->environment_variables() + ->where('key', 'LIKE', 'SERVICE_FQDN_%') + ->orWhere('key', 'LIKE', 'SERVICE_URL_%') + ->delete(); + + $application->environment_variables_preview() + ->where('key', 'LIKE', 'SERVICE_FQDN_%') + ->orWhere('key', 'LIKE', 'SERVICE_URL_%') + ->delete(); + } + + // Clear Dockerfile specific data when switching away from dockerfile + if ($originalBuildPack === 'dockerfile') { + $application->dockerfile = null; + $application->dockerfile_location = null; + $application->dockerfile_target_build = null; + $application->custom_healthcheck_found = false; + } + } }); static::created(function ($application) { ApplicationSetting::create([ diff --git a/database/migrations/2025_11_18_083747_cleanup_dockerfile_data_for_non_dockerfile_buildpacks.php b/database/migrations/2025_11_18_083747_cleanup_dockerfile_data_for_non_dockerfile_buildpacks.php new file mode 100644 index 000000000..959662cd5 --- /dev/null +++ b/database/migrations/2025_11_18_083747_cleanup_dockerfile_data_for_non_dockerfile_buildpacks.php @@ -0,0 +1,31 @@ +where('build_pack', '!=', 'dockerfile') + ->update([ + 'dockerfile' => null, + 'dockerfile_location' => null, + 'dockerfile_target_build' => null, + 'custom_healthcheck_found' => false, + ]); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + // No rollback needed - we're cleaning up corrupt data + } +}; diff --git a/tests/Feature/ApplicationBuildpackCleanupTest.php b/tests/Feature/ApplicationBuildpackCleanupTest.php new file mode 100644 index 000000000..b6b535a76 --- /dev/null +++ b/tests/Feature/ApplicationBuildpackCleanupTest.php @@ -0,0 +1,183 @@ +create(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'build_pack' => 'dockerfile', + 'dockerfile' => 'FROM node:18\nHEALTHCHECK CMD curl -f http://localhost/ || exit 1', + 'dockerfile_location' => '/Dockerfile', + 'dockerfile_target_build' => 'production', + 'custom_healthcheck_found' => true, + ]); + + // Change buildpack to nixpacks + $application->build_pack = 'nixpacks'; + $application->save(); + + // Reload from database + $application->refresh(); + + // Verify dockerfile fields were cleared + expect($application->build_pack)->toBe('nixpacks'); + expect($application->dockerfile)->toBeNull(); + expect($application->dockerfile_location)->toBeNull(); + expect($application->dockerfile_target_build)->toBeNull(); + expect($application->custom_healthcheck_found)->toBeFalse(); + }); + + test('model clears dockerfile fields when build_pack changes from dockerfile to static', function () { + $team = Team::factory()->create(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'build_pack' => 'dockerfile', + 'dockerfile' => 'FROM nginx:alpine', + 'dockerfile_location' => '/custom.Dockerfile', + 'dockerfile_target_build' => 'prod', + 'custom_healthcheck_found' => true, + ]); + + $application->build_pack = 'static'; + $application->save(); + $application->refresh(); + + expect($application->build_pack)->toBe('static'); + expect($application->dockerfile)->toBeNull(); + expect($application->dockerfile_location)->toBeNull(); + expect($application->dockerfile_target_build)->toBeNull(); + expect($application->custom_healthcheck_found)->toBeFalse(); + }); + + test('model clears dockercompose fields when build_pack changes from dockercompose to nixpacks', function () { + $team = Team::factory()->create(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'build_pack' => 'dockercompose', + 'docker_compose_domains' => '{"app": "example.com"}', + 'docker_compose_raw' => 'version: "3.8"\nservices:\n app:\n image: nginx', + ]); + + // Add environment variables that should be deleted + EnvironmentVariable::create([ + 'application_id' => $application->id, + 'key' => 'SERVICE_FQDN_APP', + 'value' => 'app.example.com', + 'is_build_time' => false, + 'is_preview' => false, + ]); + + EnvironmentVariable::create([ + 'application_id' => $application->id, + 'key' => 'SERVICE_URL_APP', + 'value' => 'http://app.example.com', + 'is_build_time' => false, + 'is_preview' => false, + ]); + + EnvironmentVariable::create([ + 'application_id' => $application->id, + 'key' => 'REGULAR_VAR', + 'value' => 'should_remain', + 'is_build_time' => false, + 'is_preview' => false, + ]); + + $application->build_pack = 'nixpacks'; + $application->save(); + $application->refresh(); + + expect($application->build_pack)->toBe('nixpacks'); + expect($application->docker_compose_domains)->toBeNull(); + expect($application->docker_compose_raw)->toBeNull(); + + // Verify SERVICE_FQDN_* and SERVICE_URL_* were deleted + expect($application->environment_variables()->where('key', 'SERVICE_FQDN_APP')->count())->toBe(0); + expect($application->environment_variables()->where('key', 'SERVICE_URL_APP')->count())->toBe(0); + + // Verify regular variables remain + expect($application->environment_variables()->where('key', 'REGULAR_VAR')->count())->toBe(1); + }); + + test('model does not clear dockerfile fields when switching to dockerfile', function () { + $team = Team::factory()->create(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'build_pack' => 'nixpacks', + 'dockerfile' => null, + ]); + + $application->build_pack = 'dockerfile'; + $application->save(); + $application->refresh(); + + // When switching TO dockerfile, no cleanup should happen + expect($application->build_pack)->toBe('dockerfile'); + }); + + test('model does not clear fields when switching between non-dockerfile buildpacks', function () { + $team = Team::factory()->create(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'build_pack' => 'nixpacks', + 'dockerfile' => null, + 'dockerfile_location' => null, + ]); + + $application->build_pack = 'static'; + $application->save(); + $application->refresh(); + + expect($application->build_pack)->toBe('static'); + expect($application->dockerfile)->toBeNull(); + }); + + test('model does not trigger cleanup when build_pack is not changed', function () { + $team = Team::factory()->create(); + $project = Project::factory()->create(['team_id' => $team->id]); + $environment = Environment::factory()->create(['project_id' => $project->id]); + + $application = Application::factory()->create([ + 'environment_id' => $environment->id, + 'build_pack' => 'dockerfile', + 'dockerfile' => 'FROM alpine:latest', + 'dockerfile_location' => '/Dockerfile', + 'custom_healthcheck_found' => true, + ]); + + // Update another field without changing build_pack + $application->name = 'Updated Name'; + $application->save(); + $application->refresh(); + + // Dockerfile fields should remain unchanged + expect($application->build_pack)->toBe('dockerfile'); + expect($application->dockerfile)->toBe('FROM alpine:latest'); + expect($application->dockerfile_location)->toBe('/Dockerfile'); + expect($application->custom_healthcheck_found)->toBeTrue(); + }); +}); diff --git a/tests/Feature/BuildpackSwitchCleanupTest.php b/tests/Feature/BuildpackSwitchCleanupTest.php new file mode 100644 index 000000000..b040f9a8f --- /dev/null +++ b/tests/Feature/BuildpackSwitchCleanupTest.php @@ -0,0 +1,134 @@ +team = Team::factory()->create(); + $this->user = User::factory()->create(); + $this->team->members()->attach($this->user->id, ['role' => 'owner']); + + // Set current team + $this->actingAs($this->user); + session(['currentTeam' => $this->team]); + + // Create project and environment + $this->project = Project::factory()->create(['team_id' => $this->team->id]); + $this->environment = Environment::factory()->create(['project_id' => $this->project->id]); +}); + +describe('Buildpack Switching Cleanup', function () { + test('clears dockerfile fields when switching from dockerfile to nixpacks', function () { + // Create an application with dockerfile buildpack and dockerfile content + $application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'build_pack' => 'dockerfile', + 'dockerfile' => 'FROM node:18\nHEALTHCHECK CMD curl -f http://localhost/ || exit 1', + 'dockerfile_location' => '/Dockerfile', + 'dockerfile_target_build' => 'production', + 'custom_healthcheck_found' => true, + ]); + + // Switch to nixpacks buildpack + Livewire::test(General::class, ['application' => $application]) + ->assertSuccessful() + ->set('buildPack', 'nixpacks') + ->call('updatedBuildPack'); + + // Verify dockerfile fields were cleared + $application->refresh(); + expect($application->build_pack)->toBe('nixpacks'); + expect($application->dockerfile)->toBeNull(); + expect($application->dockerfile_location)->toBeNull(); + expect($application->dockerfile_target_build)->toBeNull(); + expect($application->custom_healthcheck_found)->toBeFalse(); + }); + + test('clears dockerfile fields when switching from dockerfile to static', function () { + $application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'build_pack' => 'dockerfile', + 'dockerfile' => 'FROM nginx:alpine', + 'dockerfile_location' => '/custom.Dockerfile', + 'dockerfile_target_build' => 'prod', + 'custom_healthcheck_found' => true, + ]); + + Livewire::test(General::class, ['application' => $application]) + ->assertSuccessful() + ->set('buildPack', 'static') + ->call('updatedBuildPack'); + + $application->refresh(); + expect($application->build_pack)->toBe('static'); + expect($application->dockerfile)->toBeNull(); + expect($application->dockerfile_location)->toBeNull(); + expect($application->dockerfile_target_build)->toBeNull(); + expect($application->custom_healthcheck_found)->toBeFalse(); + }); + + test('does not clear dockerfile fields when switching to dockerfile', function () { + $application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'build_pack' => 'nixpacks', + 'dockerfile' => null, + ]); + + Livewire::test(General::class, ['application' => $application]) + ->assertSuccessful() + ->set('buildPack', 'dockerfile') + ->call('updatedBuildPack'); + + // When switching TO dockerfile, fields remain as they were + $application->refresh(); + expect($application->build_pack)->toBe('dockerfile'); + }); + + test('does not affect fields when switching between non-dockerfile buildpacks', function () { + $application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'build_pack' => 'nixpacks', + 'dockerfile' => null, + 'dockerfile_location' => null, + ]); + + Livewire::test(General::class, ['application' => $application]) + ->assertSuccessful() + ->set('buildPack', 'static') + ->call('updatedBuildPack'); + + $application->refresh(); + expect($application->build_pack)->toBe('static'); + expect($application->dockerfile)->toBeNull(); + }); + + test('clears dockerfile fields when switching from dockerfile to dockercompose', function () { + $application = Application::factory()->create([ + 'environment_id' => $this->environment->id, + 'build_pack' => 'dockerfile', + 'dockerfile' => 'FROM alpine:latest', + 'dockerfile_location' => '/docker/Dockerfile', + 'custom_healthcheck_found' => true, + ]); + + Livewire::test(General::class, ['application' => $application]) + ->assertSuccessful() + ->set('buildPack', 'dockercompose') + ->call('updatedBuildPack'); + + $application->refresh(); + expect($application->build_pack)->toBe('dockercompose'); + expect($application->dockerfile)->toBeNull(); + expect($application->dockerfile_location)->toBeNull(); + expect($application->custom_healthcheck_found)->toBeFalse(); + }); +});