Skip to content

Add preflight-check for cask packages to prevent manual installation conflicts #271

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.

Sign up for GitHub

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

Open
paolomainardi wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add preflight-check for cask packages to prevent manual installation conflicts #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc476ad
Add preflight-check for cask packages to prevent manual installation …
github-actions[bot] Oct 24, 2025
a64e6a2
Optimize preflight-check brew calls for significant performance impro…
github-actions[bot] Oct 24, 2025
ca1ee14
Apply suggestion from @Copilot
paolomainardi Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 70 additions & 18 deletions ansible/macos/macos/base.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@
name: all_packages
tags: always

- name: Load cask to app name mappings
include_vars:
file: "{{ dev_env_dir }}/config/packages/cask-app-names.yml"
name: cask_mappings
tags: always

- name: Collect all package lists
tags: brew_packages
set_fact:
Expand Down Expand Up @@ -104,31 +110,77 @@
when: all_removed_homebrew | length > 0
become: false

- name: Docker and container runtime installation
tags: [docker]
- name: Preflight check for cask packages
tags: cask
block:
- name: Check if Docker Desktop is already installed
shell: brew list --cask docker-desktop >/dev/null 2>&1 && echo yes || echo no
register: docker_desktop_installed
- name: Get all installed casks from Homebrew (single call optimization)
shell: brew list --cask 2>/dev/null || true
register: all_installed_casks
changed_when: false
become: false

- name: Remove Docker Desktop from install list if already installed
set_fact:
all_cask_packages: "{{ all_cask_packages | difference(['docker-desktop']) }}"
when: "'docker-desktop' in all_cask_packages and docker_desktop_installed.stdout == 'yes'"
- name: Check which cask packages are already installed via Homebrew
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
Comment on lines 127 to 131
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@paolomainardi paolomainardi Oct 24, 2025

Choose a reason for hiding this comment

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

@claude

Copy link

@claude claude bot Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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
  done

Lines 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: false

Performance 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:

  1. Line 145: The :brew and :manual suffixes could be eliminated by outputting just package names from different streams or files
  2. Line 170: The conditional is redundant since the outer when already guards against empty lists

Would you like me to implement these optimizations?

Comment on lines +123 to +131
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
register: homebrew_cask_check
changed_when: false
become: false

- name: Google Chrome installation
tags: [chrome]
block:
- name: Check if Google Chrome is already installed
shell: brew list --cask google-chrome >/dev/null 2>&1 && echo yes || echo no
register: google_chrome_installed
- 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

Comment on lines +136 to +153
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
- 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
}}

Copilot uses AI. Check for mistakes.
- name: Parse installation status
set_fact:
homebrew_installed_casks: "{{ homebrew_cask_check.stdout_lines }}"
manually_installed_casks: "{{ manual_cask_check.stdout_lines }}"

- name: Display warning for manually installed applications
debug:
msg:
- "⚠️ WARNING: The following applications are installed manually (not via Homebrew):"
- "{{ manually_installed_casks | join(', ') }}"
- ""
- "These will be SKIPPED during installation to avoid conflicts."
- ""
- "📋 Recommendation: For better package management, consider:"
- " 1. Uninstalling the manually installed applications"
- " 2. Re-running sparkdock to install them via Homebrew"
- ""
- "This ensures all applications are managed consistently through Homebrew."
when: manually_installed_casks | length > 0

- name: Remove Google Chrome from install list if already installed
- name: Remove already-installed casks from installation list
set_fact:
all_cask_packages: "{{ all_cask_packages | difference(['google-chrome']) }}"
when: "'google-chrome' in all_cask_packages and google_chrome_installed.stdout == 'yes'"
all_cask_packages: "{{ all_cask_packages | difference(homebrew_installed_casks) | difference(manually_installed_casks) }}"

- name: Display casks to be installed
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)' }}"
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

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(', ') }}.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@paolomainardi paolomainardi Oct 24, 2025

Choose a reason for hiding this comment

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

ok

when: ansible_verbosity >= 0
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

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).

Suggested change
when: ansible_verbosity >= 0

Copilot uses AI. Check for mistakes.

- name: Install cask packages
tags: cask
Expand Down
20 changes: 20 additions & 0 deletions config/packages/cask-app-names.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Mapping of Homebrew cask package names to their Application bundle names
# This file is used by the preflight-check to detect manually installed applications
# Format: cask_package_name: "Application Name.app"
#
# Only include casks that:
# 1. Are commonly installed manually by users
# 2. Cause installation conflicts when already present
# 3. Have standard installation paths in /Applications
#
# Do NOT include casks that are rarely installed manually or have no known conflict issues.
# Example: Do not include utility casks that are typically installed as dependencies and do not conflict with manual installations.

cask_to_app_mapping:
google-chrome: "Google Chrome.app"
docker-desktop: "Docker.app"
visual-studio-code: "Visual Studio Code.app"
slack: "Slack.app"
zoom: "zoom.us.app"
iterm2: "iTerm.app"
ghostty: "Ghostty.app"
Loading