fix ARCH detection and normalization#8658
Conversation
|
|
📝 WalkthroughWalkthroughAdded defensive checks for empty release lookups and normalized CPU architecture identifiers (x86_64/amd64 → amd64, aarch64/arm64 → arm64); UPPER_CPU_ARCH computation moved after normalization and normalization blocks added in multiple script locations for consistency. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/cli/community/install.sh`:
- Around line 218-223: The upgrade() flow still assigns APP_RELEASE via command
substitution without checking for empty results, so a failed checkLatestRelease
can propagate an empty tag; update upgrade() to call checkLatestRelease into
APP_RELEASE (or a temp var) and validate it's non-empty before proceeding,
logging an error and exiting (similar to the install path) if the release lookup
failed; reference the upgrade() function and the checkLatestRelease/APP_RELEASE
symbols when making the change.
Add error handling for missing latest release in upgrade function.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
deployments/cli/community/install.sh (1)
430-434: SC2155 on Line 430 still present — split declaration and assignment.The guard on lines 431-434 correctly catches the empty string, so this is functionally safe. However,
local latest_release=$(checkLatestRelease)still masks the subshell's exit code becauselocalalways exits 0 (SC2155). The past review's suggested fix included splitting this into two statements:🔧 Suggested fix
- local latest_release=$(checkLatestRelease) + local latest_release + latest_release=$(checkLatestRelease) if [ -z "$latest_release" ]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/cli/community/install.sh` around lines 430 - 434, Split the `local latest_release=$(checkLatestRelease)` into two statements so the subshell exit status isn't masked: first declare the variable with `local latest_release`, then assign it with `latest_release=$(checkLatestRelease)` and keep the existing guard that checks `-z "$latest_release"`. Update the usage in this block (the variable `latest_release` and the call to `checkLatestRelease`) only — do not change the guard logic that exits on empty value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@deployments/cli/community/install.sh`:
- Around line 430-434: Split the `local latest_release=$(checkLatestRelease)`
into two statements so the subshell exit status isn't masked: first declare the
variable with `local latest_release`, then assign it with
`latest_release=$(checkLatestRelease)` and keep the existing guard that checks
`-z "$latest_release"`. Update the usage in this block (the variable
`latest_release` and the call to `checkLatestRelease`) only — do not change the
guard logic that exits on empty value.
2 participants
Description
Fixes two bugs in the community self-hosted install script (
deployments/cli/community/install.sh) that cause installation to fail on x86_64 systems:Bug 1:
checkLatestReleasefailure not caught by parent scriptWhen
checkLatestRelease()fails (e.g., due to GitHub API rate limiting or no network access), it callsexit 1— but because it runs inside a$(...)command substitution, that exit only terminates the subshell, not the parent script. The result isAPP_RELEASEgets set to an empty string, which cascades into a brokendocker manifest inspectcall (invalid reference format) and eventually an offer to build images locally against a blank release tag.Fix: Added a guard after the command substitution to check for an empty
APP_RELEASEand exit cleanly with a clear error message.Bug 2:
UPPER_CPU_ARCHset before CPU architecture normalizationUPPER_CPU_ARCHis derived fromCPU_ARCHat the top of the script, whereuname -mreturns the raw value (e.g.,x86_64). The normalization block that mapsx86_64→amd64andaarch64→arm64runs later near the bottom of the script, butUPPER_CPU_ARCHis never recalculated. This causes:docker manifest inspectgrep to search forx86_64instead ofamd64, which never matches since Docker manifests useamd64X86_64instead of the correctAMD64Fix: Moved the
UPPER_CPU_ARCHassignment to after the normalization block so it reflects the corrected architecture name.Type of Change
Screenshots and Media (if applicable)
Before (x86_64 system):
Summary by CodeRabbit