Repository navigation
fix(git): fixes for Copilot's review of #172 - #175
Merged
Merged
Conversation
The path guard dropped RAM mounts before picking the deepest mount that holds a path, so a tmpfs mounted inside a pool (a RAM transcode folder in appdata, say) was taken for the pool: a clones root there was accepted and clones written to RAM. findMountFor() now picks the deepest mount from the full table and returns null when that one is in RAM. Unassigned Devices' tmpfs over /mnt/disks is still refused as before. A deploy's bind mount check still accepts a folder on such a tmpfs, which the user mounted to be in RAM; a missing one there is left for Docker to create, as below an existing share. Found by Copilot on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With Create Missing External Networks on, a git deploy's checks let a missing external network through, for the deploy to create just before up. When that creation failed, it was only logged and up ran anyway: up then failed before touching a container, but as a failed up, so the commit was recorded as failed and the previous one was not put back. create_missing_external_networks now tries every network and returns 1 if any could not be created. gitdeploy stops there and puts the previous commit back, as it does for a failed pull or build. Start and Update ignore the result, as before. Found by Copilot on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A git deploy pulls and builds as steps of their own, so that a failure there puts the previous commit back. But up still followed each service's pull_policy: "build" built again and "always" pulled again during up, where a failure is recorded as a failed up and nothing is put back. up now gets --no-build and --pull missing: it uses the images the earlier steps prepared, and still fetches one they did not. Found by Copilot on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
add() made the stack folder and wrote its first files before anything checked that the projects folder is on a mounted disk; only saving the git settings refused. With a projects folder left in RAM (a pool that did not mount), add wrote there and left a clone and a partly made stack folder behind. The stack folder is now checked first, before the clone, as convert already does. The test that relied on the old order to reach the "only partly made" message is replaced by one that expects nothing written. Found by Copilot on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
convert copies the .env next to an indirect stack's old compose file into the stack folder, since that one stops being used once the compose file is in the clone. It skipped the copy whenever the stack had an env file setting at all. But a setting that names a missing file is ignored, and the .env next to the compose file is used instead: after the convert that .env was no longer used, and its values were lost. The copy is now decided by which .env is actually in use. Found by Copilot on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tack folder A deploy with --save-local-changes saves edits made in the clone as a patch and discards them. When that happened at the same commit as the one deployed, no commit differed, so nothing was recreated: a container with a single-file mount of an edited config kept the edit, while the deploy reported success. When the discarded changes include files in the stack's folder, the deploy now leaves a git_discarded_changes file in the stack folder (before discarding, so a failure to write it stops the deploy with the changes in place). While it is there, every deploy recreates the containers; a successful one removes it. Found by Copilot on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A git deploy's checks run docker compose with a minimal environment, in
the compose file's folder. compose-git then started compose.sh with the
caller's whole shell environment, from the caller's folder. Variables
exported in that shell win over the stack's .env in compose, and ${PWD}
was the caller's folder, so the checks could pass for one port or bind
path while up deployed another. DOCKER_HOST could even point up at
another daemon than the one the checks asked.
compose-git now starts compose.sh with the checks' environment (plus the
documented COMPOSE_LOCK_TIMEOUT and COMPOSE_LOCK_DIR, and DOCKER_CONFIG)
in the compose file's folder, and the checks set PWD to their folder as
a shell would.
Found by Copilot on mstrhakr#172.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A deploy checks the new commit out in the clone, then runs the checks, pull and build, then up. Containers are not touched before up, but one that mounts files from the clone sees the new commit's files during that time, and the old ones again if the deploy is put back. An app that reloads its config on change can pick up a commit that is then put back. Say so, with what to do about it. Raised by Copilot on mstrhakr#172. Checking the new commit out only right before up (a staging copy for the checks and build) would narrow this, but is a change of its own. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…be asked about A deploy's check read any failed "docker network inspect" or "docker volume inspect" as "does not exist". With the daemon not answering, the deploy still stopped, but told the user to create a network or volume that may well exist. Only Docker's own "not found" answer now means missing; any other failure says the check could not be made, with Docker's message, as the container name check already does. The test stand-in now prints Docker's real "not found" messages, so the tests reach both answers. Found while checking Copilot's findings on mstrhakr#172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Git runs for git stacks use a home folder in /var/tmp, where anyone can create things, and the global config git reads (the list of trusted repositories) is kept there. A folder already there was used as it was: if another user made it first, or put a symlink there, they could give git a config of their own, which root then ran with. The folder is now used only when it is a plain folder owned by whoever runs the plugin (root on Unraid), and made private; otherwise git is not run and the result says why. Found while checking Copilot's findings on mstrhakr#174, which raised the same pattern for the folder that holds git credentials. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Copilot's review of #172 found nine things in the git stacks code from #171. I checked each against the code
and on Unraid: seven are real (all low severity, none loses data), one is not, and one is a design trade-off
that the docs now explain. This fixes the seven, plus two of the same kind found while checking.
What it fixes, one commit each, each with a test:
up; the deploy now stops and puts theprevious commit back, as for a failed pull or build.
upbuilt or pulled again for services withpull_policy: buildoralways; it now gets--no-build --pull missingand uses what the earlier steps prepared.compose-git addwrote the stack folder before checking the projects folder is mounted.convertwith an env file setting that names a missing file lost the.envactually in use.--save-local-changesat the same commit did not recreate containers, so asingle-file mount kept the discarded edit.
upran with different environments, so an exported variable or${PWD}could makeupdeploy something other than what was checked.up(Copilot's staging-tree finding).network/volume inspectwas always read as "does not exist", and the git home folder in/var/tmpwas used even when another user had made it first.Not changed: Copilot's
.env-from-the-working-directory finding. Compose reads.envfrom the compose file'sfolder, not the working directory, so it does not apply.
Reading: 10 commits, each one fix with its test, in the order above.
Testing: the full suite (PHPUnit, PHPStan, ShellCheck, Bats as non-root) passes at every commit. On Unraid
7.3.2 with Docker Compose 5.5, an end-to-end script checked the environment and
${PWD}, apull_policy: buildservice, the recreate after discarding an edited single-file mount, a network that cannot be created, and a
clones folder on a nested tmpfs (22 checks), plus a symlinked git home folder.