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

Closed
paolomainardi wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
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
76 changes: 58 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,65 @@
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: Check which cask packages are already installed via Homebrew
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
Comment on lines +127 to +131

Copilot AI Oct 24, 2025

Copy link

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.

@paolomainardi paolomainardi Oct 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@claude

@claude claude Bot Oct 24, 2025 •
edited
Loading

Copy link
Copy Markdown

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

Copilot AI Oct 24, 2025

Copy link

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: Remove Docker Desktop from install list if already installed
- name: Check which cask packages are manually installed
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 %}
Comment thread
paolomainardi marked this conversation as resolved.
register: manual_cask_check
changed_when: false
become: false

Comment on lines +136 to +153

Copilot AI Oct 24, 2025

Copy link

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:
all_cask_packages: "{{ all_cask_packages | difference(['docker-desktop']) }}"
when: "'docker-desktop' in all_cask_packages and docker_desktop_installed.stdout == 'yes'"
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 }}"

Copilot AI Oct 24, 2025

Copy link

Choose a reason for hiding this comment

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

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

Copilot uses AI. Check for mistakes.

@paolomainardi paolomainardi Oct 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok


- 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
changed_when: false
- 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)' }}"

Copilot AI Oct 24, 2025

Copy link

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.

@paolomainardi paolomainardi Oct 24, 2025

Copy link
Copy Markdown
Member Author

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

Copilot AI Oct 24, 2025

Copy link

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
17 changes: 17 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,17 @@
# 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
Comment thread
paolomainardi marked this conversation as resolved.

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