Simplify yarn linking with clean conflict resolution

[Copilot]

Fixes yarn linking issues that occur when yarn is already installed from other sources (like npm) by implementing a simple, clean solution that properly checks package installation status.

Problem

The original yarn linking task failed when there were conflicts with existing yarn installations, causing Ansible provisioning to fail. Additionally, the logic was checking against the linked packages list rather than verifying if yarn was actually installed via homebrew.

Solution

Replaced complex bash scripting with a simple, clean approach:

• Uses brew link --overwrite yarn to handle conflicts directly
• Checks if yarn is in the homebrew packages list before attempting to link
• Removed yarn from the linked packages configuration since it's handled separately
• Simplified the generic package linking task
• Removed 40+ lines of complex bash functions in favor of clean, logical conditions

Benefits

• Simpler: No complex bash functions or conditional logic
• Cleaner: Easy to read and understand
• Robust: Handles yarn linking conflicts effectively and only when appropriate
• Logical: Only attempts yarn linking when yarn is actually installed via homebrew
• Maintainable: Follows standard Ansible patterns

The solution maintains the same functionality while being much more straightforward and following Ansible best practices.

---

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

---

PR Type

Bug fix, Enhancement

---

Description

• Fix Ansible templating syntax for homebrew_prefix variable

• Add specialized yarn linking handler with conflict resolution

• Implement robust error handling for orphaned symlinks

• Separate yarn linking from other package linking logic

---

Changes walkthrough 📝

Relevant files

Bug fix

base.yml Enhanced yarn linking with conflict resolution                      ansible/macos/macos/base.yml Replace simple package linking with specialized yarn conflict resolution handler Fix homebrew_prefix variable syntax from shell ${} to Ansible {{ }} templating Add functions to detect yarn installation status and symlink validity Implement --overwrite flag for handling linking conflicts Separate yarn linking from other packages to prevent conflicts +60/-3

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[paolomainardi]

/review

[paolomainardi]

/improve

[Copilot]

Pull Request Overview

This PR fixes yarn linking issues in the Ansible provisioning script by implementing robust conflict resolution. The main problem was that the original script used shell variable syntax instead of Ansible templating syntax for the homebrew_prefix variable, causing "unbound variable" errors.

Key changes:

• Implemented specialized yarn linking handler with conflict resolution logic
• Fixed variable reference syntax from ${homebrew_prefix} to {{ homebrew_prefix }}
• Split package linking into yarn-specific and general package handling

[sparkfabrik-ai-bot]

PR Code Suggestions ✨

Latest suggestions up to 37369c5
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add error handling for readlink The function should handle cases where readlink fails or returns an empty string. Add error handling to prevent the script from failing when the symlink is broken or invalid. ansible/macos/macos/base.yml [140-149] # Function to check if yarn symlink points to brew installation is_yarn_properly_linked() { if [ -L "{{ homebrew_prefix }}/bin/yarn" ]; then - local target=$(readlink "{{ homebrew_prefix }}/bin/yarn") + local target=$(readlink "{{ homebrew_prefix }}/bin/yarn" 2>/dev/null || echo "") # Check if target contains yarn cellar path - echo "$target" | grep -q "Cellar/yarn/" + [ -n "$target" ] && echo "$target" | grep -q "Cellar/yarn/" else false fi } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential issue where readlink could fail on broken symlinks. The improved error handling with 2>/dev/null and checking for non-empty target is a good defensive programming practice.	Low
	Verify symlink removal success The function should verify the removal was successful and handle potential permission issues. Add verification after the removal operation to ensure the symlink was actually deleted. ansible/macos/macos/base.yml [151-157] # Function to remove orphaned yarn symlink remove_orphaned_yarn_symlink() { if [ -L "{{ homebrew_prefix }}/bin/yarn" ] || [ -f "{{ homebrew_prefix }}/bin/yarn" ]; then echo "Removing orphaned yarn symlink..." rm -f "{{ homebrew_prefix }}/bin/yarn" + if [ -e "{{ homebrew_prefix }}/bin/yarn" ]; then + echo "Warning: Failed to remove yarn symlink" + return 1 + fi fi } Suggestion importance[1-10]: 5 __ Why: The suggestion adds verification after removal which is useful, but the impact is moderate since rm -f typically doesn't fail on permission issues and the script uses set -euo pipefail which would catch most errors anyway.	Low
	Handle symlink removal failures gracefully The script should handle the case where remove_orphaned_yarn_symlink fails and continue gracefully. Add error handling to prevent the entire task from failing if symlink removal encounters issues. ansible/macos/macos/base.yml [169-176] else echo "Yarn is not installed via brew" # If yarn symlink exists but package is not installed, remove it if [ -L "{{ homebrew_prefix }}/bin/yarn" ] || [ -f "{{ homebrew_prefix }}/bin/yarn" ]; then echo "Found orphaned yarn symlink, removing it..." - remove_orphaned_yarn_symlink + if ! remove_orphaned_yarn_symlink; then + echo "Warning: Could not remove orphaned yarn symlink, continuing..." + fi fi fi Suggestion importance[1-10]: 4 __ Why: The suggestion adds graceful error handling for the remove_orphaned_yarn_symlink function call, but this depends on suggestion 2 being implemented first. The improvement is minor since the script already has error handling via set -euo pipefail.	Low

---

Previous suggestions

Suggestions up to commit 37369c5

Category	Suggestion	Impact
Possible issue	Handle grep failure gracefully The function uses grep -q which can fail and cause the script to exit due to set -euo pipefail. Add explicit return codes to handle grep failures gracefully and prevent script termination. ansible/macos/macos/base.yml [140-149] # Function to check if yarn symlink points to brew installation is_yarn_properly_linked() { if [ -L "{{ homebrew_prefix }}/bin/yarn" ]; then local target=$(readlink "{{ homebrew_prefix }}/bin/yarn") # Check if target contains yarn cellar path - echo "$target" | grep -q "Cellar/yarn/" + if echo "$target" | grep -q "Cellar/yarn/"; then + return 0 + else + return 1 + fi else - false + return 1 fi } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that grep -q can fail with set -euo pipefail and provides a proper solution with explicit return codes. This prevents potential script termination and improves robustness.	Medium
General	Add removal error handling The function checks for both symlink and regular file but doesn't verify removal success. Add error handling to ensure the removal operation completes successfully before proceeding. ansible/macos/macos/base.yml [151-157] # Function to remove orphaned yarn symlink remove_orphaned_yarn_symlink() { if [ -L "{{ homebrew_prefix }}/bin/yarn" ] || [ -f "{{ homebrew_prefix }}/bin/yarn" ]; then echo "Removing orphaned yarn symlink..." - rm -f "{{ homebrew_prefix }}/bin/yarn" + if ! rm -f "{{ homebrew_prefix }}/bin/yarn"; then + echo "Failed to remove yarn symlink" >&2 + return 1 + fi + echo "Yarn symlink removed successfully" fi } Suggestion importance[1-10]: 6 __ Why: The suggestion adds error handling for the rm command which is good practice, but the impact is moderate since rm -f typically doesn't fail and the function is used in non-critical contexts.	Low
	Check function return status The function call to remove_orphaned_yarn_symlink doesn't check its return status, which could mask removal failures. Add error handling to ensure the operation succeeds. ansible/macos/macos/base.yml [169-176] else echo "Yarn is not installed via brew" # If yarn symlink exists but package is not installed, remove it if [ -L "{{ homebrew_prefix }}/bin/yarn" ] || [ -f "{{ homebrew_prefix }}/bin/yarn" ]; then echo "Found orphaned yarn symlink, removing it..." - remove_orphaned_yarn_symlink + if ! remove_orphaned_yarn_symlink; then + echo "Failed to remove orphaned yarn symlink" >&2 + exit 1 + fi fi fi Suggestion importance[1-10]: 5 __ Why: The suggestion adds error checking for the function call, but this depends on suggestion 2 being implemented first. The improvement is minor since the original code would continue execution even if removal fails.	Low

[paolomainardi]

/describe

[sparkfabrik-ai-bot]

PR Reviewer Guide 🔍

(Review updated until commit 37369c5)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Gap The yarn linking logic checks if yarn is installed via brew but doesn't handle the case where yarn might be installed through other means (npm, direct install). This could lead to unexpected behavior when yarn exists but wasn't installed via brew. if is_yarn_installed; then echo "Yarn is installed via brew" if is_yarn_properly_linked; then echo "Yarn is already properly linked" else echo "Yarn is not properly linked, attempting to fix..." # Use --overwrite to handle conflicts brew link --overwrite yarn echo "Yarn linked successfully with --overwrite" fi else echo "Yarn is not installed via brew" # If yarn symlink exists but package is not installed, remove it if [ -L "{{ homebrew_prefix }}/bin/yarn" ] || [ -f "{{ homebrew_prefix }}/bin/yarn" ]; then echo "Found orphaned yarn symlink, removing it..." remove_orphaned_yarn_symlink fi fi Error Handling The script uses 'set -euo pipefail' but doesn't have proper error handling for the 'brew link --overwrite yarn' command. If this command fails, the entire task will fail without clear indication of what went wrong. brew link --overwrite yarn echo "Yarn linked successfully with --overwrite" Race Condition There's a potential race condition between checking if yarn is properly linked and the actual linking operation. The symlink state could change between the check and the link operation. if is_yarn_properly_linked; then echo "Yarn is already properly linked" else echo "Yarn is not properly linked, attempting to fix..." # Use --overwrite to handle conflicts brew link --overwrite yarn echo "Yarn linked successfully with --overwrite" fi

[sparkfabrik-ai-bot]

Persistent review updated to latest commit 37369c5

[sparkfabrik-ai-bot]

PR Description updated to latest commit (37369c5)

[paolomainardi]

@copilot make this code way simple than this, we do not need bash functions, just make it plain and clean.

[Copilot]

Simplified the yarn linking code as requested. Removed all bash functions and replaced with plain, clean commands:

• Simple brew link --overwrite yarn for yarn linking
• Separate task for other packages using standard homebrew module
• Reduced from 46 lines of complex bash to 4 lines of simple commands

The code is now much cleaner and easier to understand. (commit: ad86de3)