Repository navigation
fix(installer): vendor CLI installs with CONFIG_URL - #225
Merged
Merged
Conversation
…ilures With CONFIG_URL, install_config now runs config:install with --force and, if the directory of the installed upsun is writable, --bin-dir set to it. Root installs (e.g. in CI) put the vendor executable in /usr/bin next to upsun, instead of ~/.platform-alt/bin, which is not on the PATH. An existing executable with the same name (e.g. from a previous installer) no longer stops the install; if another one comes first in the PATH, a warning names it. The outro names the vendor executable, read from config:install's output, with its full path if it is not found on the PATH. call_user returned the status of indent, so a failed command was never retried with sudo, e.g. moving the binary to /usr/bin as a non-root user. It now returns the command's status. The raw method installs in ~/.local/bin if the default directory is not writable and sudo is not available. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
scripts/test/installer-config.sh serves a local build and the test config, and runs installer.sh with CONFIG_URL in Debian and Alpine containers: apt, apk and raw installs as root, raw installs as a non-root user with and without sudo, and an install over an existing executable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
As root, when the vendor executable goes in a system directory such as /usr/bin, its configuration was saved in root's home directory, which other users usually cannot read. The installer now passes --config-dir /etc/platform-alt in that case, e.g. for a Dockerfile that installs as root and then switches to another user. Config auto-updates are skipped when the config's directory is not writable, so other users do not get an error for a config that only root can update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
config:install rejects a symlink as --bin-dir, so a symlinked INSTALL_DIR made the install fail. The directory is now resolved first, and paths are compared by file rather than by name. With a umask such as 077, a shared root install created files that other users could not read or run. The umask is set to 022 for that install, and /etc/platform-alt is made readable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Config updates wrote a new file with mode 0644 filtered by the umask, and renamed it over the old one. Under a restrictive umask, a shared config (e.g. in /etc/platform-alt, updated by root) became unreadable to other users. Updates now keep the existing file's mode, and alt files are written with their mode regardless of the umask. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The directory is named after the package that writes it, leaving /etc/upsun for other system-wide settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 3 warnings · 🔵 2 minor points
🔍 Full review · 7 files reviewed
Verification
call_usernow returnssh -c's exit code underset -eu; I ran it in dash and bash: failures reachif !/||callers, and success does not abort the script.Updateregisters the deferred LastChecked save before the new writability check, so an unwritable shared config is checked once per interval rather than on every run.writeFile's explicitos.Chmodfixes the mode even when a leftover.tmpfile exists, andUpdatepasses the original file's permission bits.config:installdefines--bin-dir,--config-dirand--force, and printsExecutable: <path>to stderr, which the installer captures with2>&1and parses with sed.
New Go tests cover the Go changes: TestUpdateWithUnwritableDir and TestUpdateKeepsMode (unix only), run by the Go tests in ci.yml. The installer changes are only exercised by the manual Docker script scripts/test/installer-config.sh. test-installer.yml runs installer.sh but never sets CONFIG_URL, so no CI job covers the new install_config or check_directories paths.
Review details
- Commit: 32e8fad
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
- config:install shortened paths that only share a string prefix with the home directory, e.g. "~usr/bin" for HOME=/. It now requires a path separator after the home directory. - The raw method's fallback to ~/.local/bin no longer applies to a directory in the home directory that does not exist yet, e.g. ~/.global/bin. - The README says the executable is not put next to upsun with Homebrew. - The container tests fail when the installer fails, rather than taking tee's status, and use the Docker host's architecture. 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
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.
Fixes for installing an alternative CLI with
CONFIG_URL(added in #215, not released yet), found by testing in Debian and Alpine containers.config:installnow gets--bin-dirset to the directory of the installedupsun, when it is writable. Root installs (e.g. in CI) previously put the shim in~/.platform-alt/bin, which is not on the PATH. Homebrew's directory is left alone./etc/upsun/platform-alt(mode 755, umask 022), so other users can run the shim, e.g. a Dockerfile that installs as root and then switchesUSER.--force: the installer is explicitly installing this config, so an existing executable with the same name no longer stops it. If another one comes first in the PATH, a warning names it.config:install's output), with its full path if it is not on the PATH.call_userreturned the status ofindent, so failures were never retried with sudo, e.g. moving the binary to/usr/binas a non-root user. The raw method now also falls back to~/.local/binwhen the default directory is not writable and sudo is not available.INSTALL_DIR: resolved before passing--bin-dir, which rejects symlinks.scripts/test/installer-config.shruns the installer withCONFIG_URLagainst a localmake singlebuild in containers: apt, apk and raw as root, raw as a non-root user with and without sudo, over an existing executable, and withumask 077and a symlinkedINSTALL_DIR. The apt and apk cases install the released package.🤖 Generated with Claude Code