Fix: prevent SERVICE_NAME with dashes from breaking DotEnv parsing #7215
Conversation
Elandlord
changed the title
Elandlord
changed the title
|
@CodeRabbit review! |
|
@ShadowArcanist: I'll review the changes in this PR right away! ✅ Actions performedReview triggered.
|
WalkthroughThe pull request modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/Jobs/ApplicationDeploymentJob.php (2)
1178-1183: Apply the same sanitization to preview deployments.This code generates SERVICE_NAME variables for preview/PR deployments but lacks the sanitization applied at line 1128. Service names with dashes or dots will still break DotEnv parsers for preview deployments.
Apply this diff to maintain consistency:
$rawDockerCompose = Yaml::parse($this->application->docker_compose_raw); $rawServices = data_get($rawDockerCompose, 'services', []); foreach ($rawServices as $rawServiceName => $_) { - $envs->push('SERVICE_NAME_'.str($rawServiceName)->upper().'='.addPreviewDeploymentSuffix($rawServiceName, $this->pull_request_id)); + $envs->push('SERVICE_NAME_'.str($rawServiceName)->replace('-', '_')->replace('.', '_')->upper().'='.addPreviewDeploymentSuffix($rawServiceName, $this->pull_request_id)); }
1340-1390: Apply the same sanitization to build-time SERVICE_NAME variables.The
generate_buildtime_environment_variables()method generates SERVICE_NAME variables at lines 1351 and 1373, but both lack the dash/dot sanitization. This will cause DotEnv parser failures during the build phase when services have dashes or dots in their names.Apply this diff:
$services = data_get($dockerCompose, 'services', []); foreach ($services as $serviceName => $_) { - $envs->push('SERVICE_NAME_'.str($serviceName)->upper().'='.escapeBashEnvValue($serviceName)); + $envs->push('SERVICE_NAME_'.str($serviceName)->replace('-', '_')->replace('.', '_')->upper().'='.escapeBashEnvValue($serviceName)); } // ... later in the preview section ... $rawServices = data_get($rawDockerCompose, 'services', []); foreach ($rawServices as $rawServiceName => $_) { - $envs->push('SERVICE_NAME_'.str($rawServiceName)->upper().'='.escapeBashEnvValue(addPreviewDeploymentSuffix($rawServiceName, $this->pull_request_id))); + $envs->push('SERVICE_NAME_'.str($rawServiceName)->replace('-', '_')->replace('.', '_')->upper().'='.escapeBashEnvValue(addPreviewDeploymentSuffix($rawServiceName, $this->pull_request_id))); }
🧹 Nitpick comments (2)
app/Jobs/ApplicationDeploymentJob.php (2)
1128-1128: Good fix, but consider collision handling.The sanitization correctly prevents DotEnv parser breakage by ensuring SERVICE_NAME keys are valid shell identifiers. The original service name is properly preserved in the value.
However, there's a potential collision risk: services with names differing only by dashes/dots (e.g.,
my-serviceandmy.service) would both map toSERVICE_NAME_MY_SERVICE, causing the latter to silently overwrite the former.Consider adding collision detection:
$sanitizedKey = str($serviceName)->replace('-', '_')->replace('.', '_')->upper(); $envKey = 'SERVICE_NAME_'.$sanitizedKey; if ($envs->contains(fn($env) => str($env)->startsWith($envKey.'='))) { $this->application_deployment_queue->addLogEntry("Warning: SERVICE_NAME collision detected for service '{$serviceName}' (sanitized to {$sanitizedKey})", 'warning'); } $envs->push($envKey.'='.$serviceName);
1107-1117: Consider sanitizing SERVICE_FQDN and SERVICE_URL keys for consistency.For consistency with the SERVICE_NAME fix, consider applying the same sanitization to SERVICE_FQDN and SERVICE_URL variable keys. While these derive from the
docker_compose_domainsconfiguration rather than direct service names, they may still contain dashes or dots if the domain keys match service names.Example for line 1115-1116:
- $envs->push('SERVICE_URL_'.str($forServiceName)->upper().'='.$coolifyUrl->__toString()); - $envs->push('SERVICE_FQDN_'.str($forServiceName)->upper().'='.$coolifyFqdn); + $envs->push('SERVICE_URL_'.str($forServiceName)->replace('-', '_')->replace('.', '_')->upper().'='.$coolifyUrl->__toString()); + $envs->push('SERVICE_FQDN_'.str($forServiceName)->replace('-', '_')->replace('.', '_')->upper().'='.$coolifyFqdn);Apply similar changes to lines 1173-1174 for preview deployments.
Also applies to: 1165-1175
|
@ShadowArcanist I have added the changes requested by CodeRabbit |
2 participants
Changes
ApplicationDeploymentJob.php@generate_runtime_environment_variables()the recently addedSERVICE_NAME.envvalue can contain dashes, causing dotEnv parsers to break on deployment. In the linked issue, people are mentioning both Symfony and Bun apps breaking. Upon inspection, I saw that the service name does not take into account characters like-or.which causes the parser to break upon deployment.In order to fix it, I have added the replace functions in the exact way they are added in
App\Models\Service.phpfor example.Issues
Example:
Summary by CodeRabbit