Conversation
Add install.sh that allows users to install the Honeybadger CLI with a single curl command. The script: - Detects OS (Linux/macOS) and architecture (x86_64/arm64) - Downloads the appropriate release from GitHub (latest by default) - Supports specifying a specific version with --version flag - Installs binary to /usr/local/bin - Configures systemd service for the metrics agent - Accepts API key via --api-key flag or prompts interactively - Supports --interval flag for custom reporting intervals - Includes --no-service flag for binary-only installation - Applies security hardening to the systemd service unit
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive bash installation script (install.sh) that enables one-command installation of the Honeybadger CLI from GitHub releases. The script handles platform detection, binary download and installation, and optional systemd service configuration for automated metrics reporting.
Changes:
- Adds install.sh with platform detection for Linux/macOS and architecture detection for x86_64/arm64
- Implements automatic download of CLI binaries from GitHub releases with version selection support
- Includes systemd service configuration with security hardening for continuous metrics reporting
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| VERSION="$2" | ||
| shift 2 | ||
| ;; | ||
| --interval) |
There was a problem hiding this comment.
The --interval flag is not validated. A user could provide a non-numeric value or a negative/zero value, which would result in invalid systemd service configuration. Add validation to ensure INTERVAL is a positive integer.
| --interval) | |
| --interval) | |
| if [[ -z "$2" ]]; then | |
| error "--interval requires a positive integer argument (seconds)" | |
| exit 1 | |
| fi | |
| if ! [[ "$2" =~ ^[0-9]+$ ]] || [ "$2" -le 0 ]; then | |
| error "Invalid --interval value: '$2'. Interval must be a positive integer (seconds)." | |
| exit 1 | |
| fi |
| if ! "${INSTALL_DIR}/${BINARY_NAME}" --version &> /dev/null; then | ||
| warn "Binary installed but version check failed" |
There was a problem hiding this comment.
The binary version check on line 425 redirects both stdout and stderr to /dev/null, but if it fails, the script only issues a warning and continues. If the binary is fundamentally broken or incompatible, it would be better to fail the installation. Consider making this a hard failure or at least checking the exit code more explicitly.
| if ! "${INSTALL_DIR}/${BINARY_NAME}" --version &> /dev/null; then | |
| warn "Binary installed but version check failed" | |
| if ! "${INSTALL_DIR}/${BINARY_NAME}" --version > /dev/null; then | |
| warn "Binary version check failed; aborting installation." | |
| exit 1 |
| create_systemd_service() { | ||
| local service_file="/etc/systemd/system/${SERVICE_NAME}.service" | ||
|
|
||
| info "Creating systemd service..." | ||
|
|
||
| cat > "$service_file" << EOF | ||
| [Unit] | ||
| Description=Honeybadger Agent - System Metrics Reporter | ||
| Documentation=https://github.com/honeybadger-io/cli | ||
| After=network-online.target | ||
| Wants=network-online.target | ||
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart=${INSTALL_DIR}/${BINARY_NAME} agent --interval ${INTERVAL} | ||
| Restart=always | ||
| RestartSec=10 | ||
| Environment="HONEYBADGER_API_KEY=${API_KEY}" | ||
|
|
||
| # Security hardening | ||
| NoNewPrivileges=true | ||
| ProtectSystem=strict | ||
| ProtectHome=read-only | ||
| PrivateTmp=true | ||
| ProtectKernelTunables=true | ||
| ProtectKernelModules=true | ||
| ProtectControlGroups=true | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target | ||
| EOF | ||
|
|
||
| # Secure the service file (contains API key) | ||
| chmod 600 "$service_file" | ||
|
|
||
| success "Systemd service created at ${service_file}" | ||
| } |
There was a problem hiding this comment.
The script does not handle the case where a systemd service with the same name already exists. If the user re-runs the installer or if the service already exists, the script will overwrite the service file and restart it without warning. Consider checking if the service exists and prompting the user whether to overwrite it, or at least stopping the service before overwriting the configuration.
| RestartSec=10 | ||
| Environment="HONEYBADGER_API_KEY=${API_KEY}" | ||
|
|
||
| # Security hardening |
There was a problem hiding this comment.
The comment "Security hardening" on line 315 could be more descriptive. Consider expanding it to explain what these security settings do and why they're important, especially since this is a template that users might modify. For example: "Security hardening: Restrict privileges and filesystem access to minimize attack surface".
| # Security hardening | |
| # Security hardening: Restrict privileges and filesystem access to minimize attack surface |
| --api-key) | ||
| API_KEY="$2" | ||
| shift 2 | ||
| ;; | ||
| --version) | ||
| VERSION="$2" | ||
| shift 2 | ||
| ;; | ||
| --interval) |
There was a problem hiding this comment.
The parse_args function should handle cases where a flag is provided without its required argument. For flags like --api-key, --version, and --interval, if they are the last argument or followed by another flag, $2 will be empty or another flag, which could lead to incorrect behavior. Add validation to ensure that required arguments are provided for these flags.
| --api-key) | |
| API_KEY="$2" | |
| shift 2 | |
| ;; | |
| --version) | |
| VERSION="$2" | |
| shift 2 | |
| ;; | |
| --interval) | |
| --api-key) | |
| if [[ -z "$2" || "$2" == --* ]]; then | |
| error "--api-key requires a value." | |
| usage | |
| fi | |
| API_KEY="$2" | |
| shift 2 | |
| ;; | |
| --version) | |
| if [[ -z "$2" || "$2" == --* ]]; then | |
| error "--version requires a value." | |
| usage | |
| fi | |
| VERSION="$2" | |
| shift 2 | |
| ;; | |
| --interval) | |
| if [[ -z "$2" || "$2" == --* ]]; then | |
| error "--interval requires a value." | |
| usage | |
| fi |
| echo "Binary installed: ${INSTALL_DIR}/${BINARY_NAME}" | ||
| echo "Version: ${VERSION}" | ||
|
|
||
| if [[ "$INSTALL_SERVICE" == true ]] && check_systemd; then |
There was a problem hiding this comment.
The print_summary function calls check_systemd again on line 368 without checking its return value correctly. The function is called within an if condition that will execute the then-branch if check_systemd returns 0 (success). However, if systemd was not available earlier in the installation, this check could produce confusing output since it will still display systemd-related messages even when the service was not actually installed.
| check_systemd() { | ||
| if ! command -v systemctl &> /dev/null; then | ||
| warn "systemd is not available on this system" | ||
| warn "Skipping service installation. You can run the agent manually with:" | ||
| echo " ${INSTALL_DIR}/${BINARY_NAME} agent --api-key YOUR_API_KEY" | ||
| return 1 | ||
| fi | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
The script uses 'set -e' at the beginning, which causes it to exit on any error. However, the check_systemd function returns 1 (failure) when systemd is not available. If check_systemd is called and returns 1, it will cause the script to exit due to 'set -e', even though the function is designed to handle this case gracefully. This is currently avoided because check_systemd is only called within if conditions, which prevents the exit, but this creates a subtle dependency that could break if the code is refactored.
| # Construct download URL | ||
| local archive_name="cli_${os}_${arch}.tar.gz" |
There was a problem hiding this comment.
The download URL construction assumes a specific naming convention (cli_OS_ARCH.tar.gz) but there's no validation that this matches the actual release asset naming on GitHub. If the release naming convention changes, the download will fail with a potentially confusing error message. Consider adding a comment documenting the expected naming convention, or implementing a fallback to list available assets and select the correct one.
| # Construct download URL | |
| local archive_name="cli_${os}_${arch}.tar.gz" | |
| # Expected asset naming convention: | |
| # cli_<os>_<arch>.tar.gz | |
| # We validate that this asset exists in the GitHub release before downloading, | |
| # so that changes to the release asset naming do not fail with a confusing error. | |
| local archive_name="cli_${os}_${arch}.tar.gz" | |
| local api_url="https://api.github.com/repos/${GITHUB_REPO}/releases/tags/${version}" | |
| # Try to locate the expected asset in the release assets | |
| local asset_name | |
| asset_name=$(curl -sSL "$api_url" \ | |
| | grep -o '"name":[[:space:]]*"[^"]*"' \ | |
| | sed 's/.*"name":[[:space:]]*"\([^"]*\)".*/\1/' \ | |
| | grep -m1 "^${archive_name}$" || true) | |
| if [ -z "$asset_name" ]; then | |
| error "Could not find expected release asset '${archive_name}' for version ${version}." | |
| info "Available assets for this release are:" | |
| curl -sSL "$api_url" \ | |
| | grep -o '"name":[[:space:]]*"[^"]*"' \ | |
| | sed 's/.*"name":[[:space:]]*"\([^"]*\)".*/ - \1/' | |
| error "The installer expects assets to be named 'cli_<os>_<arch>.tar.gz'." | |
| error "Please download the correct asset manually from:" | |
| error " https://github.com/${GITHUB_REPO}/releases/tag/${version}" | |
| exit 1 | |
| fi | |
| archive_name="$asset_name" |
| if [[ $EUID -ne 0 ]]; then | ||
| error "This script must be run as root (use sudo)" | ||
| exit 1 |
There was a problem hiding this comment.
The script requires root privileges (checked on line 405) but macOS users typically don't need root to install to /usr/local/bin if they own that directory. Consider making the root check conditional based on whether the INSTALL_DIR is writable by the current user, or at least document this requirement more clearly for macOS users who might be surprised by the sudo requirement.
| if [[ $EUID -ne 0 ]]; then | |
| error "This script must be run as root (use sudo)" | |
| exit 1 | |
| # Only require root if the install directory is not writable | |
| if [[ ! -w "$INSTALL_DIR" ]]; then | |
| if [[ $EUID -ne 0 ]]; then | |
| error "This script must be run as root (use sudo) to write to $INSTALL_DIR" | |
| exit 1 | |
| fi |
| if ! curl -sSL -o "${tmp_dir}/${archive_name}" "$download_url"; then | ||
| error "Failed to download from: $download_url" |
There was a problem hiding this comment.
The curl command on line 240 uses -sSL flags where -S shows errors even in silent mode. However, the error handling relies on the exit code from curl. If the download fails due to network issues or a 404, the error message "Failed to download from: URL" might not provide enough context about what went wrong. Consider capturing and displaying curl's error output to help users diagnose download failures.
| if ! curl -sSL -o "${tmp_dir}/${archive_name}" "$download_url"; then | |
| error "Failed to download from: $download_url" | |
| local curl_error_log="${tmp_dir}/curl_error.log" | |
| if ! curl -sSL -S -o "${tmp_dir}/${archive_name}" "$download_url" 2>"${curl_error_log}"; then | |
| local curl_status=$? | |
| if [ -s "${curl_error_log}" ]; then | |
| error "Failed to download from: $download_url" | |
| error "curl error (${curl_status}): $(cat "${curl_error_log}")" | |
| else | |
| error "Failed to download from: $download_url (curl exit code ${curl_status})" | |
| fi |
Add install.sh that allows users to install the Honeybadger CLI with a single curl command. The script: