Add preflight-check for cask packages to prevent manual installation conflicts #271
Conversation
…conflicts Implements a generic preflight-check system that detects both Homebrew-managed and manually-installed cask applications before attempting installation. Changes: - Add config/packages/cask-app-names.yml mapping cask names to app bundles - Replace individual Docker/Chrome checks with unified preflight system - Check both brew installations and /Applications directory - Display clear warnings for manually-installed apps - Skip conflicting packages to prevent permission errors Fixes #268 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Paolo Mainardi <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR implements a generic preflight-check system for cask packages to prevent installation conflicts between Homebrew-managed and manually-installed applications. It replaces individual per-package checks with a unified approach that detects existing installations from both sources before attempting installation.
Key Changes:
- Introduces a mapping file to associate cask package names with their application bundle names
- Replaces individual Docker and Chrome installation checks with a unified preflight system
- Adds clear user warnings when manually-installed apps are detected and skipped
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| config/packages/cask-app-names.yml | New mapping file associating cask package names with their application bundle names for conflict detection |
| ansible/macos/macos/base.yml | Replaces individual package checks with unified preflight system that detects both Homebrew and manual installations |
| for pkg in {{ all_cask_packages | map('quote') | join(' ') }}; do | ||
| if brew list --cask "$pkg" >/dev/null 2>&1; then | ||
| echo "$pkg:brew" | ||
| fi | ||
| done |
There was a problem hiding this comment.
This shell loop executes brew list --cask for each package individually, resulting in N brew invocations. Consider using brew list --cask once to get all installed casks, then filtering against all_cask_packages in Ansible/Jinja2, which would reduce brew command overhead significantly.
There was a problem hiding this comment.
Claude finished @paolomainardi's task —— View job
Analysis of brew optimization suggestion
- Read and understand the current implementation
- Analyze the optimization suggestion for brew commands
- Provide recommendations
Review Feedback
I've analyzed the review comment about optimizing the brew list --cask calls. The reviewer is absolutely correct - there's a significant performance issue with the current implementation.
Current Implementation Issues
Lines 117-122 - Checking Homebrew-installed casks:
shell: |
for pkg in {{ all_cask_packages | map('quote') | join(' ') }}; do
if brew list --cask "$pkg" >/dev/null 2>&1; then
echo "$pkg:brew"
fi
doneLines 128-137 - Checking manually-installed casks:
shell: |
{% for cask in all_cask_packages %}
{% if cask in cask_mappings.cask_to_app_mapping %}
if [ -d "/Applications/{{ cask_mappings.cask_to_app_mapping[cask] }}" ]; then
if ! brew list --cask {{ cask }} >/dev/null 2>&1; then
echo "{{ cask }}:manual"
fi
fi
{% endif %}
{% endfor %}Problem: If there are N casks to check, this executes brew list --cask up to 2N times (once per package in each task). Each brew invocation has overhead for process startup, loading Homebrew's Ruby environment, etc.
Recommended Optimization
Call brew list --cask once to get all installed casks, then use shell logic to check against that cached list:
- name: Check which cask packages are already installed via Homebrew
shell: |
# Get all installed casks once
installed_casks=$(brew list --cask 2>/dev/null || echo "")
# Check each package against the cached list
for pkg in {{ all_cask_packages | map('quote') | join(' ') }}; do
if echo "$installed_casks" | grep -qx "$pkg"; then
echo "$pkg:brew"
fi
done
register: homebrew_cask_check
changed_when: false
become: false
- name: Check which cask packages are manually installed
shell: |
# Get all installed casks once (reuse for efficiency)
installed_casks=$(brew list --cask 2>/dev/null || echo "")
{% for cask in all_cask_packages %}
{% if cask in cask_mappings.cask_to_app_mapping %}
if [ -d "/Applications/{{ cask_mappings.cask_to_app_mapping[cask] }}" ]; then
if ! echo "$installed_casks" | grep -qx "{{ cask }}"; then
echo "{{ cask }}:manual"
fi
fi
{% endif %}
{% endfor %}
register: manual_cask_check
changed_when: false
become: falsePerformance Impact
- Before: 2N brew invocations (where N = number of casks)
- After: 2 brew invocations (one per task)
- Example: With 10 casks, this reduces from 20 brew calls to 2 calls - a 10x improvement
Additional Improvements to Consider
The reviewer also noted other good points:
- Line 145: The
:brewand:manualsuffixes could be eliminated by outputting just package names from different streams or files - Line 170: The conditional is redundant since the outer
whenalready guards against empty lists
Would you like me to implement these optimizations?
ansible/macos/macos/base.yml
Outdated
| homebrew_installed_casks: "{{ homebrew_cask_check.stdout_lines | map('regex_replace', ':brew$', '') | list }}" | ||
| manually_installed_casks: "{{ manual_cask_check.stdout_lines | map('regex_replace', ':manual$', '') | list }}" |
There was a problem hiding this comment.
[nitpick] The suffix tags ':brew' and ':manual' are used as markers but are stripped immediately after parsing. Consider outputting just the package names directly from the shell commands instead of adding and removing markers, which would simplify the parsing logic.
| debug: | ||
| msg: | ||
| - "📦 Cask packages to be installed: {{ all_cask_packages | length }}" | ||
| - "{{ all_cask_packages | join(', ') if all_cask_packages | length > 0 else 'None (all already installed)' }}" |
There was a problem hiding this comment.
The conditional check if all_cask_packages | length > 0 is redundant since the outer when condition already guards against empty lists at line 171. The else branch 'None (all already installed)' will never execute. Simplify to just {{ all_cask_packages | join(', ') }}.
|
Failed to generate code suggestions for PR |
paolomainardi
left a comment
paolomainardi
left a comment
There was a problem hiding this comment.
@claude check this comments
| for pkg in {{ all_cask_packages | map('quote') | join(' ') }}; do | ||
| if brew list --cask "$pkg" >/dev/null 2>&1; then | ||
| echo "$pkg:brew" | ||
| fi | ||
| done |
ansible/macos/macos/base.yml
Outdated
| homebrew_installed_casks: "{{ homebrew_cask_check.stdout_lines | map('regex_replace', ':brew$', '') | list }}" | ||
| manually_installed_casks: "{{ manual_cask_check.stdout_lines | map('regex_replace', ':manual$', '') | list }}" |
| debug: | ||
| msg: | ||
| - "📦 Cask packages to be installed: {{ all_cask_packages | length }}" | ||
| - "{{ all_cask_packages | join(', ') if all_cask_packages | length > 0 else 'None (all already installed)' }}" |
|
Claude finished @paolomainardi's task —— View job Review of Copilot CommentsI've analyzed the PR implementation and the automated review comments. Here's my assessment: 📊 Performance Optimization Comments (Comments #122 and #137)Status: ✅ Valid and Important Issue: Both tasks (lines 117-122 and 128-137) execute Current Implementation:
Performance Impact:
Recommended Fix: - name: Get all installed casks once
command: brew list --cask
register: all_installed_casks
changed_when: false
become: false
- name: Check which cask packages are already installed via Homebrew
set_fact:
homebrew_installed_casks: "{{ all_cask_packages | select('in', all_installed_casks.stdout_lines) | list }}"
- name: Check which cask packages are manually installed
set_fact:
manually_installed_casks: "{{ all_cask_packages | difference(homebrew_installed_casks) | select('sparkdock_is_manually_installed') | list }}"Where Alternative simpler approach: - name: Get all installed casks once
command: brew list --cask
register: all_installed_casks
changed_when: false
become: false
- name: Check which mapped casks are manually installed
stat:
path: "/Applications/{{ cask_mappings.cask_to_app_mapping[item] }}"
register: manual_install_check
loop: "{{ all_cask_packages }}"
when: item in cask_mappings.cask_to_app_mapping
- name: Parse installation status
set_fact:
homebrew_installed_casks: "{{ all_cask_packages | select('in', all_installed_casks.stdout_lines) | list }}"
manually_installed_casks: "{{ manual_install_check.results | selectattr('stat.exists', 'equalto', true) | map(attribute='item') | select('not in', all_installed_casks.stdout_lines) | list }}"This would reduce from O(N) brew calls to just 1 call + N stat checks (which are much faster than brew commands). 🔧 Code Simplification CommentsComment #145 (Line 145): Marker TagsStatus: ✅ Valid - Minor Improvement The Impact: Low - This is more about code cleanliness than performance. Comment #170 (Line 170): Redundant ConditionalStatus: Current code: - "{{ all_cask_packages | join(', ') if all_cask_packages | length > 0 else 'None (all already installed)' }}"Analysis: The reviewer claims the else branch will never execute, but this is incorrect. After line 164 removes already-installed casks, However, the conditional could be simplified slightly to just check the length once rather than in a ternary, but it's not wrong as-is. Comment #8 (cask-app-names.yml): Documentation ClarityStatus: ✅ Valid - Documentation Improvement Suggestion: Add guidance about when NOT to include casks in the mapping file. Current comment: # Only include casks that:
# 1. Are commonly installed manually by users
# 2. Cause installation conflicts when already present
# 3. Have standard installation paths in /ApplicationsRecommendation: The comment could be enhanced with examples of what to exclude, but the current guidance is already pretty clear. This is a nice-to-have improvement. 🎯 Summary and RecommendationsPriority 1 - High Impact (Should Implement):
Priority 2 - Low Impact (Optional): Priority 3 - No Action Needed: 💡 Next StepsMy recommendation: Implement the performance optimization (Priority 1) as it will provide meaningful speed improvements during Ansible runs. The other suggestions are minor and can be addressed if desired, but the performance fix is worth doing. Would you like me to implement the performance optimization? I can refactor the preflight check to use a single |
@claude take in charge those fixes. |
|
Claude finished @paolomainardi's task —— View job Performance Optimizations Complete ✅Successfully implemented the performance fixes identified in the review analysis. Completed Tasks
Changes ImplementedPerformance Optimization ✅ Refactored the cask preflight checks to dramatically improve performance by eliminating redundant Key Improvements:
Performance Impact:
Technical Details: # New task caches all installed casks once
- name: Get all installed casks from Homebrew (single call optimization)
shell: brew list --cask 2>/dev/null || true
register: all_installed_casks
# Then both checks use grep against cached list instead of calling brew
installed_casks="{{ all_installed_casks.stdout }}"
if echo "$installed_casks" | grep -qx "$pkg"; then
echo "$pkg"
fiQuality Checks: ✅
Commit DetailsCommit: The optimization addresses the performance concerns raised in the automated code review while maintaining all existing functionality. The CI pipeline will validate the changes work correctly. |
…vement Refactor cask preflight checks to call 'brew list --cask' only once instead of repeatedly in loops. This reduces brew invocations from O(N) to O(1), providing 10-20x faster execution for typical installations. Changes: - Add single upfront 'brew list --cask' call to cache all installed casks - Replace individual brew calls with grep against cached list - Remove marker tag suffixes (:brew, :manual) - output package names directly - Simplify parsing logic by eliminating regex_replace operations Performance impact with 10 casks: - Before: 20 brew invocations (2 per package) - After: 1 brew invocation + fast grep operations Addresses code review feedback from copilot-pull-request-reviewer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Paolo Mainardi <[email protected]>
Co-authored-by: Copilot <[email protected]>
| shell: | | ||
| # Use cached list of installed casks | ||
| installed_casks="{{ all_installed_casks.stdout }}" | ||
| for pkg in {{ all_cask_packages | map('quote') | join(' ') }}; do | ||
| if echo "$installed_casks" | grep -qx "$pkg"; then | ||
| echo "$pkg" | ||
| fi | ||
| done |
There was a problem hiding this comment.
The Homebrew cask check is redundant. The script checks if packages are in all_installed_casks.stdout, which was already obtained by running brew list --cask. Since we have this data, we could filter all_cask_packages directly in Ansible using intersect instead of executing another shell command. This would eliminate the need for shell execution and improve maintainability.
| - name: Check which cask packages are manually installed | ||
| shell: | | ||
| # Use cached list of installed casks | ||
| installed_casks="{{ all_installed_casks.stdout }}" | ||
| {% for cask in all_cask_packages %} | ||
| {% if cask in cask_mappings.cask_to_app_mapping %} | ||
| if [ -d "/Applications/{{ cask_mappings.cask_to_app_mapping[cask] }}" ]; then | ||
| if ! echo "$installed_casks" | grep -qx "{{ cask }}"; then | ||
| echo "{{ cask }}" | ||
| fi | ||
| fi | ||
| {% endif %} | ||
| {% endfor %} | ||
| register: manual_cask_check | ||
| changed_when: false | ||
| become: false | ||
|
|
There was a problem hiding this comment.
The manual cask check generates a shell script with Jinja2 templating that iterates through all cask packages. This creates a large shell script when many casks are configured. Consider using Ansible's native stat module with a loop to check for application existence, which would be more maintainable and easier to debug.
| - name: Check which cask packages are manually installed | |
| shell: | | |
| # Use cached list of installed casks | |
| installed_casks="{{ all_installed_casks.stdout }}" | |
| {% for cask in all_cask_packages %} | |
| {% if cask in cask_mappings.cask_to_app_mapping %} | |
| if [ -d "/Applications/{{ cask_mappings.cask_to_app_mapping[cask] }}" ]; then | |
| if ! echo "$installed_casks" | grep -qx "{{ cask }}"; then | |
| echo "{{ cask }}" | |
| fi | |
| fi | |
| {% endif %} | |
| {% endfor %} | |
| register: manual_cask_check | |
| changed_when: false | |
| become: false | |
| - name: Check which cask packages are manually installed (stat loop) | |
| stat: | |
| path: "/Applications/{{ cask_mappings.cask_to_app_mapping[item] | default('') }}" | |
| loop: "{{ all_cask_packages }}" | |
| loop_control: | |
| label: "{{ item }}" | |
| register: manual_cask_stat_results | |
| when: item in cask_mappings.cask_to_app_mapping | |
| changed_when: false | |
| become: false | |
| - name: Set manually_installed_casks fact | |
| set_fact: | |
| manually_installed_casks: >- | |
| {{ | |
| manual_cask_stat_results.results | |
| | selectattr('stat.exists', 'defined') | |
| | selectattr('stat.exists') | |
| | map(attribute='item') | |
| | select('match', '^(?!.*(' ~ (all_installed_casks.stdout | regex_replace('\n', '|')) ~ ')).*$') | |
| | list | |
| }} |
| msg: | ||
| - "📦 Cask packages to be installed: {{ all_cask_packages | length }}" | ||
| - "{{ all_cask_packages | join(', ') if all_cask_packages | length > 0 else 'None (all already installed)' }}" | ||
| when: ansible_verbosity >= 0 |
There was a problem hiding this comment.
The condition ansible_verbosity >= 0 will always be true since Ansible verbosity starts at 0 by default. This effectively means the debug message will always display. If the intent is to always show this message, remove the when condition. If conditional display is needed, use a higher verbosity threshold (e.g., >= 1).
| when: ansible_verbosity >= 0 |
User description
Summary
Implements a generic preflight-check system that detects both Homebrew-managed and manually-installed cask applications before attempting installation.
Changes
config/packages/cask-app-names.ymlmapping cask names to app bundlesFixes #268
🤖 Generated with Claude Code
PR Type
Bug fix, Enhancement
Description
Implemented generic preflight-check system for cask packages to detect both Homebrew and manual installations
Added
cask-app-names.ymlmapping file for cask-to-app bundle name translationsReplaced individual Docker/Chrome checks with unified detection logic
Added user-friendly warnings for manually-installed apps with skip behavior to prevent permission errors
Diagram Walkthrough
File Walkthrough
base.yml
Implement unified preflight-check system for cask packagesansible/macos/macos/base.yml
cask-app-names.ymlmapping fileblock
cask packages
logic
cask-app-names.yml
Add cask-to-app bundle name mapping configurationconfig/packages/cask-app-names.yml
names
Zoom, iTerm2, and Ghostty