fix(volumes): resolve env fallback paths for convert-to-file#9364
fix(volumes): resolve env fallback paths for convert-to-file#9364andrasbacsai wants to merge 5 commits intonextfrom
Conversation
Normalize `${VAR}` and `${VAR:-default}` volume sources via `resolveEnvVarDefault()`,
use preview env vars during application parsing, and re-resolve persisted file-volume
paths after environment-variable updates. Also surface env-var source info in the
file storage UI and add unit coverage for env-var/default resolution and path safety.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request enhances Docker volume path resolution by detecting and resolving environment variable references in file storage configurations. The changes introduce a new public property ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Livewire/Project/Service/FileStorage.php`:
- Around line 93-145: The loop variable $serviceVolumes in the
detectEnvVarSource() method is misleading because it holds the entire service
definition, not just volumes; rename it to $service (or $serviceDefinition) and
update any references inside the foreach to improve clarity (search for the
foreach over $compose['services'] and the subsequent data_get($serviceVolumes,
'volumes', []) usage), leaving the rest of the logic unchanged and keeping the
try/catch and Yaml::parse usage as-is.
In `@app/Models/LocalFileVolume.php`:
- Around line 52-56: resolvedFsPath() (and the other locations noted) currently
resolves legacy ${VAR} syntax without the resource's env collection and can
return the bare '.' fallback which later becomes $workdir in shell ops; change
resolvedFsPath() to first resolve against the owning resource’s env vars (use
the resource/env collection associated with this LocalFileVolume) and if
resolution yields the bare current-directory fallback ('.' or empty) throw a
hard error rather than returning it; likewise update callers
updateFileVolumePaths(), deleteStorageOnServer(), and saveStorageOnServer() to
rely on the env-aware resolved path and to fail early if the path is the bare
dot to avoid building any shell commands that could target $workdir.
- Around line 146-149: The code in LocalFileVolume::reResolveVolumePaths (the
$mainDirectory assignment) hardcodes '/applications/' which mismatches
serviceParser()'s v4+ storage root and causes volume paths to be repointed
incorrectly; change $mainDirectory to use the same service storage root as
serviceParser() (e.g. build the path with
base_configuration_dir().'/services/'.$service->uuid or call the shared helper
used by serviceParser()) so both lookup and write paths are consistent.
In `@bootstrap/helpers/parsers.php`:
- Around line 292-304: The code currently replaces "${VAR:-default}" with the
default value early (in the preg_match block operating on $source), which
prevents applicationParser()/serviceParser() and $allEnvironments from resolving
real or preview env values; change the logic so that when preg_match finds a
pattern with ':-' you do NOT overwrite $source with the default string — instead
preserve the full original short-syntax token (e.g. keep '${VAR:-default}') so
later env-aware resolution in applicationParser() / serviceParser() can apply
overrides from $allEnvironments; only convert to a plain '${VAR}' when the
default part is explicitly empty, and add a small regression test that supplies
an overriding env value to ensure the preserved short syntax picks the
env-provided value rather than the static default.
- Around line 708-709: resolveEnvVarDefault() can return a bare "." which
sourceIsLocal() doesn't treat as local, causing bind sources to be misclassified
as named volumes; after each call to resolveEnvVarDefault (e.g., where $source
is set) normalize a bare "." to "./" (e.g., if ($source === '.') $source = './')
so the later sourceIsLocal() / LocalFileVolume logic picks the bind-mount
branch; alternatively, update sourceIsLocal() to return true for "." — apply
this normalization in the same spots where resolveEnvVarDefault() is used (the
occurrences you noted) so the current-directory fallback becomes a
LocalFileVolume.
In `@bootstrap/helpers/shared.php`:
- Around line 1237-1270: The code in resolveEnvVarDefault uses
strpos($varContent, ':-') !== false to detect the default separator; replace
that check with PHP 8's str_contains($varContent, ':-') for consistency with the
codebase style, leaving the subsequent explode(':-', $varContent, 2) and
variable assignment logic ($varName, $defaultValue) intact and preserving
behavior when $environmentVariables is null or when defaults are empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbb9037a-e832-4b3b-b707-2979a9c501b6
📒 Files selected for processing (8)
app/Livewire/Project/Service/FileStorage.phpapp/Livewire/Project/Shared/EnvironmentVariable/All.phpapp/Livewire/Project/Shared/EnvironmentVariable/Show.phpapp/Models/LocalFileVolume.phpbootstrap/helpers/parsers.phpbootstrap/helpers/shared.phpresources/views/livewire/project/service/file-storage.blade.phptests/Unit/VolumeArrayFormatSecurityTest.php
app/Models/LocalFileVolume.php
Outdated
| public function resolvedFsPath(): string | ||
| { | ||
| $resolved = resolveEnvVarDefault(str($this->fs_path)); | ||
|
|
||
| return $resolved->value(); |
There was a problem hiding this comment.
Do not turn an unresolved env var into $workdir.
resolvedFsPath() resolves legacy ${VAR...} without the owning env collection, so it can only produce the fallback or . — never the actual resource value. updateFileVolumePaths() also skips that bare-dot result, and the downstream file ops expand it to $workdir; deleteStorageOnServer() can rm -rf the whole workdir, and saveStorageOnServer() can replace it with a file. That is less “convert to file” and more “hasta la vista, app data.” Please resolve against the related resource’s env vars first, and hard-fail on the bare current-directory fallback before any shell command is built.
Also applies to: 201-203, 223-227, 258-279, 296-325, 337-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Models/LocalFileVolume.php` around lines 52 - 56, resolvedFsPath() (and
the other locations noted) currently resolves legacy ${VAR} syntax without the
resource's env collection and can return the bare '.' fallback which later
becomes $workdir in shell ops; change resolvedFsPath() to first resolve against
the owning resource’s env vars (use the resource/env collection associated with
this LocalFileVolume) and if resolution yields the bare current-directory
fallback ('.' or empty) throw a hard error rather than returning it; likewise
update callers updateFileVolumePaths(), deleteStorageOnServer(), and
saveStorageOnServer() to rely on the env-aware resolved path and to fail early
if the path is the bare dot to avoid building any shell commands that could
target $workdir.
bootstrap/helpers/parsers.php
Outdated
| // Resolve env vars in source (e.g., ${VAR} -> value from Coolify env vars) | ||
| $source = resolveEnvVarDefault($source, $allEnvironments); |
There was a problem hiding this comment.
Treat bare . as a local bind source.
resolveEnvVarDefault() returns . for ${VAR} / ${VAR:-} when nothing is set, but sourceIsLocal() does not recognize bare dot. These paths fall into the named-volume branch instead of the bind-mount branch, so the current-directory fallback never becomes a LocalFileVolume. Tiny dot, giant self-hosted headache. Normalizing . to ./ here, or teaching sourceIsLocal() about bare dot, would keep the fallback on the correct path.
Also applies to: 734-737, 2101-2102, 2127-2130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bootstrap/helpers/parsers.php` around lines 708 - 709, resolveEnvVarDefault()
can return a bare "." which sourceIsLocal() doesn't treat as local, causing bind
sources to be misclassified as named volumes; after each call to
resolveEnvVarDefault (e.g., where $source is set) normalize a bare "." to "./"
(e.g., if ($source === '.') $source = './') so the later sourceIsLocal() /
LocalFileVolume logic picks the bind-mount branch; alternatively, update
sourceIsLocal() to return true for "." — apply this normalization in the same
spots where resolveEnvVarDefault() is used (the occurrences you noted) so the
current-directory fallback becomes a LocalFileVolume.
Keep `${VAR:-default}` intact in volume parsing so env overrides can be
resolved later, then re-resolve local file volume paths after env var updates
or deletions.
Also treat `.` as a local source path, update file-storage UI copy/actions for
read-only mounts, and add regression/unit tests for parser, source locality,
and service volume path handling.
Switch `wireNavigate()` to return `wire:navigate` when enabled, including the exception fallback path. Add a unit test to assert the helper no longer contains `wire:navigate.hover`.
Return unresolved ${VAR} references unchanged in resolveEnvVarDefault
instead of coercing them to ".", so sourceIsLocal() does not classify
missing env vars as local bind mounts.
Also hard-fail LocalFileVolume::resolvedFsPath() when unresolved placeholders
remain in the resolved value, and update unit tests to cover the regression
and expected local-default behavior.
Switch default parser version to v6 for Application and Service, and only
preserve/resolve `${VAR:-default}` sources in v6 parsing flows.
Also update file storage path re-resolution to run for v6 resources and
reclassify volumes between LocalFileVolume and LocalPersistentVolume when
env var resolution changes.
1 participant
Summary
${VAR:-./path}before storage-path validation and file operations.resolveEnvVarDefault()and applies it in volume parsing (applicationParser,serviceParser) andLocalFileVolumepath handling (save/load/delete).LocalFileVolume::reResolveVolumePaths()and triggers it after env var updates so file volume paths stay in sync when env values change.LocalFileVolume::resolvedFsPath().Fixes #8854