-
Notifications
You must be signed in to change notification settings - Fork 668
Refactor CLI download scripts to extract shared code blocks #10881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… and custom help handler
…empDirectory, Get-RuntimeIdentifier) and use them in Install-AspireCli
…remove OS param from Expand-AspireCliArchive; default install path to ~/.aspire
- Group shared helper functions into a clearly marked shared block - No behavior changes; function signatures preserved - Align help text default path with ~/.aspire
…tifier, new_temp_dir, remove_temp_dir) and update usages (RID + temp dir)
…/.tar.gz); remove OS dependency in installer; default install path to ~/.aspire; help text aligned
- Group shared helper functions into a clearly marked shared block - No behavior changes; code only reorganized for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Aspire CLI download scripts to improve code organization by extracting shared functionality into clearly marked code blocks, preparing the foundation for code reuse in future features.
- Extracts common helper functions into shared code sections marked with START/END comments
- Enhances documentation with comprehensive PowerShell help and improved argument parsing in Bash
- Improves cross-platform compatibility with better archive detection and platform-specific handling
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
eng/scripts/get-aspire-cli.sh | Refactored to extract shared helper functions, improved argument parsing with renamed variables, and enhanced cross-platform archive handling |
eng/scripts/get-aspire-cli.ps1 | Added comprehensive comment-based help documentation, extracted shared functions, and improved code organization with better separation of concerns |
DEFAULT_QUALITY="release" | ||
|
||
# Function to show help | ||
show_help() { | ||
cat << 'EOF' | ||
cat << EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using unquoted EOF delimiter makes the heredoc subject to variable expansion. This could cause issues if variables contain special characters. Use quoted 'EOF' to prevent expansion: cat << 'EOF'
Copilot uses AI. Check for mistakes.
@@ -461,7 +367,7 @@ add_to_path() | |||
elif [[ -f "$config_file" ]] && grep -Fxq "$command" "$config_file"; then | |||
say_info "Command already exists in $config_file, skipping addition" | |||
elif [[ -w $config_file ]]; then | |||
echo -e "\n# Added by get-aspire-cli.sh" >> "$config_file" | |||
echo -e "\n# Added by get-aspire-cli*.sh script" >> "$config_file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The wildcard pattern in the comment 'get-aspire-cli*.sh script' is unclear and doesn't match the actual filename pattern. Consider using a more descriptive comment like 'Added by Aspire CLI installation script'.
echo -e "\n# Added by get-aspire-cli*.sh script" >> "$config_file" | |
echo -e "\n# Added by Aspire CLI installation script" >> "$config_file" |
Copilot uses AI. Check for mistakes.
if [[ "$DRY_RUN" != true ]]; then | ||
printf "\nTo use the Aspire CLI in new terminal sessions, restart your terminal or run:\n" | ||
say_info " source $config_file" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition will execute the block when DRY_RUN is false, but the following printf and say_info statements should be shown in both dry-run and normal execution modes since they are informational messages about what to do after installation.
if [[ "$DRY_RUN" != true ]]; then | |
printf "\nTo use the Aspire CLI in new terminal sessions, restart your terminal or run:\n" | |
say_info " source $config_file" | |
fi | |
printf "\nTo use the Aspire CLI in new terminal sessions, restart your terminal or run:\n" | |
say_info " source $config_file" |
Copilot uses AI. Check for mistakes.
This PR refactors the Aspire CLI download scripts (
get-aspire-cli.ps1
andget-aspire-cli.sh
) to improve code organization and prepare for code reuse in upcoming features.Key Changes
Code Organization & Reusability
# START: Shared code
and# END: Shared code
markers to identify reusable componentsEnhanced Functionality
Improved Helper Functions
Get-RuntimeIdentifier
,New-TempDirectory
,Remove-TempDirectory
, andGet-DefaultInstallPrefix
get_runtime_identifier
,new_temp_dir
,remove_temp_dir
, and improved platform detection functionsCode Quality Improvements
-Help
parameter in PowerShell) in favor of standard help mechanismsImpact
This refactoring maintains full backward compatibility while making the codebase more maintainable and setting up the foundation for a future PR that will introduce
get-aspire-cli-pr
scripts to download CLI builds from specific pull requests, leveraging these shared building blocks.