[ci] Add FreeBSD port release job to GitHub Actions #4916
Conversation
Includes scripts for generating FreeBSD port update diff and issue body.
WalkthroughAdds a new GitHub Actions job Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GHA as GitHub Actions
participant Repo as Repository (scripts)
participant Ports as FreeBSD Ports (cgit)
participant GH as GitHub API
participant GMP as Go Module Proxy
participant VM as FreeBSD Test VM
participant Art as Artifact Storage
GHA->>Repo: checkout & run freebsd-port-diff.sh / freebsd-port-issue-body.sh
Repo->>Ports: fetch current Makefile & distinfo (OLD_VERSION)
Repo->>GH: fetch tags/releases (NEW_VERSION + history)
Repo->>GMP: download .mod and .zip for NEW_VERSION
Repo-->>Repo: compute checksums & sizes, build new Makefile/distinfo
Repo-->>Repo: generate unified diff and issue text files
GHA->>VM: provision VM, upload/apply diff, run portlint, build, install, verify
VM-->>GHA: return build & QA results and pkg artifacts
GHA->>Art: upload freebsd-port-files artifact (diff, issue text, packages)
Art-->>GHA: confirm upload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 3
🧹 Nitpick comments (6)
.github/workflows/release.yml (1)
22-36: Consider restricting the job to tag releases only.The new
release_freebsd_portjob runs on every push to main and pull requests, but FreeBSD port updates should typically only occur when releasing tagged versions. This could generate unnecessary artifacts and noise on non-release builds.Add a conditional to limit execution to tag pushes:
release_freebsd_port: runs-on: ubuntu-latest + if: startsWith(github.ref, 'refs/tags/') steps:Alternatively, if you want it to run on all builds (e.g., for testing purposes), consider documenting this intention in a comment.
release_files/freebsd-port-issue-body.sh (2)
104-116: Changelog generation relies on external API without resilience.The
generate_changelog_sectionfunction fetches all releases again viafetch_releases_between_versions, which triggers additional curl requests to GitHub. If GitHub is temporarily unavailable, the issue body generation will fail completely.Consider caching the releases list to avoid redundant external calls. Modify the main script to fetch releases once and pass to the function:
-generate_changelog_section() { - local releases - releases=$(fetch_releases_between_versions) - +generate_changelog_section() { + local releases="$1" + echo "Changelogs:" if [ -n "$releases" ]; thenThen call it as:
generate_changelog_section "$(fetch_releases_between_versions)"
59-84: Batch external API calls into a single setup phase.The version detection logic (lines 59–80) makes independent curl calls for OLD_VERSION and NEW_VERSION. Combined with the changelog generation, this creates multiple round-trips to external APIs. If any call fails mid-stream, the script exits with
set -ebut leaves incomplete output files.Group all external API calls early in the script before any file operations:
+ # Fetch all external data upfront to fail fast + PORTS_VERSION=$(fetch_current_ports_version) + GITHUB_RELEASES=$(fetch_all_tags) + LATEST_RELEASE=$(echo "$GITHUB_RELEASES" | tail -1) + OLD_VERSION="${1:-}" NEW_VERSION="${2:-}" if [ -z "$OLD_VERSION" ]; then - OLD_VERSION=$(fetch_current_ports_version) + OLD_VERSION="$PORTS_VERSION"This ensures all external state is fetched before any side effects (file writes).
release_files/freebsd-port-diff.sh (3)
93-109: Remove unused parameter and extract duplicate AWK pattern.Line 94 declares
old_versionas a parameter togenerate_new_makefile()but it's never used in the function body. Additionally, the awk pattern'{print $1}'is repeated 4 times in the checksums function (lines 68, 69, 71, 72).Remove the unused parameter and define a constant for the pattern:
-generate_new_makefile() { - local old_version="$1" - local new_version="$2" - local old_makefile="$3" +generate_new_makefile() { + local new_version="$1" + local old_makefile="$2"For the AWK pattern, define it at the script level:
set -e +AWK_FIRST_FIELD='{print $1}' GITHUB_REPO="netbirdio/netbird"Then update the lines:
- mod_sha256=$(sha256sum "$mod_file" | awk '{print $1}') - zip_sha256=$(sha256sum "$zip_file" | awk '{print $1}') + mod_sha256=$(sha256sum "$mod_file" | awk "$AWK_FIRST_FIELD") + zip_sha256=$(sha256sum "$zip_file" | awk "$AWK_FIRST_FIELD") elif command -v shasum &>/dev/null; then - mod_sha256=$(shasum -a 256 "$mod_file" | awk '{print $1}') - zip_sha256=$(shasum -a 256 "$zip_file" | awk '{print $1}') + mod_sha256=$(shasum -a 256 "$mod_file" | awk "$AWK_FIRST_FIELD") + zip_sha256=$(shasum -a 256 "$zip_file" | awk "$AWK_FIRST_FIELD")Update the function call at line 155:
-NEW_MAKEFILE=$(generate_new_makefile "$OLD_VERSION" "$NEW_VERSION" "$OLD_MAKEFILE") +NEW_MAKEFILE=$(generate_new_makefile "$NEW_VERSION" "$OLD_MAKEFILE")
102-102: Makefile sed patterns use literal tabs; consider using explicit whitespace.Lines 102 and 107 use hardcoded tabs in sed replacements for
DISTVERSION= ${new_version}. While this works, it's fragile if the FreeBSD Makefile format changes or when debugging. The current Makefile may use spaces instead of tabs.Replace literal tabs with explicit tab notation for clarity and robustness:
- sed "s/^DISTVERSION=.*/DISTVERSION= ${new_version}/" | \ + sed "s/^DISTVERSION=.*/DISTVERSION=\t${new_version}/" | \Alternatively, preserve the original whitespace dynamically:
sed "s/^\(DISTVERSION=\)[[:space:]]*.*/\1${new_version}/"Also applies to: 107-107
41-44: Validate fetched ports files are non-empty.The
fetch_ports_file()function returns curl output without validating it's non-empty. While callers (lines 140–150) check for empty results, a curl error could silently return an HTTP error page (non-empty but invalid). Add error detection.Add HTTP status validation to
fetch_ports_file():fetch_ports_file() { local filename="$1" - curl -sL "${PORTS_CGIT_BASE}/${filename}" 2>/dev/null + curl -sfL "${PORTS_CGIT_BASE}/${filename}" 2>/dev/null || return 1 }The
-fflag makes curl return a non-zero exit code for HTTP 4xx/5xx errors, preventing invalid responses from being used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks)release_files/freebsd-port-diff.sh(1 hunks)release_files/freebsd-port-issue-body.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
release_files/freebsd-port-issue-body.sh (1)
release_files/freebsd-port-diff.sh (3)
fetch_all_tags(21-26)fetch_current_ports_version(28-34)fetch_latest_github_release(36-39)
release_files/freebsd-port-diff.sh (1)
release_files/freebsd-port-issue-body.sh (3)
fetch_all_tags(31-37)fetch_current_ports_version(20-29)fetch_latest_github_release(39-57)
🪛 GitHub Check: SonarCloud Code Analysis
release_files/freebsd-port-issue-body.sh
[warning] 31-31: Add an explicit return statement at the end of the function.
[warning] 90-90: Add an explicit return statement at the end of the function.
[failure] 72-72: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 96-96: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 97-97: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 64-64: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 62-62: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 82-82: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 104-104: Add an explicit return statement at the end of the function.
[failure] 52-52: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 109-109: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 98-98: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 74-74: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 24-24: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 46-46: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
release_files/freebsd-port-diff.sh
[failure] 116-116: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 36-36: Add an explicit return statement at the end of the function.
[warning] 72-72: Define a constant instead of using the literal '{print $1}' 4 times.
[failure] 122-122: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 124-124: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 94-94: Remove the unused local variable 'old_version'.
[failure] 141-141: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 184-184: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 60-60: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 28-28: Add an explicit return statement at the end of the function.
[failure] 148-148: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 21-21: Add an explicit return statement at the end of the function.
[warning] 41-41: Add an explicit return statement at the end of the function.
[warning] 93-93: Add an explicit return statement at the end of the function.
[failure] 131-131: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 Shellcheck (0.11.0)
release_files/freebsd-port-diff.sh
[warning] 50-50: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 94-94: old_version appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 166-166: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Android / Build
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release_ui
- GitHub Check: Client / Unit
| local version="$1" | ||
| local tmpdir | ||
| tmpdir=$(mktemp -d) | ||
| trap "rm -rf '$tmpdir'" EXIT |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use single quotes in trap to defer variable expansion.
Lines 50 and 166 use unquoted variables in trap commands, which expands them immediately rather than when the trap fires. This is a security best practice to ensure cleanup uses the variable value at exit time, not at definition time.
Apply these diffs to fix trap quoting per ShellCheck SC2064:
compute_checksums() {
local version="$1"
local tmpdir
tmpdir=$(mktemp -d)
- trap "rm -rf '$tmpdir'" EXIT
+ trap "rm -rf \"$tmpdir\"" EXIT # Create temp files for diff
TMPDIR=$(mktemp -d)
-trap "rm -rf '$TMPDIR'" EXIT
+trap "rm -rf \"$TMPDIR\"" EXITActually, the safest approach is to use single quotes around the entire command:
- trap "rm -rf '$tmpdir'" EXIT
+ trap 'rm -rf "$tmpdir"' EXITAlso applies to: 166-166
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 50-50: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🤖 Prompt for AI Agents
In release_files/freebsd-port-diff.sh around lines 50 and 166, the trap commands
use double quotes which cause immediate variable expansion (triggers ShellCheck
SC2064); replace them so variable expansion is deferred by quoting the entire
command and protecting the variable with inner double quotes — i.e. change the
trap definitions to use single quotes around the whole command and double quotes
around $tmpdir so the value is expanded at trap execution time.
| curl -sL "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null | ||
| curl -sL "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null |
There was a problem hiding this comment.
Go module proxy downloads lack validation and timeout.
The curl commands on lines 57–58 download files from proxy.golang.org without:
- Timeout specifications (could hang indefinitely)
- Size limits (could download unexpectedly large files)
- Checksum pre-validation
If the Go proxy is down or returns garbage, the script proceeds with invalid data.
Add timeouts, size limits, and pre-download validation:
- curl -sL "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null
- curl -sL "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null
+ curl -sfL --max-time 30 --max-filesize 10M "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null || return 1
+ curl -sfL --max-time 30 --max-filesize 100M "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null || return 1This ensures:
-f: Fail on HTTP errors--max-time 30: Abort if download exceeds 30 seconds--max-filesize N: Reject files larger than expected- Exit code handling with
|| return 1
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| curl -sL "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null | |
| curl -sL "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null | |
| curl -sfL --max-time 30 --max-filesize 10M "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null || return 1 | |
| curl -sfL --max-time 30 --max-filesize 100M "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null || return 1 |
| fetch_releases_between_versions() { | ||
| echo "Fetching release history from GitHub..." >&2 | ||
|
|
||
| # Fetch all tags and filter to those between OLD and NEW versions | ||
| fetch_all_tags | \ | ||
| while read -r ver; do | ||
| if [ "$(printf '%s\n' "$OLD_VERSION" "$ver" | sort -V | head -n1)" = "$OLD_VERSION" ] && \ | ||
| [ "$(printf '%s\n' "$ver" "$NEW_VERSION" | sort -V | head -n1)" = "$ver" ] && \ | ||
| [ "$ver" != "$OLD_VERSION" ]; then | ||
| echo "$ver" | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Test 1: Standard version range filtering (as provided in review)
OLD_VERSION="0.56.0"
NEW_VERSION="0.59.1"
SAMPLE_TAGS="0.55.0
0.56.0
0.57.0
0.58.0
0.59.0
0.59.1
0.60.0"
echo "=== Test 1: Standard version range filtering ==="
echo "OLD_VERSION=$OLD_VERSION, NEW_VERSION=$NEW_VERSION"
echo "Expected: 0.57.0, 0.58.0, 0.59.0, 0.59.1"
echo "Results:"
echo "$SAMPLE_TAGS" | while read -r ver; do
if [ "$(printf '%s\n' "$OLD_VERSION" "$ver" | sort -V | head -n1)" = "$OLD_VERSION" ] && \
[ "$(printf '%s\n' "$ver" "$NEW_VERSION" | sort -V | head -n1)" = "$ver" ] && \
[ "$ver" != "$OLD_VERSION" ]; then
echo " ✓ $ver"
fi
done
# Test 2: Pre-release versions
echo ""
echo "=== Test 2: Pre-release versions ==="
OLD_VERSION="0.56.0"
NEW_VERSION="0.59.1"
PRERELEASE_TAGS="0.56.0
0.57.0-rc1
0.57.0-beta
0.57.0
0.58.0-alpha
0.58.0
0.59.0
0.59.1-rc1
0.59.1
0.60.0"
echo "Testing with pre-release versions:"
echo "$PRERELEASE_TAGS" | while read -r ver; do
if [ "$(printf '%s\n' "$OLD_VERSION" "$ver" | sort -V | head -n1)" = "$OLD_VERSION" ] && \
[ "$(printf '%s\n' "$ver" "$NEW_VERSION" | sort -V | head -n1)" = "$ver" ] && \
[ "$ver" != "$OLD_VERSION" ]; then
echo " ✓ $ver"
fi
done
# Test 3: Malformed/edge case versions
echo ""
echo "=== Test 3: Edge cases with malformed versions ==="
OLD_VERSION="1.0.0"
NEW_VERSION="2.0.0"
EDGE_TAGS="1.0.0
1.5.0
1.5.0.1
1.5
2.0.0
2.1.0
v1.7.0
1.0.0a"
echo "Testing with malformed/edge case versions:"
echo "$EDGE_TAGS" | while read -r ver; do
if [ "$(printf '%s\n' "$OLD_VERSION" "$ver" | sort -V | head -n1)" = "$OLD_VERSION" ] && \
[ "$(printf '%s\n' "$ver" "$NEW_VERSION" | sort -V | head -n1)" = "$ver" ] && \
[ "$ver" != "$OLD_VERSION" ]; then
echo " ✓ $ver (included)"
else
echo " ✗ $ver (excluded)"
fi
done
# Test 4: Verify sort -V behavior directly
echo ""
echo "=== Test 4: Direct sort -V behavior with edge cases ==="
printf '%s\n' "1.0.0" "1.5.0-rc1" "1.5.0" "2.0.0" | sort -VRepository: netbirdio/netbird
Length of output: 757
Add version validation and handle Git tag prefixes.
The version filtering logic relies on sort -V for version comparison, which has edge case issues. Testing reveals:
- Git tag prefix handling: Versions with "v" prefix (e.g.,
v1.7.0) are excluded, failing silently. If GitHub releases are tagged with "v" prefixes, they won't be included in the changelog. - Pre-release sorting anomaly:
sort -Vplaces pre-release versions (e.g.,0.59.1-rc1) after their release versions (e.g.,0.59.1), which is semantically incorrect. This may cause pre-releases to be unexpectedly included or excluded depending on the version range. - Malformed version handling: Ambiguous formats like
1.0.0aor1.5.0.1are accepted without validation, potentially including unintended tags.
Add explicit version validation before filtering and strip common tag prefixes (like v) to handle real-world GitHub tag formats reliably.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 90-90: Add an explicit return statement at the end of the function.
[failure] 96-96: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 97-97: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 98-98: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🤖 Prompt for AI Agents
In release_files/freebsd-port-issue-body.sh around lines 90 to 102, the tag
filtering loop must be made robust: normalize tags by stripping common Git tag
prefixes (e.g., leading "v" or "V" and "refs/tags/"), validate each tag against
a strict semver regex (e.g. major.minor.patch with optional prerelease/build)
and skip malformed tags, and replace the current sort -V reliance with a
semver-aware comparison (e.g., call a small Python snippet using
packaging.version.Version or a system semver tool) so prereleases compare
correctly relative to releases; finally, use the normalized, validated,
semver-sorted list to apply the OLD/NEW range check and echo only tags that fall
strictly between OLD_VERSION and NEW_VERSION (ensure OLD_VERSION and NEW_VERSION
themselves are normalized/validated the same way).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/release.yml (3)
35-41: Version extraction lacks defensive error handling and file validation.If the previous script fails to generate
netbird-*.diffor creates multiple diff files, thelsglob andsedparsing will either fail silently or produce incorrect/multiple version strings. The downstreampatchcommand (line 71) depends on this version being correct, so failures here cascade.Consider adding explicit validation:
- name: Extract version id: version run: | - VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') - echo "version=$VERSION" >> $GITHUB_OUTPUT - echo "Generated files for version: $VERSION" - cat netbird-*.diff + # Validate diff file exists + if [[ ! -f netbird-*.diff ]]; then + echo "Error: No netbird-*.diff file generated" + exit 1 + fi + + # Extract version and validate + VERSION=$(ls -1 netbird-*.diff | head -1 | sed 's/netbird-\(.*\)\.diff/\1/') + if [[ -z "$VERSION" ]]; then + echo "Error: Failed to extract version from diff filename" + exit 1 + fi + + # Verify only one diff file exists + DIFF_COUNT=$(ls -1 netbird-*.diff | wc -l) + if [[ $DIFF_COUNT -gt 1 ]]; then + echo "Warning: Multiple diff files found, using first: $VERSION" + fi + + echo "version=$VERSION" >> $GITHUB_OUTPUT + echo "Generated files for version: $VERSION" + echo "Diff file size: $(wc -c < netbird-*.diff | head -1) bytes"
74-74: Clarify the intent of error suppression in build steps.Lines 74 and 78 suppress all errors with
|| true. While this allows the workflow to continue, it may hide real build or QA failures. Consider:
- Logging output before suppressing:
portlint -AC | tee portlint.log || true- Uploading logs as artifacts for review
- Documenting which failures are acceptable vs. which should block the job
If suppression is intentional, add a comment explaining why failures are acceptable:
# Run port linting - portlint -AC || true + # portlint warnings are non-blocking; we only fail on critical issues + portlint -AC || trueAlso applies to: 78-78
22-24: Consider job dependencies and sequencing.The
release_freebsd_portjob runs independently onubuntu-22.04and has noneeds:constraint linking it to thereleasejob. This means:
- Both jobs run in parallel (good for speed)
- FreeBSD port CI completes independent of main release (good for decoupling)
- But if port build fails, it doesn't block the release job (verify this is intentional)
If the FreeBSD port should be a release blocker, consider adding:
needs: [release] # Only run after main release succeedsIf independent release is intentional, the current setup is fine. Verify this design with your team.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/release.yml (3)
57-60: Consider making Go version configurable.Go version 1.24.10 is hardcoded in the FreeBSD port test. This makes it difficult to update Go versions across the entire workflow when needed.
Extract the Go version to a workflow environment variable:
env: SIGN_PIPE_VER: "v0.0.23" GORELEASER_VER: "v2.3.2" PRODUCT_NAME: "NetBird" COPYRIGHT: "NetBird GmbH" + GO_VERSION: "1.24.10"Then reference it in the script:
- GO_TARBALL="go1.24.10.freebsd-amd64.tar.gz" + GO_TARBALL="go${{ env.GO_VERSION }}.freebsd-amd64.tar.gz"
82-82: Clarify the intent of silently ignoring portlint errors.Line 82 uses
|| trueto suppress portlint exit codes. This means any linting issues are ignored and won't cause the job to fail. Consider capturing the output for review:- # Run port linting - portlint -AC || true + # Run port linting (capture but don't fail on warnings) + portlint -AC > portlint-output.txt 2>&1 || true + echo "Portlint results:" + cat portlint-output.txtAlternatively, if portlint errors should fail the build, remove the
|| true.
79-79: Improve robustness of patch file reference.Line 79 references the diff file via
~/$DIFF_FILE(home directory). While this may work, it's clearer to reference the synced workspace location explicitly:- # Apply the generated diff - patch -p2 < ~/$DIFF_FILE + # Apply the generated diff from synced workspace + if [ ! -f "$DIFF_FILE" ]; then + echo "ERROR: Diff file not found at $DIFF_FILE" + exit 1 + fi + patch -p2 < "$DIFF_FILE"This makes the file resolution explicit and adds a safety check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Android / Build
🔇 Additional comments (2)
.github/workflows/release.yml (2)
316-316: Verify whether release_freebsd_port should be added to trigger_signer dependencies.The
trigger_signerjob (line 316) depends on[release, release_ui, release_ui_darwin]but does not includerelease_freebsd_port. Confirm whether the FreeBSD port job should complete before triggering the signing pipeline.If FreeBSD port generation is part of the release workflow that should complete before signing, update:
- needs: [release, release_ui, release_ui_darwin] + needs: [release, release_ui, release_ui_darwin, release_freebsd_port]If the FreeBSD port is intentionally independent, consider adding a comment to clarify the design decision.
29-33: Both FreeBSD port scripts exist and are properly implemented with robust error handling and edge case validation.The scripts (
release_files/freebsd-port-diff.shandrelease_files/freebsd-port-issue-body.sh) are well-designed with:
- Comprehensive error detection (empty downloads, missing checksums, API failures)
- Cross-platform compatibility (macOS/Linux stat command handling)
- Automatic version detection with fallbacks (GitHub API fallback in issue-body script)
- Proper output file generation matching workflow expectations (
netbird-${NEW_VERSION}.diffandnetbird-${NEW_VERSION}-issue.txt)The workflow correctly executes these scripts.
| - name: Extract version | ||
| id: version | ||
| run: | | ||
| VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') | ||
| echo "version=$VERSION" >> $GITHUB_OUTPUT | ||
| echo "Generated files for version: $VERSION" | ||
| cat netbird-*.diff |
There was a problem hiding this comment.
Harden version extraction to handle edge cases.
Line 38 uses ls netbird-*.diff which could match multiple files, causing the version extraction to fail or produce unexpected results. Additionally, the script outputs all diff file contents to logs (line 41), which may not be necessary.
Consider strengthening the version extraction:
- VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/')
- echo "version=$VERSION" >> $GITHUB_OUTPUT
- echo "Generated files for version: $VERSION"
- cat netbird-*.diff
+ DIFF_FILE=$(ls netbird-*.diff | head -1)
+ if [ -z "$DIFF_FILE" ]; then
+ echo "ERROR: No diff file found"
+ exit 1
+ fi
+ VERSION=$(basename "$DIFF_FILE" | sed 's/netbird-\(.*\)\.diff/\1/')
+ echo "version=$VERSION" >> $GITHUB_OUTPUT
+ echo "Generated files for version: $VERSION"This ensures:
- Only one diff file is selected (first match)
- Explicit error if no file is found
- Clearer intent with variable naming
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Extract version | |
| id: version | |
| run: | | |
| VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') | |
| echo "version=$VERSION" >> $GITHUB_OUTPUT | |
| echo "Generated files for version: $VERSION" | |
| cat netbird-*.diff | |
| - name: Extract version | |
| id: version | |
| run: | | |
| DIFF_FILE=$(ls netbird-*.diff | head -1) | |
| if [ -z "$DIFF_FILE" ]; then | |
| echo "ERROR: No diff file found" | |
| exit 1 | |
| fi | |
| VERSION=$(basename "$DIFF_FILE" | sed 's/netbird-\(.*\)\.diff/\1/') | |
| echo "version=$VERSION" >> $GITHUB_OUTPUT | |
| echo "Generated files for version: $VERSION" |
🤖 Prompt for AI Agents
.github/workflows/release.yml lines 35-41: harden version extraction to handle
multiple or missing diff files and avoid dumping diffs to logs; change the
script to explicitly glob and select only the first matching file (e.g., capture
matches into an array and pick element 0), check and fail with a clear error if
no netbird-*.diff file is found, derive VERSION from that single filename using
a precise pattern/substitution, export it to GITHUB_OUTPUT as before, and remove
or comment out the cat netbird-*.diff line so diff contents are not printed to
logs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
35-41: Duplicate: Harden version extraction to handle edge cases.This is the same issue flagged in the previous review. The current implementation at line 38 still uses
ls netbird-*.diffwhich could match multiple files, lacks error handling if no file is found, and dumps diff contents to logs at line 41.For reference, the prior reviewer suggested:
- Select only the first matching file explicitly
- Check and fail with a clear error if no file is found
- Remove or comment out the
cat netbird-*.difflinePlease apply the previously suggested fix to ensure robustness.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
89-89: Clarify intent: Are portlint and make stage-qa failures non-blocking?Lines 89 and 93 suppress failures with
|| trueon portlint and make stage-qa steps. If these are informational/best-effort checks, this is reasonable; however, if they are required validations, failures should be fatal to prevent passing a broken port.If these checks are meant to enforce correctness, consider removing
|| trueso real issues are surfaced:- # Run port linting - portlint -AC || true + # Run port linting + portlint -AC- make stage - make stage-qa || true - make package + make stage + make stage-qa + make packageIf they are intentionally non-blocking (e.g., for diagnostic purposes), consider adding a comment to clarify that distinction.
Also applies to: 93-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
🔇 Additional comments (1)
.github/workflows/release.yml (1)
101-108: LGTM.The artifact upload configuration is appropriate. The glob patterns correctly capture the generated files, and the 30-day retention is reasonable for release materials.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
35-41: Version extraction still needs hardening (duplicate of prior feedback).This concern was raised in the previous review and remains unresolved. Line 38 uses
ls netbird-*.diffwhich can match multiple files, and line 41 dumps diff contents to logs unnecessarily.Apply the suggested fix from the prior review:
- name: Extract version id: version run: | - VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') + DIFF_FILE=$(ls netbird-*.diff | head -1) + if [ -z "$DIFF_FILE" ]; then + echo "ERROR: No diff file found" + exit 1 + fi + VERSION=$(basename "$DIFF_FILE" | sed 's/netbird-\(.*\)\.diff/\1/') echo "version=$VERSION" >> $GITHUB_OUTPUT echo "Generated files for version: $VERSION" - cat netbird-*.diff
🧹 Nitpick comments (2)
.github/workflows/release.yml (2)
54-54: Go version is hardcoded; consider adding a maintenance comment.Line 54 pins Go to
go1.24.10.freebsd-amd64.tar.gz. This is good for reproducibility, but future releases may need to update this. Consider adding a brief inline comment explaining the pinning strategy or linking to where this should be updated during future releases.Example:
- GO_TARBALL="go1.24.10.freebsd-amd64.tar.gz" + # Update this version with each new NetBird release if needed + GO_TARBALL="go1.24.10.freebsd-amd64.tar.gz"
60-60: Clarify the intent of the commented sparse-checkout command.Line 60 initializes a sparse clone with
--sparse, but line 62's sparse-checkout command is commented out. This leaves the sparse-checkout in an undefined state. Was this intentional (e.g., for debugging or due to a known issue), or should the command be uncommented?If sparse-checkout is not needed, remove the
--sparseflag from line 60. If it should be used, uncomment line 62:- # git sparse-checkout set Mk Templates Keywords security/netbird + git sparse-checkout set Mk Templates Keywords security/netbirdOr, if sparse-checkout is intentionally skipped, remove the
--sparseflag:- git clone --depth 1 --filter=blob:none --sparse https://git.FreeBSD.org/ports.git /usr/ports + git clone --depth 1 --filter=blob:none https://git.FreeBSD.org/ports.git /usr/portsAlso applies to: 62-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui_darwin
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (1)
.github/workflows/release.yml (1)
29-33: Verify that scripts produce artifacts with expected filenames.The workflow depends on
freebsd-port-diff.shandfreebsd-port-issue-body.sh(lines 29-33) producing files matching the patterns in the artifact upload step (lines 105-107):netbird-*-issue.txtandnetbird-*.diff. If script outputs don't match these patterns, the artifact upload will produce nothing.Please confirm the exact filenames (and locations) produced by both scripts, or provide the script contents for verification. You may also generate a small test to validate outputs match expected patterns.
Also applies to: 101-108
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
35-41: Harden version extraction to handle edge cases [DUPLICATE from past review].Line 38 uses
ls netbird-*.diffwhich could match multiple files and cause unpredictable results. Additionally, line 41 outputs the full diff to logs unnecessarily. The test job's approach at lines 68–76 provides a better pattern.Apply this diff to strengthen version extraction and match the pattern used in the test section:
- name: Extract version id: version run: | - VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') - echo "version=$VERSION" >> $GITHUB_OUTPUT - echo "Generated files for version: $VERSION" - cat netbird-*.diff + DIFF_FILE=$(find . -maxdepth 1 -name "netbird-*.diff" -type f 2>/dev/null | head -1) + if [ -z "$DIFF_FILE" ]; then + echo "ERROR: No diff file found" + exit 1 + fi + VERSION=$(basename "$DIFF_FILE" | sed 's/netbird-\(.*\)\.diff/\1/') + echo "version=$VERSION" >> $GITHUB_OUTPUT + echo "Generated files for version: $VERSION"This ensures only one file is processed and diff contents are not dumped to logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Signal / Unit (386)
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (1)
.github/workflows/release.yml (1)
94-101: Artifact upload looks good.The upload configuration correctly captures the generated FreeBSD port diff and issue body with a reasonable 30-day retention policy.
| pkg install -y git curl portlint go | ||
| # Install Go for building | ||
| GO_TARBALL="go1.24.10.freebsd-amd64.tar.gz" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go 1.24.10 download availability FreeBSD amd64
💡 Result:
Go 1.24.10 — there is no official prebuilt freebsd/amd64 binary on the Go downloads page. The go1.24.10 builds on the dl page include freebsd/arm, freebsd/arm64 and freebsd/riscv64 but not freebsd-amd64. [1][2]
Options: build from source (go1.24.10.src.tar.gz) or use a different release that provides freebsd-amd64 binaries (for example go1.25.5 does). [1][2]
Sources:
[1] go.dev — All releases (shows go1.25.5.freebsd-amd64).
[2] go.dev — All releases (shows go1.24.10 freebsd entries: arm, arm64, riscv64; no freebsd-amd64).
Update Go version to one with official freebsd-amd64 binary support.
Go 1.24.10 does not have an official prebuilt binary for freebsd-amd64 on the Go downloads page. Available architectures for 1.24.10 are freebsd/arm, freebsd/arm64, and freebsd/riscv64 only. This will cause the download to fail. Use a version like 1.25.5 that provides freebsd-amd64, or build from source using go1.24.10.src.tar.gz.
🤖 Prompt for AI Agents
.github/workflows/release.yml around line 54: the workflow sets GO_TARBALL to
"go1.24.10.freebsd-amd64.tar.gz" but Go 1.24.10 has no official freebsd-amd64
binary; update this to a release that provides freebsd-amd64 (for example change
to "go1.25.5.freebsd-amd64.tar.gz") or else point to the source tarball
"go1.24.10.src.tar.gz" and adjust the install steps to build from source; ensure
any GO_VERSION or related variables are kept consistent with the chosen tarball.
| make package | ||
| pkg add ./work/pkg/netbird-*.pkg | ||
| netbird version | grep $version |
There was a problem hiding this comment.
Quote the version variable in grep pattern.
The version variable used in the grep command is unquoted, which could cause issues if the extracted version contains regex special characters or spaces.
Apply this diff to fix the quoting:
- netbird version | grep $version
+ netbird version | grep "$version"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| netbird version | grep $version | |
| netbird version | grep "$version" |
🤖 Prompt for AI Agents
.github/workflows/release.yml around line 90: the grep command uses an unquoted
variable (netbird version | grep $version) which can misinterpret regex
metacharacters or spaces; update the command to quote the variable (use grep
"$version") so the extracted version is treated as a literal string, ensuring
correct matching and avoiding unexpected shell/grep behavior.
Describe your changes
Includes scripts for generating FreeBSD port update diff and issue body.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.