feat: install.sh - #59
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
I'd like to check the go.mod path and fix the error surfacing that could have a wrong install leaving the shell half configured
| case "$ARCH" in | ||
| x86_64 | amd64) ARCH=amd64 ;; | ||
| aarch64 | arm64) ARCH=arm64 ;; | ||
| *) err "unsupported architecture '${ARCH}' — 'go install github.com/${REPO}@${VERSION}' builds from source" ;; |
There was a problem hiding this comment.
Ok claude is flagging an issue with the go.mod from here:
Both go install fallbacks are broken for v2: @latest never selects prereleases (silently installs v1.1.0), and once 2.0.0 is stable it still resolves v1.1.0 because go.mod lacks the /v2 module-path suffix.
Can we check it please ?
There was a problem hiding this comment.
Confirmed. go.mod declares module github.com/Flagsmith/flagsmith-cli (no /v2), so:
go install github.com/Flagsmith/flagsmith-cli@latestresolves the v0/v1 module path → v1.1.0 (and@latestnever selects prereleases regardless).go install github.com/Flagsmith/flagsmith-cli@v2.0.0-beta.1would be rejected: major version v2 requires a/v2module-path suffix.
The same issue affects README.md, which now says go install github.com/Flagsmith/flagsmith-cli@latest as an alternative. Fixing go.mod is separate work; for this PR, either remove the go install suggestion or caveat it as v1-only.
| fi | ||
| } | ||
|
|
||
| main "$@" || exit 1 |
There was a problem hiding this comment.
| main "$@" || exit 1 | |
| main "$@" |
Otherwise the || will turn off the set -eu inside main. Leading to swallowing errors it's supposed to raise and succeeding because last command is an echo.
Reproduced with the following
set -eu
main() { false; echo "STILL RUNNING after failure"; }
main "$@" || exit 1
--
set -eu
main() { false; echo "STILL RUNNING"; }
main "$@"
The script pins the version it was released alongside, so the common path makes no API call and cannot be rate limited: release-please rewrites DEFAULT_VERSION through the `extra-files` entry, whose Generic updater replaces the first semver token on any line carrying `x-release-please-version`. Only the token changes, so the leading v survives. Everything lives in functions with `main "$@"` last, so a truncated download cannot execute half a script. The strict curl flags apply to https URLs only — FLAGSMITH_CLI_BASE_URL exists to point the script at a local server for testing, and that server is plain http. PATH handling follows uv: an env script the startup files source, edited once each, skipped when the install dir is already on PATH. The env script lives under our own directory because $HOME/.local/bin/env belongs to cargo-dist and uv writes it. beep boop
Each `run:` step gets a fresh shell, and an Actions shell is neither a login nor an interactive one, so nothing it starts reads the startup files the installer edited. Appending the directory to the file $GITHUB_PATH points at is the mechanism the runner does honour: it prepends those entries for every later step. So `curl ... | sh` in one step and `flagsmith` in the next now works on any runner, rather than only on images that already ship ~/.local/bin on PATH. Taken from uv's Add-Ci-Path. The release smoke job drops its explicit sourcing of the env script and relies on this instead. beep boop
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeWell-structured install script modelled on established installers (uv, rustup). Two issues from the existing review threads need addressing before merge: All checks from the latest CI run passed; the cancelled checks are from a superseded run.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Automate: a CI job that runs the installer against a goreleaser snapshot (already added in the release workflow; consider also running it on PR CI with a mock server). Product take: This is the primary binary distribution channel for the Go CLI rewrite — essential for adoption. The script itself is solid and follows established conventions. Once the two Majors are addressed, it's ready to ship. 🧭 Assumptions & unverified claims
A script that diligently verifies checksums but then |
2 participants
Closes #44.
curl -fsSL https://raw.githubusercontent.com/Flagsmith/flagsmith-cli/main/install.sh | shThe script installs to
$HOME/.local/bin, verifies the sha256 fromchecksums.txt, and adds the install directory toPATHif it's not there.The script pins the version it shipped with. The pin is bumped by release-please.
Verified locally
Against a goreleaser snapshot served over http, into a sandboxed
HOME:.profilesources it, and sourcing it putsflagsmithonPATH--no-modify-pathwrites neither the rc line nor the env scriptchecksum mismatchexpected exactly one checksumvall behaveImportant
DEFAULT_VERSIONisv2.0.0-beta.1, which is what ci: configure release-please for the 2.0.0 beta line #54'sRelease-As:footer forces.gh attestation verify --owner Flagsmith <file>— but the script doesn't check it, since users won't haveghinstalled.curl | shis unaffected (curl sets no quarantine xattr); a browser download on macOS will be Gatekeeper-blocked.install.ps1for Windows is deliberately out of scope — the script tells Windows users to take the.zip.add_ci_pathappends the install dir to$GITHUB_PATH, socurl … | shin one Actions step leaves the CLI on PATH for later steps — an Actions shell is neither login nor interactive, so it never reads the startup files the installer edits. Taken from uv. The release smoke job relies on it rather than sourcing the env script.