Add support for TMPDIR in install.sh#5285
Conversation
* Adds support for the POSIX TMPDIR standard in `install.sh` which enables the user to indicate their temporary directory with the TMPDIR environment variable. If no TMPDIR value is set, the installer falls back to the original behavior of using `/tmp`. * Adds caching if the installation package is already present locally and the remote package is not newer. This would prevent re-downloading the same release package file if the newest package is already present. * Expands the curl arguments for these package downloads from their single character version (e.g. -L) to the full version (e.g. --location) for greater clarity. * Disable the curl progress meter when fetching the binary package. This matches the curl behavior when downloading the MacOS package and keeps the installer output clean. * Remove the deletion of `/tmp/netbird.pkg` for MacOS installations. This leaves it to the OS to manage keeping the `/tmp` directory clean and matches the behavior when downloading binary packages to `/tmp`. This also makes caching of packages possible by retaining the downloaded package. Fixes netbirdio#5284
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Installer as "install.sh"
participant GitHub as "GitHub Release"
participant FS as "Filesystem (${TMPDIR:-/tmp})"
User->>Installer: run install.sh (pass APP_TYPE)
Installer->>GitHub: curl request (--fail --silent --show-error --location --remote-name --time-cond, add Authorization header if GITHUB_TOKEN)
GitHub-->>Installer: return release asset
Installer->>FS: save asset to ${TMPDIR:-/tmp}
Installer->>Installer: compute BINARY_NAME using APP_TYPE + DARWIN/LINUX constants
Installer->>FS: move/install binary/package to target location (APP_TYPE-driven paths)
Installer-->>User: report install completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 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
🤖 Fix all issues with AI agents
In `@release_files/install.sh`:
- Line 79: The fallback curl invocation that retries the download currently
re-sends the conditional header (--time-cond) and can silently accept a 304 and
leave a stale binary; update the second curl command (the fallback after the ||)
to remove the --time-cond "${TMPDIR:-/tmp}/${BINARY_NAME}" argument so the
fallback always requests the resource unconditionally using the same flags
(e.g., keep --fail --silent --show-error --location --remote-name --dns-servers
8.8.8.8) with DOWNLOAD_URL and TMPDIR/BINARY_NAME unchanged.
🧹 Nitpick comments (2)
release_files/install.sh (2)
77-80: Consider extracting${TMPDIR:-/tmp}into a variable to reduce repetition and risk of divergence.The expression
${TMPDIR:-/tmp}is repeated seven times across the file. A single assignment near the top (e.g.,NB_TMPDIR="${TMPDIR:-/tmp}") would be easier to read, less error-prone, and simpler to change later.♻️ Suggested refactor
Add near the top of the file (e.g., after line 19):
+NB_TMPDIR="${TMPDIR:-/tmp}"Then replace every occurrence, for example:
- cd "${TMPDIR:-/tmp}" && curl --fail --silent --show-error --location --remote-name --time-cond "${TMPDIR:-/tmp}/${BINARY_NAME}" --header "Authorization: token ${GITHUB_TOKEN}" "$DOWNLOAD_URL" + cd "${NB_TMPDIR}" && curl --fail --silent --show-error --location --remote-name --time-cond "${NB_TMPDIR}/${BINARY_NAME}" --header "Authorization: token ${GITHUB_TOKEN}" "$DOWNLOAD_URL"…and similarly for lines 79, 188, and 189.
77-77: These curl invocations are very long single lines — consider line continuations for readability.Lines 77 and 79 are well over 150 characters. Shell line continuations (
\) would make these easier to review and maintain.♻️ Example for line 77
- cd "${TMPDIR:-/tmp}" && curl --fail --silent --show-error --location --remote-name --time-cond "${TMPDIR:-/tmp}/${BINARY_NAME}" --header "Authorization: token ${GITHUB_TOKEN}" "$DOWNLOAD_URL" + cd "${TMPDIR:-/tmp}" && \ + curl --fail --silent --show-error --location --remote-name \ + --time-cond "${TMPDIR:-/tmp}/${BINARY_NAME}" \ + --header "Authorization: token ${GITHUB_TOKEN}" \ + "$DOWNLOAD_URL"
This satisfies the SonarQube issue of "String literals should not be duplicated [shelldre:S1192](https://sonarcloud.io/organizations/netbirdio/rules?open=shelldre%3AS1192&rule_key=shelldre%3AS1192)" by setting the OS_TYPE values of "darwin" and "linux" to constants. This also changes the leading whitespace for one line from tabs to spaces to match its neighboring lines.
When fetching the binary release, if the curl fetch fails, the installer retries using the 8.8.8.8 DNS server. This commit removes the --time-cond curl argument from this fallback retry fetch since under this condition, where DNS isn't working, we should download the binary release regardless of whether or not there is a local copy of the file, to ensure we get an updated file if there is a newer version.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@release_files/install.sh`:
- Around line 16-17: The DARWIN and LINUX constants are defined as lowercase
strings but never dereferenced, causing OS_TYPE to become the uppercase literals
"DARWIN"/"LINUX" and producing wrong download filenames; fix by either (A)
dereferencing the constants where assigned and compared—e.g., set OS_TYPE to
"$DARWIN" or "$LINUX" and compare against "$DARWIN"/"$LINUX" wherever [
"$OS_TYPE" = ... ] is used so interpolated filenames use lowercase—or (B) remove
the DARWIN/LINUX constants and replace their usages with the lowercase literals
"darwin"/"linux" directly (ensure all comparisons and the OS_TYPE assignment and
any interpolation into download filenames reference the lowercase value). Ensure
changes touch the OS_TYPE assignments, comparisons, and any download filename
interpolation that uses OS_TYPE.
This was a mistake where the `$` was missing from the constants.
|
I'm unsure what the convention is here in the netbird repo I'm finding that SonarQube is identifying pre-existing issues with I began fixing the pre-existing issues that SonarQube identified (see 47ad132 and ce24863 ), but after doing so, SonarQube found further pre-existing issues with I'm trying to figure out if I should work on those pre-existing issues or ignore them. |
This was a mistake where the `$` was missing from the constants.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
To satisfy SonarQube which identified > Function parameters should be assigned to local variables with descriptive names [shelldre:S7679](https://sonarcloud.io/organizations/netbirdio/rules?open=shelldre%3AS7679&rule_key=shelldre%3AS7679) This replaces the use of `$1` in the `download_release_binary()` function in `installer.sh` with the variable `$APP_TYPE`
|
The remaining SonarQube issues involve
I think that maintaining POSIX compatibility across the potentially broad set of clients that one might run If the maintainers disagree and would like to see that change, I can make it in this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@release_files/install.sh`:
- Around line 80-82: Replace the curl calls that use --remote-name and
--time-cond (the branches that run with and without GITHUB_TOKEN) with a safe
temporary-file download pattern: create a temp file (using mktemp with
"${TMPDIR:-/tmp}/${BINARY_NAME}.XXXXXX"), invoke curl with -o "$tmp" and -z
"${TMPDIR:-/tmp}/${BINARY_NAME}" (preserve any header/option differences like
--header "Authorization: token ${GITHUB_TOKEN}" and the --dns-servers fallback),
capture the HTTP status via -w '%{http_code}', and only mv the temp file to
"${TMPDIR:-/tmp}/${BINARY_NAME}" when the status is 200; otherwise rm -f the
temp file. Update both the authenticated branch and the public branch (the two
curl invocations around BINARY_NAME/TMPDIR/DOWNLOAD_URL) to use this pattern to
avoid truncation on HTTP 304.
🧹 Nitpick comments (1)
release_files/install.sh (1)
100-100: The parameter expansion${APP_TYPE%_"${BINARY_BASE_NAME}"}is a no-op — likely meant to useBINARY_NAME.
APP_TYPEis"netbird"or"netbird-ui", which never ends with_${BINARY_BASE_NAME}, so the%suffix strip never matches and the expansion just returnsAPP_TYPEunchanged. This works by coincidence (the extracted binary name happens to equalAPP_TYPE), but it's misleading.If the intent was to derive the binary name from the archive filename, it should operate on
BINARY_NAME. If the intent is simply to reference the extracted binary byAPP_TYPE, drop the%expansion for clarity:Suggested simplification
- ${SUDO} mv "${APP_TYPE%_"${BINARY_BASE_NAME}"}" "$INSTALL_DIR/" + ${SUDO} mv "${APP_TYPE}" "$INSTALL_DIR/"
This tells curl to set the modified time of the file it downloads to that on the remote server. This will enable effective caching if the package is fetched again.
Given the broad set of OSes that the install.sh runs on, we should prefer POSIX compliance over the reliability best practice that SonarQube calls out in [shelldre:S7688](https://sonarcloud.io/organizations/netbirdio/rules?open=shelldre%3AS7688&rule_key=shelldre%3AS7688) This disables SonarQube on these lines (sadly there is no more targeted way to do this, for example to disable only this specific shelldre for these lines)
The syntax for the NOSONAR annotation is //NOSONAR
Moving the //NOSONAR annotation to the end of the comment to be at the end of the line
It looks like the //NOSONAR annotation doesn't work in shell files.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@release_files/install.sh`:
- Around line 67-74: Keep the existing UI Linux asset naming as-is (do not
change the repeated "linux" segment); fix the unsafe unquoted assignment by
changing APP_TYPE=$1 to APP_TYPE="$1" so APP_TYPE safely handles spaces/special
chars, and simplify the pointless parameter expansion
`${APP_TYPE%_"${BINARY_BASE_NAME}"}` to just `${APP_TYPE}` (adjust uses in
BINARY_NAME/BINARY_BASE_NAME logic accordingly) to improve clarity.
🧹 Nitpick comments (2)
release_files/install.sh (2)
100-100: No-op parameter expansion — simplify to$APP_TYPE.
${APP_TYPE%_"${BINARY_BASE_NAME}"}attempts to strip a suffix fromAPP_TYPE, butAPP_TYPE("netbird"or"netbird-ui") never ends with_${BINARY_BASE_NAME}, so the%expansion is always a no-op. This is confusing to read and suggests the extracted binary name is derived fromBINARY_NAMEwhen it's really justAPP_TYPE.♻️ Proposed simplification
- ${SUDO} mv "${APP_TYPE%_"${BINARY_BASE_NAME}"}" "$INSTALL_DIR/" + ${SUDO} mv "${APP_TYPE}" "$INSTALL_DIR/"
80-82:cdinside the function permanently changes the working directory for the caller.
cd "${TMPDIR:-/tmp}"runs in the current shell (not a subshell), so afterdownload_release_binaryreturns, the script's working directory is${TMPDIR:-/tmp}. This happens to work because subsequent operations reference$BINARY_NAMEas a relative path and later code uses absolute paths for service installation. However, this is fragile — future changes that assume the original working directory after callinginstall_native_binariescould silently break.Consider wrapping the function body in a subshell
(...)or saving/restoring the directory.
release_files/install.sh
Outdated
| if [ "$APP_TYPE" = "$UI_APP" ]; then # Prefer POSIX compliance over shelldre:S7688 //NOSONAR | ||
| BINARY_NAME="${APP_TYPE}-${OS_TYPE}_${BINARY_BASE_NAME}" | ||
| if [ "$OS_TYPE" = "$DARWIN" ]; then # Prefer POSIX compliance over shelldre:S7688 //NOSONAR | ||
| BINARY_NAME="${APP_TYPE}_${BINARY_BASE_NAME}" | ||
| fi | ||
| else | ||
| BINARY_NAME="$1_${BINARY_BASE_NAME}" | ||
| BINARY_NAME="${APP_TYPE}_${BINARY_BASE_NAME}" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking latest GitHub release assets for netbird-ui ==="
curl -s https://api.github.com/repos/netbirdio/netbird/releases/latest \
| jq -r '.assets[] | select(.name | contains("netbird-ui")) | .name' | head -20
echo ""
echo "=== Checking goreleaser UI config for naming patterns ==="
fd '.goreleaser' . --type f 2>/dev/null | head -5
# Look for goreleaser config files
fd 'goreleaser' . --type f 2>/dev/null | while read f; do
if [[ "$f" == *".yml"* ]] || [[ "$f" == *".yaml"* ]]; then
echo "File: $f"
head -30 "$f"
echo "---"
fi
done
echo ""
echo "=== Checking current install.sh lines 55-75 ==="
sed -n '55,75p' release_files/install.sh
echo ""
echo "=== Checking old install.sh from main branch ==="
curl -s https://raw.githubusercontent.com/netbirdio/netbird/main/release_files/install.sh 2>/dev/null | sed -n '55,75p' || echo "Could not fetch main branch"Repository: netbirdio/netbird
Length of output: 2307
🏁 Script executed:
sed -n '95,105p' release_files/install.shRepository: netbirdio/netbird
Length of output: 416
🏁 Script executed:
sed -n '55,60p' release_files/install.shRepository: netbirdio/netbird
Length of output: 355
Remove the binary naming concern; fix line 57 quoting.
The UI Linux asset name netbird-ui-linux_0.65.0_linux_amd64.tar.gz is correct per GitHub releases—the repeated linux segment is intentional. However, line 57 requires quoting: APP_TYPE="$1" instead of APP_TYPE=$1 to safely handle potential whitespace or special characters.
Additionally, the parameter expansion on line 100—${APP_TYPE%_"${BINARY_BASE_NAME}"} —is a no-op (never matches, returns APP_TYPE unchanged) and could be simplified to just ${APP_TYPE} for clarity.
🤖 Prompt for AI Agents
In `@release_files/install.sh` around lines 67 - 74, Keep the existing UI Linux
asset naming as-is (do not change the repeated "linux" segment); fix the unsafe
unquoted assignment by changing APP_TYPE=$1 to APP_TYPE="$1" so APP_TYPE safely
handles spaces/special chars, and simplify the pointless parameter expansion
`${APP_TYPE%_"${BINARY_BASE_NAME}"}` to just `${APP_TYPE}` (adjust uses in
BINARY_NAME/BINARY_BASE_NAME logic accordingly) to improve clarity.
1 participant
Describe your changes
install.shwhich enables the user to indicate their temporary directory with the TMPDIR environment variable. If no TMPDIR value is set, the installer falls back to the original behavior of using/tmp./tmp/netbird.pkgfor MacOS installations. This leaves it to the OS to manage keeping the/tmpdirectory clean and matches the behavior when downloading binary packages to/tmp. This also makes caching of packages possible by retaining the downloaded package.Issue ticket number and link
Fixes #5284
Stack
Checklist
Summary by CodeRabbit
Bug Fixes
Improvements