Skip to content

Conversation

@patrick-stephens
Copy link
Contributor

@patrick-stephens patrick-stephens commented Oct 28, 2025

Tweaks to install script to properly cope with packages.fluent.do URL and support various additional options.

Greptile Overview

Updated On: 2025-10-31 18:25:20 UTC

Greptile Summary

The install script has been significantly rewritten to support the new packages.fluent.do infrastructure. The changes successfully implement:

  • Dynamic version discovery from HTML index parsing
  • Interactive version selection mode (-i flag)
  • Enhanced OS/distro detection (Ubuntu, Debian, RHEL-based, Alpine)
  • Improved logging with color-coded output
  • Download-only mode (-d flag) and force installation option (-f flag)
  • Better error handling and package verification

Progress on Previous Issues:

  • ✅ Fixed: Line 497 DOWNLOAD_ONLY logic bug (changed != false to != true)
  • ✅ Fixed: Line 449 FORCE variable now uses safe parameter expansion ${FORCE:-}
  • ❌ Not Fixed: Lines 288-290 DISTRO_VERSION can still be undefined if distribution detection fails, creating malformed package paths like package-debian-.arm64v8

The script is functional for common cases but the remaining DISTRO_VERSION issue could cause failures on systems where /etc/os-release and /etc/lsb-release are both missing.

Confidence Score: 4/5

  • This PR is safe to merge with minor risk - two of three critical bugs were fixed, one undefined variable issue remains
  • Score reflects that 2 out of 3 previously identified critical bugs have been properly fixed (DOWNLOAD_ONLY logic and FORCE initialization). The remaining DISTRO_VERSION issue is an edge case that only affects systems without standard distribution detection files, making the overall risk low for production environments
  • install.sh requires minor attention - add default value for DISTRO_VERSION at lines 288-289 to prevent malformed paths when distribution detection fails

Important Files Changed

File Analysis

Filename Score Overview
install.sh 4/5 Major rewrite successfully implements dynamic version fetching from packages.fluent.do with interactive mode and enhanced platform detection. Two of three critical bugs from previous review were fixed (DOWNLOAD_ONLY logic and FORCE variable initialization), but DISTRO_VERSION undefined variable issue remains unresolved

Sequence Diagram

sequenceDiagram
    participant User
    participant Script as install.sh
    participant PackageServer as packages.fluent.do
    participant PkgMgr as Package Manager
    
    User->>Script: Execute install.sh [options]
    Script->>Script: Parse CLI arguments (-v, -i, -u, -d, -f, etc.)
    Script->>Script: Initialize FORCE, INTERACTIVE, DOWNLOAD_ONLY flags
    
    Script->>Script: detect_platform() - Detect OS & arch
    Note over Script: Sets OS_TYPE (linux/darwin) and ARCH_TYPE (amd64/arm64)
    
    Script->>Script: detect_distro() - Detect distro & pkg format
    Note over Script: Sets DISTRO_ID, DISTRO_VERSION, PKG_FORMAT, PKG_MANAGER
    
    alt Force flag not set and generic format
        Script->>User: Error: Unsupported distribution
    end
    
    Script->>PackageServer: fetch_available_versions() - GET /
    PackageServer-->>Script: Return HTML with data-version attributes
    Script->>Script: Parse HTML, extract & sort versions
    
    alt Interactive mode (-i)
        Script->>User: list_versions() - Display numbered list
        User-->>Script: Select version number or input version
        Script->>Script: Validate selection
    else Version specified (-v)
        Script->>Script: Use VERSION from CLI argument
    else Default
        Script->>Script: get_latest_version() - Pick first from sorted list
    end
    
    Script->>Script: find_package() - Build package search paths
    Note over Script: Maps DISTRO_ID to target_os (ubuntu/debian/almalinux/alpine)<br/>Maps ARCH_TYPE to target_arch (amd64/arm64v8)
    
    loop For each package directory pattern
        Script->>PackageServer: HTTP HEAD ${PACKAGES_URL}/${version}/output/${dir}/
        PackageServer-->>Script: Return HTTP status
        alt Directory exists (200/301/302)
            Script->>Script: Set matching_dir and break
        end
    end
    
    alt No matching directory
        Script->>User: Error: No package directory found
    end
    
    Script->>PackageServer: GET ${PACKAGES_URL}/${version}/output/${matching_dir}/
    PackageServer-->>Script: Return HTML directory listing
    Script->>Script: Extract package filename by format (.deb/.rpm/.apk)
    
    Script->>Script: Construct full package path
    
    Script->>Script: Create temp directory with trap for cleanup
    Script->>PackageServer: download_package() - curl -L -f
    PackageServer-->>Script: Download package to temp file
    Script->>Script: Verify file exists and non-empty
    
    alt Download only mode (-d flag)
        Script->>User: Package downloaded, skip install
    else Normal install
        Script->>PkgMgr: install_package() - Call apt-get/yum/apk
        Note over Script,PkgMgr: For deb: apt-get update && apt-get install<br/>For rpm: yum install<br/>For apk: apk add --allow-untrusted
        PkgMgr-->>Script: Installation result
        
        Script->>Script: verify_installation() - Test FLUENTDO_AGENT_BINARY
        alt Binary works
            Script->>User: Success with version info
        else Binary not found
            Script->>User: Warning: verification issues
        end
        
        Script->>User: Installation complete with next steps
    end
    
    Script->>Script: Cleanup temp directory (trap EXIT)
Loading

@patrick-stephens patrick-stephens marked this pull request as ready for review October 28, 2025 16:12
@patrick-stephens patrick-stephens requested a review from a team as a code owner October 28, 2025 16:12
niedbalski
niedbalski previously approved these changes Oct 28, 2025
Signed-off-by: Patrick Stephens <pat@fluent.do>
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

install.sh Outdated
Comment on lines 288 to 290
"package-${target_os}-${DISTRO_VERSION}.${target_arch}" # e.g., package-debian-bookworm.arm64v8
"package-${target_os}-${DISTRO_VERSION}" # e.g., package-debian-bookworm (amd64 default)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: DISTRO_VERSION may be undefined if detect_distro() returns early or fails. This causes empty path components like package-debian-.arm64v8

Suggested change
"package-${target_os}-${DISTRO_VERSION}.${target_arch}" # e.g., package-debian-bookworm.arm64v8
"package-${target_os}-${DISTRO_VERSION}" # e.g., package-debian-bookworm (amd64 default)
)
local package_dirs=(
"package-${target_os}-${DISTRO_VERSION:-unknown}.${target_arch}" # e.g., package-debian-bookworm.arm64v8
"package-${target_os}-${DISTRO_VERSION:-unknown}" # e.g., package-debian-bookworm (amd64 default)
)
Prompt To Fix With AI
This is a comment left during a code review.
Path: install.sh
Line: 288:290

Comment:
**logic:** `DISTRO_VERSION` may be undefined if `detect_distro()` returns early or fails. This causes empty path components like `package-debian-.arm64v8`

```suggestion
    local package_dirs=(
        "package-${target_os}-${DISTRO_VERSION:-unknown}.${target_arch}"   # e.g., package-debian-bookworm.arm64v8
        "package-${target_os}-${DISTRO_VERSION:-unknown}"                  # e.g., package-debian-bookworm (amd64 default)
    )
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptile-ai probably better to fail with an explanation, e.g. ${DISTRO_VERSION:?Unknown distribution}

patrick-stephens and others added 18 commits October 29, 2025 16:37
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: Pat <patrick.j.k.stephens@gmail.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: Pat <patrick.j.k.stephens@gmail.com>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
Signed-off-by: Patrick Stephens <pat@fluent.do>
@patrick-stephens patrick-stephens merged commit e9aae24 into main Oct 31, 2025
8 checks passed
@patrick-stephens patrick-stephens deleted the update_install_script branch October 31, 2025 18:22
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

# Construct candidate directory names for the package
# e.g., package-almalinux-8.arm64
# e.g., package-almalinux-8 (for amd64)
local package_dir="package-${target_os}-${DISTRO_VERSION}${target_arch_dir_suffix}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: DISTRO_VERSION can still be empty if detect_distro() returns early at line 263 when neither /etc/os-release nor /etc/lsb-release exist, creating malformed paths like package-debian-.arm64v8

Suggested change
local package_dir="package-${target_os}-${DISTRO_VERSION}${target_arch_dir_suffix}"
local package_dir="package-${target_os}-${DISTRO_VERSION:-unknown}${target_arch_dir_suffix}"
Prompt To Fix With AI
This is a comment left during a code review.
Path: install.sh
Line: 557:557

Comment:
**logic:** `DISTRO_VERSION` can still be empty if `detect_distro()` returns early at line 263 when neither `/etc/os-release` nor `/etc/lsb-release` exist, creating malformed paths like `package-debian-.arm64v8`

```suggestion
    local package_dir="package-${target_os}-${DISTRO_VERSION:-unknown}${target_arch_dir_suffix}"
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants