refactor: remove CoolifyResource model, derive app UUID from API#49
refactor: remove CoolifyResource model, derive app UUID from API#49
Conversation
The webhook secret is already stored on Coolify via the API and displayed for the user to copy to GitHub. Storing it in the local database was pointless and a potential security concern. - Remove webhook_secret from CoolifyResource model - Remove migration that added webhook_secret column - Secret is still generated, set on Coolify, and displayed in output Stu Mason + AI <me@stumason.dev>
BREAKING CHANGE: Removes database storage for Coolify resource info. Application UUIDs are now derived from the Coolify API by matching the current git repository. Changes: - Remove CoolifyResource model and migration - Add Coolify::getApplicationUuid() that fetches apps from API - Add Coolify::getCurrentGitRepository() helper - Update ProvisionCommand to write COOLIFY_PROJECT_UUID to .env - Update all commands to use Coolify::getApplicationUuid() - Update DashboardStatsController to derive from API - Simplify EnvironmentController (multi-env support removed) - Update all tests for new architecture
Code ReviewThis is a significant architectural refactor that removes the database-backed storage model in favor of deriving application UUIDs from the Coolify API. Overall, this is a well-executed change with good test coverage. Here's my detailed feedback: ✅ Strengths
🔍 Code Quality IssuesCritical: Security - Command Injection RiskLocation: $result = Process::run('git remote get-url origin 2>/dev/null');Issue: While this appears safe (no user input), the code runs in the application's working directory which could be influenced by deployment configuration. Recommendation: Add validation to ensure we're in a git repository: public static function getCurrentGitRepository(): ?string
{
// Validate we're in a git repo first
if (\!is_dir(base_path('.git'))) {
return null;
}
$result = Process::run('git remote get-url origin 2>/dev/null');
// ... rest of code
}High: Performance - N+1 API CallsLocation: $applications = static::applications()->all();
foreach ($applications as $app) {
// Match repository
}Issue: Impact:
Recommendation:
Medium: .env File Manipulation IssuesLocation: protected function updateEnvFile(string , string ): void
{
// ...
if (preg_match("/^{}=.*/m", $content)) {
$content = preg_replace("/^{}=.*/m", "{}={}", $content);
}
// ...
}Issues:
Recommendation: Use Laravel's built-in DotenvEditor or add proper escaping: protected function updateEnvFile(string $key, string $value): void
{
$envPath = base_path('.env');
$key = preg_quote($key, '/');
if (\!File::exists($envPath)) {
File::put($envPath, "{$key}={$value}\n");
return;
}
$content = File::get($envPath);
// Only match uncommented lines
if (preg_match("/^{$key}=.*/m", $content)) {
$content = preg_replace("/^{$key}=.*/m", "{$key}={$value}", $content);
} else {
$content = rtrim($content, "\n") . "\n{$key}={$value}\n";
}
// Atomic write
$tempPath = $envPath . '.tmp';
File::put($tempPath, $content);
File::move($tempPath, $envPath);
}Medium: Git Repository NormalizationLocation: $normalizedAppRepo = preg_replace('/\.git$/', '', $appRepo);
$normalizedAppRepo = preg_replace('#^git@github\.com:#', '', $normalizedAppRepo);
$normalizedAppRepo = preg_replace('#^https?://github\.com/#', '', $normalizedAppRepo);Issues:
Recommendation: protected static function normalizeGitUrl(string $url): string
{
// Remove .git suffix
$url = preg_replace('/\.git$/', '', $url);
// Extract owner/repo from various formats
// git@host:owner/repo or git@host:port/owner/repo
if (preg_match('#^git@[^:]+:(\d+/)?(.+)$#', $url, $matches)) {
return $matches[2];
}
// https://host/owner/repo or http://host/owner/repo
if (preg_match('#^https?://[^/]+/(.+)$#', $url, $matches)) {
return $matches[1];
}
// ssh://git@host/owner/repo
if (preg_match('#^ssh://[^/]+/(.+)$#', $url, $matches)) {
return $matches[1];
}
return $url;
}Low: Missing Error ContextLocation: } catch (CoolifyApiException) {
// Application not found
}Issue: Silent failures make debugging difficult. Users won't know if the app wasn't found vs API is down vs authentication failed. Recommendation: Log errors at minimum: } catch (CoolifyApiException $e) {
Log::warning('Failed to fetch application from Coolify', [
'app_uuid' => $appUuid,
'error' => $e->getMessage(),
]);
}Low: Unintended File in PRLocation: {"voice": "nova", "speed": 1.1, "enabled": true}Issue: This appears to be a personal configuration file that shouldn't be in version control. Recommendation:
🔒 Security ConcernsCache Poisoning RiskLocation: The cache key is only based on $cacheKey = "coolify.app_uuid.{$projectUuid}";Issue: If git repository changes (e.g., rebasing, switching repos), the cached UUID remains for up to 30 seconds, potentially deploying to wrong application. Recommendation: Include git remote URL in cache key: $gitRepo = static::getCurrentGitRepository();
$cacheKey = "coolify.app_uuid.{$projectUuid}." . md5($gitRepo ?? 'none');🎯 Test Coverage Concerns
Recommendation: Add unit tests: it('extracts GitHub repository from git remote', function () {
// Mock Process facade to return git@github.com:StuMason/laravel-coolify.git
$result = Coolify::getCurrentGitRepository();
expect($result)->toBe('StuMason/laravel-coolify');
});
it('returns null when not in git repository', function () {
// Mock Process failure
$result = Coolify::getCurrentGitRepository();
expect($result)->toBeNull();
});📊 Performance Considerations
Recommendations:
🔄 Breaking Change HandlingThis PR correctly identifies this as a BREAKING CHANGE. However:
Recommendation: Add to PR description: ## Upgrade Guide
1. Note your existing `COOLIFY_PROJECT_UUID` from database or Coolify dashboard
2. Add to `.env`: `COOLIFY_PROJECT_UUID=your-uuid-here`
3. Run: `composer update`
4. Optional cleanup: Drop `coolify_resources` table✨ Minor Code Style
📝 Recommendations SummaryMust Fix:
Should Fix:
Consider:
VerdictThis is a well-designed refactor that simplifies the architecture significantly. The core idea is sound and the implementation is mostly solid. However, there are several edge cases in .env manipulation and git URL parsing that need attention before merging. Recommendation: Request changes for the critical issues (especially .env handling and the personal config file), then merge after fixes. Great work on maintaining test coverage through this large refactor! 🎉 |
- Remove all references to coolify_resources database table - Document COOLIFY_PROJECT_UUID being set automatically by provision - Explain automatic application lookup by git repository matching - Update config reference with new options
- Add null validation in deploy(), status(), logs() methods - Fix database category overwrites (multiple DBs of same type) - Add docblock noting GitHub-only support for git repository detection
- Remove .claude/ from git and add to .gitignore - Add .git directory validation before running git commands - Include git repository in cache key to prevent stale cache - Improve .env file manipulation with proper regex escaping and atomic writes - Fix dashboard test to properly mock git repository lookup
|
@claude please re-review |
|
Claude finished @StuMason's task in 1m 47s —— View job Re-Review Complete ✅
SummaryGreat work addressing the feedback from my previous review! I've verified that all 230 tests pass and lint is clean. Here's the status of the previously identified issues: ✅ Issues Resolved1.
|
Summary
COOLIFY_PROJECT_UUIDis stored in.env, everything else is fetched from APIChanges
CoolifyResourcemodel and migrationCoolify::getApplicationUuid()that fetches apps from API and matches by git repoCoolify::getCurrentGitRepository()helperProvisionCommandto writeCOOLIFY_PROJECT_UUIDto.envinstead of databaseDeployCommand,LogsCommand,StatusCommand,RestartCommand,RollbackCommand) to useCoolify::getApplicationUuid()DashboardStatsControllerto derive app info from APIEnvironmentController(multi-environment support removed)Why
The previous approach of storing Coolify details in a local database was flawed because:
migrate:fresh --seedwould wipe the Coolify configuration.envand fetch everything elseTest plan