Skip to content

Conversation

orknist
Copy link

@orknist orknist commented Oct 7, 2025

📄 Summary

This PR improves the robustness and error handling of the installation script (deploy/install.sh), making it more compatible with different environments (especially WSL) and providing better user feedback when issues occur.

✅ Changes

  • Enhancement: Add WSL (Windows Subsystem for Linux) compatibility for BASE_DIR detection using realpath fallback
  • Enhancement: Improve Docker startup checks for various environments (Mac, Linux, WSL)
  • Enhancement: Add detailed error messages for unsupported OS and installation issues
  • Enhancement: Improve error handling for analytics event sending (non-blocking with timeout)
  • Enhancement: Add version checks and fallback mechanisms for docker compose
  • Enhancement: Add directory existence validation before switching directories
  • Bug fix: Suppress unnecessary error output in various system checks

🏷️ Required: Add Relevant Labels

  • devops
  • enhancement
  • bug

👥 Reviewers

  • devops

🧪 How to Test

  1. Test on WSL environment:

    • Run the install script on Windows Subsystem for Linux
    • Verify BASE_DIR is correctly detected
    • Verify Docker Desktop integration check works properly
  2. Test on native Linux:

    • Run the install script on Ubuntu/Debian
    • Verify Docker service starts correctly
    • Verify systemctl commands work as expected
  3. Test on macOS:

    • Run the install script
    • Verify Docker Desktop starts properly
  4. Test error scenarios:

    • Run on unsupported OS and verify clear error message
    • Run without Docker and verify installation flow
    • Test with missing directories and verify error handling
  5. Test docker compose detection:

    • Test with docker compose plugin
    • Test with standalone docker-compose
    • Verify version detection works

📋 Checklist

  • Dev Review
  • Test cases added (Unit/ Integration / E2E)
  • Manually tested the changes

👀 Notes for Reviewers

Key improvements:

  1. WSL Support: The script now properly detects and handles WSL environments, providing clear instructions when Docker Desktop integration is needed.

  2. Better Error Messages: Users will now see helpful error messages instead of cryptic failures, especially for:

    • Unsupported OS detection
    • Missing directories
    • Docker accessibility issues
  3. Non-blocking Analytics: Analytics events now have a 3-second timeout and won't block the installation if they fail.

  4. Defensive Programming: Added multiple safety checks (directory existence, command availability, etc.) to prevent unexpected failures.

  5. Cleaner Output: Suppressed unnecessary error messages (like /etc/*-release not found) that were confusing users.

Testing focus areas:

  • WSL environment behavior
  • Error message clarity and helpfulness
  • Docker startup logic across different platforms

Important

Enhance deploy/install.sh for better WSL compatibility, error handling, and Docker checks.

  • WSL Compatibility:
    • Use realpath as a fallback for BASE_DIR detection in deploy/install.sh.
    • Add checks for Docker Desktop integration in WSL.
  • Error Handling:
    • Add detailed error messages for unsupported OS and missing directories.
    • Suppress unnecessary error output in system checks.
    • Improve error handling for analytics event sending with a 3-second timeout.
  • Docker Checks:
    • Improve Docker startup checks for Mac, Linux, and WSL.
    • Add version checks and fallback mechanisms for docker compose.
    • Validate directory existence before switching directories.

This description was created by Ellipsis for 27187f5. You can customize this summary. It will automatically update as commits are pushed.

- Add WSL compatibility for BASE_DIR detection using realpath
- Enhance Docker startup checks for various environments (Mac, Linux, WSL)
- Add more detailed error messages for unsupported OS and installation issues
- Improve error handling for analytics event sending
- Add version checks and fallback mechanisms for docker compose
- Add additional error checking for directory and command availability
- Make analytics event sending non-blocking with timeout
- Suppress unnecessary error output in various checks
@orknist orknist requested a review from a team as a code owner October 7, 2025 08:29
Copy link

welcome bot commented Oct 7, 2025

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 27187f5 in 1 minute and 8 seconds. Click for details.
  • Reviewed 152 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. deploy/install.sh:7
  • Draft comment:
    Consider using an explicit command check for readlink success rather than relying on assignment in the if statement. This would improve clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. deploy/install.sh:264
  • Draft comment:
    The native Linux branch uses a check on [[ -z $sudo_cmd ]] to determine if sudo is required. Verify if this logic correctly handles cases where the user isn’t running with sudo, as it might lead to permission issues with systemctl commands.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify if the logic correctly handles a specific case. This falls under the rule of not asking the author to double-check or verify their intention. Therefore, this comment should be removed.
3. deploy/install.sh:447
  • Draft comment:
    Instead of silently returning on unknown event types in send_event, consider logging a warning to support troubleshooting.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. deploy/install.sh:452
  • Draft comment:
    Initialize the variable 'others' to an empty string at the start of send_event to avoid unintended spacing or issues when it's not set.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
5. deploy/install.sh:606
  • Draft comment:
    Using a blocking 'read' for email input might cause issues in non-interactive environments. Consider making this step optional or adding a timeout.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_TJ0p5UPMHQq3vgSY

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

2 participants