fix ARCH detection and normalization#8658
Conversation
|
|
📝 WalkthroughWalkthroughThe installation script now includes a defensive check for failed release lookups and normalizes CPU architecture values (x86_64/aarch64 variants) to Docker manifest standards (amd64/arm64), with UPPER_CPU_ARCH computation repositioned 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://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.
| if [ "$APP_RELEASE" == "stable" ]; then | ||
| export APP_RELEASE=$(checkLatestRelease) | ||
| if [ -z "$APP_RELEASE" ]; then | ||
| echo "Failed to determine the latest release. Exiting..." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Also guard upgrade() against empty release tags.
The new check prevents the install path from continuing with an empty tag, but upgrade() (Line 430) still uses a command substitution without this guard, so a failed lookup can still propagate an empty tag and trigger the same downstream errors. Consider reusing the guard there too.
🔧 Suggested fix for upgrade()
function upgrade() {
- local latest_release=$(checkLatestRelease)
+ local latest_release
+ latest_release=$(checkLatestRelease)
+ if [ -z "$latest_release" ]; then
+ echo "Failed to determine the latest release. Exiting..."
+ exit 1
+ fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 219-219: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 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.
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