create ingest script for windows and linux/mac #1489
Conversation
WalkthroughAdds two new Fluent Bit management scripts: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (9)
scripts/ingest.sh (4)
115-125: Consider checking if the binary is executable.
get_fluent_bit_binfinds the binary path but doesn't verify it's executable. If the file exists but isn't executable, downstream commands will fail with a confusing error.get_fluent_bit_bin() { if [ -f /opt/fluent-bit/bin/fluent-bit ]; then echo "/opt/fluent-bit/bin/fluent-bit" elif [ -f /opt/homebrew/bin/fluent-bit ]; then echo "/opt/homebrew/bin/fluent-bit" elif [ -f /usr/local/bin/fluent-bit ]; then echo "/usr/local/bin/fluent-bit" else - which fluent-bit 2>/dev/null || echo "fluent-bit" + command -v fluent-bit 2>/dev/null || echo "fluent-bit" fi }Using
command -vis more portable thanwhich.
145-147: Remove unusedFINAL_VERSIONvariable.The variable is assigned but never used, as flagged by ShellCheck (SC2034).
FLUENT_BIT_BIN=$(get_fluent_bit_bin) - FINAL_VERSION=$("$FLUENT_BIT_BIN" --version 2>/dev/null | head -n1 | sed -n 's/.*v\([0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\).*/\1/p' || echo "unknown") nohup "$FLUENT_BIT_BIN" -c "$CONFIG_FILE" > "$LOG_FILE" 2>&1 &
59-67: Loop variableiis unused.ShellCheck (SC2034) correctly notes that
iis never used. Consider using_as a throwaway variable.- for i in {1..10}; do + for _ in {1..10}; do
280-291: Remove unusedNEW_VERSIONvariable.
NEW_VERSIONis assigned but never used in both branches (lines 284, 290). ShellCheck (SC2034) flagged this.if version_gt "$MIN_VERSION" "$CURRENT_VERSION"; then install_fluent_bit # Clear command hash to get updated binary hash -r 2>/dev/null || true - NEW_VERSION=$(get_fluent_bit_version) fi else install_fluent_bit # Clear command hash to get updated binary hash -r 2>/dev/null || true - NEW_VERSION=$(get_fluent_bit_version) fiscripts/ingest.ps1 (5)
117-120: Assigned$versionis unused; consider removing or logging.The version is captured but never used or displayed to the user.
if (Test-Path $FLUENT_BIT_EXE) { - $version = & $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1 + Write-Info "Fluent Bit already installed: $(& $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1)" return }
162-163: Assigned$installedVersionis unused.Similar to other unused version variables, this is assigned but never used.
- $installedVersion = & $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1 + Write-Info "Installed Fluent Bit: $(& $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1)"
189-189: Unused$versionvariable inStart-FluentBit.The version is retrieved but never used.
- $version = & $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1Or display it to the user if intended.
26-29: Function nameWrite-Warningshadows built-in cmdlet.PowerShell has a built-in
Write-Warningcmdlet. This custom function shadows it, which may cause unexpected behavior if other code expects the standard cmdlet behavior.-function Write-Warning { +function Write-WarningMsg { param([string]$Message) Write-Host "[WARNING] $Message" -ForegroundColor Yellow }Update all calls from
Write-WarningtoWrite-WarningMsg.
286-288: Consider setting restrictive ACLs on the config file.The config file contains credentials. Setting restrictive permissions protects against unauthorized access.
$utf8NoBom = New-Object System.Text.UTF8Encoding $false [System.IO.File]::WriteAllText($CONFIG_FILE, $configContent, $utf8NoBom) + + # Restrict config file permissions to current user only + $acl = Get-Acl $CONFIG_FILE + $acl.SetAccessRuleProtection($true, $false) + $rule = New-Object System.Security.AccessControl.FileSystemAccessRule( + [System.Security.Principal.WindowsIdentity]::GetCurrent().Name, + "FullControl", + "Allow" + ) + $acl.SetAccessRule($rule) + Set-Acl $CONFIG_FILE $acl
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/ingest.ps1(1 hunks)scripts/ingest.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nitisht
Repo: parseablehq/parseable PR: 1397
File: scripts/download.sh:5-6
Timestamp: 2025-08-01T09:01:14.693Z
Learning: The parseablehq/parseable project uses separate download scripts for different platforms - there's a dedicated Windows script, so the main download.sh script in scripts/download.sh is only intended for Unix-like systems (Linux and macOS).
🪛 Shellcheck (0.11.0)
scripts/ingest.sh
[warning] 60-60: i appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 146-146: FINAL_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 290-290: NEW_VERSION appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (9)
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: coverage
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (6)
scripts/ingest.sh (4)
1-27: LGTM! Clean script setup and configuration.The shebang, usage documentation, color definitions, and file path constants are well-organized. Using relative paths for PID, log, and config files keeps things simple for local usage.
28-50: LGTM! Helper functions and process check are correct.The
is_runningfunction properly checks for PID file existence and validates the process is actually running viaps -p.
53-78: LGTM! Graceful shutdown with fallback to force-kill.The stop function handles the graceful → force-kill pattern correctly with a 10-second timeout.
248-256: Base64 error handling behavior differs across platforms; verify on macOS.The error check on line 250 (
if ! CREDENTIALS=$(...)) relies onbase64 -dreturning a non-zero exit code for invalid input. While this works correctly on Linux, macOS's BSD-basedbase64implementation may have different behavior. Consider testing this on macOS to confirm the error handling is reliable across all deployment platforms, or add explicit validation of the decoded output format.scripts/ingest.ps1 (2)
264-270: Windows script useswindows_exporter_metrics, shell script usesnode_exporter_metrics.This is a valid platform difference, but the
Scrape_intervaldiffers (1 vs 2 seconds) and Windows config only collects CPU metrics while Linux collects all. Consider aligning the configurations or documenting the intentional differences.
36-50: LGTM! Process running check is robust.The
Test-FluentBitRunningfunction properly handles missing PID file and invalid process IDs with try-catch.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/ingest.sh (1)
227-232: Critical: Missing case terminator causes syntax error.The
ubuntu|debiancase is missing the;;terminator after line 231, causing shellcheck errors and preventing the script from parsing correctly.Apply this diff to fix the syntax error:
ubuntu|debian) print_info "Installing Fluent Bit on Ubuntu/Debian..." curl -fsSL https://raw.githubusercontent.com/fluent/fluent-bit/master/install.sh -o /tmp/install-fluentbit.sh chmod +x /tmp/install-fluentbit.sh /tmp/install-fluentbit.sh + ;; centos|rhel|fedora)
🧹 Nitpick comments (5)
scripts/ingest.sh (2)
145-146: Unused variableFINAL_VERSION.The version is extracted but never used. Consider removing this line or using it for logging/validation.
- FLUENT_BIT_BIN=$(get_fluent_bit_bin) - FINAL_VERSION=$("$FLUENT_BIT_BIN" --version 2>/dev/null | head -n1 | sed -n 's/.*v\([0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\).*/\1/p' || echo "unknown") - + FLUENT_BIT_BIN=$(get_fluent_bit_bin) +
177-197: Reduce code duplication.The
get_fluent_bit_versionfunction duplicates the path-finding logic fromget_fluent_bit_bin. Consider callingget_fluent_bit_bininstead.function get_fluent_bit_version() { local version_output - local fluent_bit_cmd - - # Try different installation locations - if [ -f /opt/fluent-bit/bin/fluent-bit ]; then - fluent_bit_cmd="/opt/fluent-bit/bin/fluent-bit" - elif [ -f /opt/homebrew/bin/fluent-bit ]; then - fluent_bit_cmd="/opt/homebrew/bin/fluent-bit" - elif [ -f /usr/local/bin/fluent-bit ]; then - fluent_bit_cmd="/usr/local/bin/fluent-bit" - else - fluent_bit_cmd="fluent-bit" - fi + local fluent_bit_cmd=$(get_fluent_bit_bin) version_output=$($fluent_bit_cmd --version 2>/dev/null | head -n1) # Extract version using sed (portable across macOS and Linux) echo "$version_output" | sed -n 's/.*v\([0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\).*/\1/p' || echo "0.0.0" }scripts/ingest.ps1 (3)
117-119: Unused version variable.The version is retrieved but never used. Consider removing this or using it for validation/logging.
if (Test-Path $FLUENT_BIT_EXE) { - $version = & $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1 return }
189-190: Unused version variable.The version is retrieved but never used. Consider removing this line.
- $version = & $FLUENT_BIT_EXE --version 2>$null | Select-Object -First 1 - # Start Fluent Bit process in background (no logging)
229-293: Good credential handling, but consider restricting config file permissions.The function properly uses parameter variables for credentials (past review concern is resolved). However, consider setting restrictive ACLs on the config file to prevent other users from reading credentials.
Add after line 289:
# Set restrictive permissions (owner only) $acl = Get-Acl $CONFIG_FILE $acl.SetAccessRuleProtection($true, $false) $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule( [System.Security.Principal.WindowsIdentity]::GetCurrent().Name, "FullControl", "Allow" ) $acl.SetAccessRule($accessRule) Set-Acl $CONFIG_FILE $acl
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/ingest.ps1(1 hunks)scripts/ingest.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nitisht
Repo: parseablehq/parseable PR: 1397
File: scripts/download.sh:5-6
Timestamp: 2025-08-01T09:01:14.693Z
Learning: The parseablehq/parseable project uses separate download scripts for different platforms - there's a dedicated Windows script, so the main download.sh script in scripts/download.sh is only intended for Unix-like systems (Linux and macOS).
📚 Learning: 2025-08-01T09:01:14.693Z
Learnt from: nitisht
Repo: parseablehq/parseable PR: 1397
File: scripts/download.sh:5-6
Timestamp: 2025-08-01T09:01:14.693Z
Learning: The parseablehq/parseable project uses separate download scripts for different platforms - there's a dedicated Windows script, so the main download.sh script in scripts/download.sh is only intended for Unix-like systems (Linux and macOS).
Applied to files:
scripts/ingest.ps1
📚 Learning: 2025-09-09T14:08:45.809Z
Learnt from: nikhilsinhaparseable
Repo: parseablehq/parseable PR: 1427
File: resources/ingest_demo_data.sh:440-440
Timestamp: 2025-09-09T14:08:45.809Z
Learning: In the resources/ingest_demo_data.sh demo script, hardcoded stream names like "demodata" in alert queries should be ignored and not flagged for replacement with $P_STREAM variables.
Applied to files:
scripts/ingest.ps1
🪛 Shellcheck (0.11.0)
scripts/ingest.sh
[error] 227-227: Couldn't parse this case item. Fix to allow more checks.
(SC1073)
[error] 233-233: Fix any mentioned problems and try again.
(SC1072)
[error] 233-233: Did you forget the ;; after the previous case item?
(SC1074)
⏰ 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). (9)
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Kafka aarch64-apple-darwin
🔇 Additional comments (13)
scripts/ingest.sh (8)
1-27: LGTM!The script setup is clean with appropriate error handling via
set -e, clear color codes, and well-organized file path definitions.
28-50: LGTM!The utility functions for output and process checking are well-implemented. The
is_runningfunction correctly validates both PID file existence and actual process status.
52-78: LGTM!The stop function implements proper graceful shutdown with a 10-second timeout before force-killing, and correctly cleans up the PID file.
80-112: LGTM!Status and log viewing functions provide clear information with good user guidance.
114-125: LGTM!The function checks multiple installation paths appropriately for different platforms (Homebrew, standard Linux paths).
199-213: LGTM!OS detection logic correctly handles macOS and Linux distributions.
297-322: Good security practices implemented.The function properly secures the config file with
chmod 600and redacts the password when displaying the configuration. The credential handling is secure.
328-392: LGTM!The command routing and help text provide a clear, user-friendly interface with comprehensive usage examples.
scripts/ingest.ps1 (5)
1-20: LGTM!Script initialization is well-structured with appropriate path handling for both per-user installation and script-relative config files.
21-92: LGTM!The helper functions and lifecycle management are well-implemented with proper error handling and stale PID cleanup.
94-106: LGTM!Architecture detection is straightforward and correct for Windows.
295-327: LGTM!Help and debug functions provide clear guidance and useful troubleshooting capabilities.
329-367: LGTM!The command routing logic is clean and provides clear error messages for incorrect usage.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.