Add packages command to catalog Sparkdock-managed packages by category#254
Add packages command to catalog Sparkdock-managed packages by category#254
Conversation
Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
|
@copilot we can make it way simpler than this. We can extend the all-packages.yml file to include static metadata, get rid of the script that uses brew, for now keep it simple. |
Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Simplified as requested. Changes in commit 10a7e2c: What changed:
New structure: cask_packages:
- name: docker-desktop
category: Container Tools
description: Docker Desktop - Container development platformOutput now shows 3 columns instead of 4: Package | Category | Description |
|
@copilot, add to the custom instructions that any new package added must include the category and a description. |
Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Added to Updated the Package Management Pattern section to:
|
|
It would be nice to have an url field on the package definition, to point to the git repo or the project page if it is closed source. |
Co-authored-by: paolomainardi <8747+paolomainardi@users.noreply.github.com>
Added optional Changes:
Example output: The URL field is optional - packages without URLs will show an empty column. |
|
@claude review this MR |
|
/describe |
|
/improve |
|
/review |
|
Preparing PR description... |
Generating PR code suggestionsWork in progress ... |
|
Preparing review... |
| # Check if next line has URL | ||
| url="" | ||
| if (getline > 0 && $0 ~ /^[[:space:]]+url:/) { | ||
| gsub(/^[[:space:]]+url: /, "") | ||
| gsub(/"/, "") | ||
| url=$0 |
There was a problem hiding this comment.
The unconditional getline used to check for a url line consumes the next package's - name: line when the current package has no url, causing the following package (e.g., btop after bc, openssl@3 after watch) to be skipped entirely. Avoid consuming the next record unless it is a url line (e.g., read into a temporary variable and only advance if it matches, or switch to a YAML-aware tool like yq). Example fix in awk: read the next line into temp; if it matches url parse it; otherwise store it and use NR-- with nextfile logic or restructure to process line-by-line outside the record block.
| # Check if next line has URL | |
| url="" | |
| if (getline > 0 && $0 ~ /^[[:space:]]+url:/) { | |
| gsub(/^[[:space:]]+url: /, "") | |
| gsub(/"/, "") | |
| url=$0 | |
| # Check if next line has URL, but do not consume if not present | |
| url="" | |
| nextline = "" | |
| if (getline nextline > 0) { | |
| if (nextline ~ /^[[:space:]]+url:/) { | |
| gsub(/^[[:space:]]+url: /, "", nextline) | |
| gsub(/"/, "", nextline) | |
| url=nextline | |
| } else { | |
| # Not a url line, so push back for next record | |
| NR-- | |
| } |
| # Skip if filter is set and doesn't match | ||
| if [[ -n "${filter_package}" ]] && [[ ! "${package}" =~ ${filter_package} ]]; then | ||
| continue |
There was a problem hiding this comment.
Using the unquoted user-supplied filter as a regex risks unintended matches or regex syntax errors if special characters are provided; for simple substring matching replace with a safer pattern like: if [[ -n "${filter_package}" ]] && [[ "${package}" != "${filter_package}" ]]; then continue; (quote the variable to avoid word-splitting for patterns containing spaces).
| # Main function | ||
| main() { | ||
| local filter_package="${1:-}" | ||
|
|
||
| # Collect all packages | ||
| declare -a all_rows=() | ||
|
|
||
| # Process cask packages | ||
| while IFS='|' read -r package category description url; do | ||
| [[ -z "${package}" ]] && continue | ||
|
|
||
| # Skip if filter is set and doesn't match | ||
| if [[ -n "${filter_package}" ]] && [[ ! "${package}" =~ ${filter_package} ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| all_rows+=("${package}|${category}|${description}|${url}") | ||
| done < <(get_yaml_package_entries "${PACKAGES_YML}" "cask_packages") | ||
|
|
||
| # Process homebrew packages | ||
| while IFS='|' read -r package category description url; do | ||
| [[ -z "${package}" ]] && continue | ||
|
|
||
| # Skip if filter is set and doesn't match | ||
| if [[ -n "${filter_package}" ]] && [[ ! "${package}" =~ ${filter_package} ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| all_rows+=("${package}|${category}|${description}|${url}") | ||
| done < <(get_yaml_package_entries "${PACKAGES_YML}" "homebrew_packages") | ||
|
|
There was a problem hiding this comment.
[nitpick] The two loops duplicate identical logic differing only by the key; factor into a small function (e.g., process_section "cask_packages"; process_section "homebrew_packages") to reduce duplication and ease future changes.
| # Main function | |
| main() { | |
| local filter_package="${1:-}" | |
| # Collect all packages | |
| declare -a all_rows=() | |
| # Process cask packages | |
| while IFS='|' read -r package category description url; do | |
| [[ -z "${package}" ]] && continue | |
| # Skip if filter is set and doesn't match | |
| if [[ -n "${filter_package}" ]] && [[ ! "${package}" =~ ${filter_package} ]]; then | |
| continue | |
| fi | |
| all_rows+=("${package}|${category}|${description}|${url}") | |
| done < <(get_yaml_package_entries "${PACKAGES_YML}" "cask_packages") | |
| # Process homebrew packages | |
| while IFS='|' read -r package category description url; do | |
| [[ -z "${package}" ]] && continue | |
| # Skip if filter is set and doesn't match | |
| if [[ -n "${filter_package}" ]] && [[ ! "${package}" =~ ${filter_package} ]]; then | |
| continue | |
| fi | |
| all_rows+=("${package}|${category}|${description}|${url}") | |
| done < <(get_yaml_package_entries "${PACKAGES_YML}" "homebrew_packages") | |
| # Helper function to process a package section | |
| process_section() { | |
| local section_key="$1" | |
| local filter_package="$2" | |
| local -n rows_ref="$3" | |
| while IFS='|' read -r package category description url; do | |
| [[ -z "${package}" ]] && continue | |
| # Skip if filter is set and doesn't match | |
| if [[ -n "${filter_package}" ]] && [[ ! "${package}" =~ ${filter_package} ]]; then | |
| continue | |
| fi | |
| rows_ref+=("${package}|${category}|${description}|${url}") | |
| done < <(get_yaml_package_entries "${PACKAGES_YML}" "${section_key}") | |
| } | |
| # Main function | |
| main() { | |
| local filter_package="${1:-}" | |
| # Collect all packages | |
| declare -a all_rows=() | |
| # Process both sections | |
| process_section "cask_packages" "${filter_package}" all_rows | |
| process_section "homebrew_packages" "${filter_package}" all_rows |
| url: https://cloud.google.com/sdk | ||
| - name: applite | ||
| category: System Utilities | ||
| description: ApplitE - Third-party app installer for macOS |
There was a problem hiding this comment.
Corrected spelling/capitalization of 'ApplitE' to 'Applite' to match the project name.
| description: ApplitE - Third-party app installer for macOS | |
| description: Applite - Third-party app installer for macOS |
| exit 1 | ||
| fi | ||
|
|
||
| # Verify table format (3 columns: Package, Category, Description) |
There was a problem hiding this comment.
Comment is outdated: the table includes four columns (Package, Category, Description, URL); update the comment to reflect the current format to avoid confusion.
| # Verify table format (3 columns: Package, Category, Description) | |
| # Verify table format (4 columns: Package, Category, Description, URL) |
|
@claude, review this MR and let me know what you think about this, what improvements we can make. |
|
Claude finished @paolomainardi's task in 1m 30s —— View job PR #254 Review - Package Catalog CommandTodo List:
Overall AssessmentThis is a well-executed feature that elegantly solves the problem of package discovery. The implementation is clean, follows project standards, and includes comprehensive testing. However, there are several critical bugs that need to be addressed before merging. ✅ Strengths
🐛 Critical Issues1. AWK getline Bug - CRITICAL (⭐ Highest Priority)File: Problem: The Impact: Looking at the YAML:
Evidence from YAML: - name: bc
category: System Utilities
description: bc - Arbitrary precision calculator
- name: btop # ← This gets consumed and lost when processing bc
category: System Monitoring
description: btop - Resource monitor with terminal UIFix: You need to peek at the next line without consuming it unless it's actually a URL line. Here's the corrected AWK logic: # Check if next line has URL (peek without consuming)
url=""
if ((getline nextline) > 0) {
if (nextline ~ /^[[:space:]]+url:/) {
gsub(/^[[:space:]]+url: /, "", nextline)
gsub(/"/, "", nextline)
url = nextline
} else {
# Not a URL line - we need to "put it back" for next iteration
# Save it in a variable and check at start of next iteration
saved_line = nextline
has_saved = 1
}
}A better approach might be to restructure the AWK to process line-by-line and maintain state, or use a YAML-aware tool like 2. Unsafe Regex Pattern (🔒 Security/Reliability)File: Problem: The user-supplied filter is used directly in a bash regex match without sanitization: if [[ ! "${package}" =~ ${filter_package} ]]; thenRisk:
Fix: For simple substring matching, use safer glob-style matching: if [[ -n "${filter_package}" ]] && [[ "${package}" != *"${filter_package}"* ]]; then
continue
fiOr escape the regex if you want to keep regex semantics: escaped_filter=$(printf '%s\n' "${filter_package}" | sed 's/[.[\*^$()+?{|]/\\&/g')
if [[ ! "${package}" =~ ${escaped_filter} ]]; then3. Code Duplication (📐 Maintainability)Files: Problem: The loops for Impact: Any bug fix or enhancement needs to be applied twice, increasing maintenance burden and risk of inconsistency. Fix: Extract into a helper function: process_package_section() {
local section_name="$1"
local filter="$2"
while IFS='|' read -r package category description url; do
[[ -z "${package}" ]] && continue
# Apply filter if provided
if [[ -n "${filter}" ]] && [[ "${package}" != *"${filter}"* ]]; then
continue
fi
all_rows+=("${package}|${category}|${description}|${url}")
done < <(get_yaml_package_entries "${PACKAGES_YML}" "${section_name}")
}
# Usage:
process_package_section "cask_packages" "${filter_package}"
process_package_section "homebrew_packages" "${filter_package}"📝 Minor Issues4. Typo in YAMLFile: description: ApplitE - Third-party app installer for macOSShould be: description: Applite - Third-party app installer for macOS5. Outdated Comment in TestsFile: Comment says "3 columns" but the table now has 4 columns (Package, Category, Description, URL): # Verify table format (3 columns: Package, Category, Description)Should be: # Verify table format (4 columns: Package, Category, Description, URL)🎯 Existing Automated ReviewThe copilot-pull-request-reviewer already caught all these issues in its review. The suggestions provided are valid and should be implemented. 📊 Testing RecommendationTo verify the getline bug, run: sjust packages | grep -E "(bc|btop|watch|openssl)"Expected behavior: Should show all 4 packages 💡 Suggested Action Plan
✨ Final ThoughtsThis is a valuable feature that significantly improves the user experience. The design decisions are sound, and the implementation quality is generally high. Once the bugs are fixed, this will be ready to merge. The most critical issue is the getline bug causing packages to disappear—that needs to be addressed first. Great work on the comprehensive metadata addition and the clean integration with sjust! The feature will be very useful once these issues are resolved. |
Overview
Adds a new
sjust packagescommand that allows users to easily discover and catalog all Sparkdock-managed packages directly from the CLI, organized by category with descriptions and optional URLs.Problem
Users had no way to view which packages are managed by Sparkdock without opening the codebase and searching through YAML configuration files. This made it difficult to:
Solution
Implemented a simple package catalog system:
1. Package Metadata
Extended
config/packages/all-packages.ymlto include metadata inline with each package:Each package now has a structured format:
2. Package Listing Script
Added
sjust/libs/list-packages.shthat:3. Sjust Commands
Added two command variants:
sjust packages [filter]- Short alias (matches issue suggestion)sjust sparkdock-packages [filter]- Long form (consistent with other commands)4. Ansible Integration
Updated
ansible/macos/macos/base.ymlto extract package names from the new structure usingmap(attribute='name')filter.5. Custom Instructions
Updated
.github/copilot-instructions.mdto document the requirement that all new packages must includename,category, anddescriptionfields, withurlbeing optional. This ensures future contributors follow the correct pattern when adding packages.Usage
Example Output
Categories
Packages are organized into 19 categories:
Testing
Added comprehensive test coverage in
.github/workflows/test-sjust.yml:Documentation
README.mdwith usage examples and Package Management sectionCHANGELOG.mdwith feature entry.github/copilot-instructions.mdwith package metadata requirements including the optional URL fieldFixes #253
Original prompt
Fixes #253
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
PR Type
Enhancement, Documentation
Description
Add
packagescommand to list Sparkdock-managed packages with categoriesExtend package configuration with inline metadata (category, description)
Implement filtering by package name
Add CI tests for package listing functionality
Diagram Walkthrough
File Walkthrough
2 files
Add package listing script with YAML parsing and filteringAdd sparkdock-packages and packages commands1 files
Restructure packages with category and description metadata1 files
Update package extraction to use name attribute1 files
Add CI tests for packages command functionality2 files
Document package management commands with examplesAdd changelog entries for packages command feature