Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds automated installation support for Microsoft LISA, providing quick-start scripts for Windows, Linux, and Docker-based deployments. The changes aim to simplify the LISA installation process across different platforms and reduce manual setup complexity.
Changes:
- Added three installation automation scripts: quick-install.sh (Linux), quick-install.ps1 (Windows), and quick-container.sh (Docker container management)
- Updated installation documentation (installation_linux.rst, docker_linux.rst) to include quick-start options
- Created comprehensive INSTALL.md guide covering prerequisites, installation options, and troubleshooting for both platforms
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| quick-install.sh | Bash script automating LISA installation on Linux with distribution detection, Python version management, and virtual environment support for Ubuntu 24.04+ |
| quick-install.ps1 | PowerShell script automating LISA installation on Windows with automatic Python and Git installation via winget or direct download |
| quick-container.sh | Bash script for running LISA in Docker containers with support for runbooks, authentication, log mounting, and Docker/Azure CLI installation |
| docs/installation_linux.rst | Updated with quick installation script usage, Docker quick-start section, and Ubuntu 24.04+ virtual environment guidance |
| docs/docker_linux.rst | Added quick-container script documentation with comprehensive examples and automatic Docker installation instructions |
| INSTALL.md | New comprehensive installation guide with prerequisites, platform-specific instructions, verification steps, and troubleshooting section |
| if [ -n "$ACCESS_TOKEN" ]; then | ||
| LISA_VARIABLES+=("azure_arm_access_token:$ACCESS_TOKEN") |
There was a problem hiding this comment.
Sensitive credentials (ACCESS_TOKEN) are being passed through command-line arguments and may be exposed in process listings, shell history, or logs. The command is echoed on line 522, which could expose the token. Consider warning users about this security implication in the help text, or use environment variables instead of command-line arguments for sensitive data.
| if [ -n "$ACCESS_TOKEN" ]; then | |
| LISA_VARIABLES+=("azure_arm_access_token:$ACCESS_TOKEN") | |
| # Prefer environment variable for access token to avoid exposing it via command-line arguments | |
| if [ -n "$AZURE_ARM_ACCESS_TOKEN" ]; then | |
| LISA_VARIABLES+=("azure_arm_access_token:$AZURE_ARM_ACCESS_TOKEN") |
| 5. **Install LISA with Azure extensions** | ||
| ```bash | ||
| pip3 install -e .[azure] |
There was a problem hiding this comment.
The installation command uses the short form pip install -e .[azure] but in the scripts and other documentation, the more explicit form pip install --editable .[azure,libvirt] is used with libvirt included. This inconsistency could lead to users having different extension sets installed. Consider standardizing on one approach and clarifying which extensions are included.
| 5. **Install LISA with Azure extensions** | |
| ```bash | |
| pip3 install -e .[azure] | |
| 5. **Install LISA with Azure and libvirt extensions** | |
| ```bash | |
| pip3 install --editable .[azure,libvirt] |
| else { | ||
| # Fallback: Download and install Python directly | ||
| Write-Host " winget not found. Downloading Python 3.12 installer..." -ForegroundColor Yellow | ||
| $pythonUrl = "https://www.python.org/ftp/python/3.12.8/python-3.12.8-amd64.exe" |
There was a problem hiding this comment.
The hardcoded Python installer URL points to version 3.12.8, which may become outdated. Consider making this version configurable or using a mechanism to fetch the latest 3.12.x version dynamically to ensure users get security updates and bug fixes.
| if (Test-Path $InstallPath) { | ||
| Write-Host " Directory $InstallPath already exists" -ForegroundColor Yellow | ||
| $response = Read-Host " Do you want to remove it and re-clone? (y/N)" | ||
| if ($response -eq 'y' -or $response -eq 'Y') { | ||
| Remove-Item -Path $InstallPath -Recurse -Force | ||
| Write-Host " Cloning LISA repository..." -ForegroundColor Yellow | ||
| # Temporarily allow errors for git clone (it outputs to stderr) | ||
| $prevErrorPref = $ErrorActionPreference | ||
| $ErrorActionPreference = "Continue" | ||
| & git clone --branch $Branch https://github.com/microsoft/lisa.git $InstallPath 2>&1 | Out-Null | ||
| $ErrorActionPreference = $prevErrorPref | ||
| } | ||
| else { | ||
| Write-Host " Using existing directory..." -ForegroundColor Yellow | ||
| $needsClone = $false | ||
| } |
There was a problem hiding this comment.
When the directory already exists, the script prompts the user but doesn't validate the clone was successful after the user chooses 'N'. If the existing directory is corrupted or incomplete, the installation will proceed with a broken state. Consider adding validation of the existing directory structure (e.g., checking for pyproject.toml) before using it.
| NEEDS_CLONE=true | ||
|
|
||
| if [ -d "$INSTALL_PATH" ]; then | ||
| echo " Directory $INSTALL_PATH already exists, removing it..." | ||
| rm -rf "$INSTALL_PATH" | ||
| echo " Cloning LISA repository..." | ||
| git clone --branch "$BRANCH" https://github.com/microsoft/lisa.git "$INSTALL_PATH" --quiet | ||
| fi | ||
|
|
||
| if [ "$NEEDS_CLONE" = true ] && [ ! -d "$INSTALL_PATH" ]; then | ||
| echo " Cloning LISA repository to $INSTALL_PATH..." | ||
| git clone --branch "$BRANCH" https://github.com/microsoft/lisa.git "$INSTALL_PATH" --quiet | ||
| fi |
There was a problem hiding this comment.
The variable NEEDS_CLONE is set to true but never set to false before the conditional check on line 358. This means the clone will always be attempted even after removing and re-cloning the directory on lines 352-355. The logic should set NEEDS_CLONE=false after the successful clone on line 355.
| # Add LISA variables | ||
| for var in "${LISA_VARIABLES[@]}"; do | ||
| DOCKER_CMD="$DOCKER_CMD -v \"$var\"" | ||
| done | ||
| fi | ||
|
|
||
| # Show the command being executed | ||
| print_info "Executing Docker command:" | ||
| echo " $DOCKER_CMD" | ||
| echo "" | ||
|
|
||
| # Execute the command | ||
| eval "$DOCKER_CMD" |
There was a problem hiding this comment.
The command string is being built with quoted variables inside, but then evaluated with 'eval'. This can lead to word splitting issues. On line 516, variables are being added with quotes inside the string: DOCKER_CMD="$DOCKER_CMD -v "$var"". When eval executes this, the quotes may not work as intended if the variable contains spaces or special characters. Consider using an array to build the command instead of a string, or ensure proper escaping.
| PYTHON_BIN=$(which "python${PYTHON_VERSION}" 2>/dev/null || find /usr -name "python${PYTHON_VERSION}" 2>/dev/null | head -1) | ||
|
|
||
| if [ -z "$PYTHON_BIN" ]; then | ||
| echo " [ERROR] Python ${PYTHON_VERSION} installation failed - binary not found" | ||
| echo " [INFO] Searching for installed Python versions..." | ||
| find /usr -name "python3.*" -type f 2>/dev/null | head -10 |
There was a problem hiding this comment.
The find command searches all of /usr which can be slow on large systems and may find non-executable files or test files. Consider limiting the search to common Python installation directories like /usr/bin or /usr/local/bin to improve performance and accuracy.
| PYTHON_BIN=$(which "python${PYTHON_VERSION}" 2>/dev/null || find /usr -name "python${PYTHON_VERSION}" 2>/dev/null | head -1) | |
| if [ -z "$PYTHON_BIN" ]; then | |
| echo " [ERROR] Python ${PYTHON_VERSION} installation failed - binary not found" | |
| echo " [INFO] Searching for installed Python versions..." | |
| find /usr -name "python3.*" -type f 2>/dev/null | head -10 | |
| PYTHON_BIN=$(which "python${PYTHON_VERSION}" 2>/dev/null || find /usr/bin /usr/local/bin -maxdepth 1 -type f -executable -name "python${PYTHON_VERSION}" 2>/dev/null | head -1) | |
| if [ -z "$PYTHON_BIN" ]; then | |
| echo " [ERROR] Python ${PYTHON_VERSION} installation failed - binary not found" | |
| echo " [INFO] Searching for installed Python versions..." | |
| find /usr/bin /usr/local/bin -maxdepth 1 -type f -name "python3.*" 2>/dev/null | head -10 |
| # Navigate to the lisa directory | ||
| cd path\to\lisa | ||
|
|
||
| # Run the installation script |
There was a problem hiding this comment.
The documentation instructs users to first navigate to the LISA directory, then run the installation script. However, the script is designed to clone the LISA repository itself. This creates confusion - users would need to have already downloaded the script separately, or cloned the repository, which defeats the purpose of the automated installation. Consider clarifying whether users should download just the script file or if they already have the repository.
| # Navigate to the lisa directory | |
| cd path\to\lisa | |
| # Run the installation script | |
| # Download the LISA quick-install script (no prior clone required) | |
| Invoke-WebRequest ` | |
| -Uri "https://raw.githubusercontent.com/microsoft/lisa/main/quick-install.ps1" ` | |
| -OutFile "quick-install.ps1" | |
| # Run the installation script (it will clone LISA to the install path) |
| echo " [ERROR] LISA installation failed" | ||
| echo "" | ||
| echo " Common causes:" | ||
| echo " 1. Python version too old (need 3.8+, have $CURRENT_VERSION)" |
There was a problem hiding this comment.
There's an inconsistency in the minimum Python version requirement. Line 99 warns that "LISA requires Python 3.9+", but lines 376 and 422 state "LISA requires Python 3.8+". This conflicting information could confuse users. The requirements should be consistent throughout the script - verify the actual minimum version and update all messages accordingly.
| echo " 1. Python version too old (need 3.8+, have $CURRENT_VERSION)" | |
| echo " 1. Python version too old (need 3.9+, have $CURRENT_VERSION)" |
| # Add LISA variables | ||
| for var in "${LISA_VARIABLES[@]}"; do | ||
| DOCKER_CMD="$DOCKER_CMD -v \"$var\"" | ||
| done | ||
| fi | ||
|
|
||
| # Show the command being executed | ||
| print_info "Executing Docker command:" | ||
| echo " $DOCKER_CMD" |
There was a problem hiding this comment.
The constructed DOCKER_CMD string includes all LISA_VARIABLES (including azure_arm_access_token from --token and any other secrets passed via -v) and is then echoed verbatim, which will print sensitive bearer tokens and credentials to the console and any logs capturing this output. An attacker with access to terminal scrollback, CI logs, or log files could reuse these tokens to access your Azure subscription or other resources. To prevent this, avoid logging secrets by either not including sensitive values in DOCKER_CMD when printing it or masking/removing secret-valued variables before echoing the command.
|
|
||
| .. code:: bash | ||
|
|
||
| bash quick-container.sh --install-docker |
There was a problem hiding this comment.
working command
bash quick-container.sh --install-docker --interactive
| @@ -55,6 +65,114 @@ Use below command to launch the LISA container. | |||
|
|
|||
| docker run --rm -i mcr.microsoft.com/lisa/runtime:latest lisa -r ./examples/runbook/hello_world.yml | |||
There was a problem hiding this comment.
lisa/examples/runbook/hello_world.yml
docker run --rm -i mcr.microsoft.com/lisa/runtime:latest lisa -r ./examples/runbook/hello_world.yml threw no such file or directory
2 participants
No description provided.