Skip to content

feat: add custom instructions, refactor sway and mkinitcpio#151

Open
paolomainardi wants to merge 7 commits intomainfrom
feature/github-instructions
Open

feat: add custom instructions, refactor sway and mkinitcpio#151
paolomainardi wants to merge 7 commits intomainfrom
feature/github-instructions

Conversation

@paolomainardi
Copy link
Member

@paolomainardi paolomainardi commented Oct 12, 2025

PR Type

Documentation, Enhancement


Description

  • Add comprehensive GitHub Copilot instructions for project

  • Refactor Sway window manager package organization

  • Update mkinitcpio configuration to use modular hooks

  • Improve system kernel installation formatting


Changes walkthrough 📝

Relevant files
Documentation
copilot-instructions.md
Add comprehensive GitHub Copilot project documentation     

.github/copilot-instructions.md

  • Add comprehensive project documentation for GitHub Copilot
  • Document two-phase installation architecture and workflow
  • Provide configuration patterns and development guidelines
  • Include critical integration points and conventions
  • +98/-0   
    Enhancement
    main.yml
    Refactor Sway package organization and add ghostty             

    playbooks/roles/sway/tasks/main.yml

  • Reorganize package installation into logical groups
  • Add ghostty terminal emulator to system utilities
  • Separate core Sway packages from system utilities
  • Fix formatting and indentation consistency
  • +8/-4     
    kernel.yml
    Refactor mkinitcpio to use modular hooks configuration     

    playbooks/roles/system/tasks/kernel.yml

  • Update mkinitcpio configuration to use modular hooks approach
  • Change from single config file to hooks-specific configuration
  • Fix minor formatting inconsistencies in package names
  • +4/-4     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @sparkfabrik-ai-bot
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Indentation Issue

    The "System utilities" task block appears to be incorrectly indented, making it a separate task instead of part of the "Install sway" block. This could cause the task to run regardless of the sway enable condition.

    - name: System utilities
      community.general.pacman:
        name:
          - alacritty
          - ghostty
          - thunar
          - wofi
          - wl-clipboard
          - ranger
    Missing Context

    The mkinitcpio configuration change switches from copying a complete config file to copying hooks configuration, but the corresponding source file change is not visible in the diff. This could break the kernel configuration if the new file doesn't exist or has incorrect content.

    - name: Copy provided default mkinitcpio default hooks.
      tags: [system, mkinitcpio]
      vars:
        enable_encrypt: "{{ (encryption == true) | ternary('encrypt', '') }}"
      ansible.builtin.copy:
        src: files/sf_default_hooks.conf
        dest: /etc/mkinitcpio.conf.d/sf_default_hooks.conf

    @sparkfabrik-ai-bot
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove potentially unavailable package

    The ghostty package may not be available in official Arch repositories and could
    cause installation failures. Verify package availability or move to AUR installation
    section if it's an AUR package.

    playbooks/roles/sway/tasks/main.yml [21-29]

     - name: System utilities
       community.general.pacman:
         name:
           - alacritty
    -      - ghostty
           - thunar
           - wofi
           - wl-clipboard
           - ranger
    Suggestion importance[1-10]: 7

    __

    Why: Valid concern about ghostty package availability in official Arch repositories. This could cause installation failures if the package doesn't exist, making it a practical issue worth addressing.

    Medium
    Ensure target directory exists

    The destination directory /etc/mkinitcpio.conf.d/ may not exist by default. Add a
    task to ensure the directory exists before copying the configuration file to prevent
    potential failures.

    playbooks/roles/system/tasks/kernel.yml [93-94]

    -ansible.builtin.copy:
    -  src: files/sf_default_hooks.conf
    -  dest: /etc/mkinitcpio.conf.d/sf_default_hooks.conf
    +- name: Ensure mkinitcpio.conf.d directory exists
    +  ansible.builtin.file:
    +    path: /etc/mkinitcpio.conf.d
    +    state: directory
    +    mode: '0755'
     
    +- name: Copy provided default mkinitcpio default hooks.
    +  tags: [system, mkinitcpio]
    +  ansible.builtin.copy:
    +    src: files/sf_default_hooks.conf
    +    dest: /etc/mkinitcpio.conf.d/sf_default_hooks.conf
    +
    Suggestion importance[1-10]: 6

    __

    Why: Good defensive programming practice. The /etc/mkinitcpio.conf.d/ directory may not exist by default, and ensuring it exists before copying files prevents potential runtime failures.

    Low
    General
    Remove unused variable definition

    The enable_encrypt variable is defined but never used in this task. Either utilize
    this variable in the configuration file or remove the unused variable definition to
    avoid confusion.

    playbooks/roles/system/tasks/kernel.yml [88-94]

     - name: Copy provided default mkinitcpio default hooks.
       tags: [system, mkinitcpio]
    -  vars:
    -    enable_encrypt: "{{ (encryption == true) | ternary('encrypt', '') }}"
       ansible.builtin.copy:
         src: files/sf_default_hooks.conf
         dest: /etc/mkinitcpio.conf.d/sf_default_hooks.conf
    Suggestion importance[1-10]: 4

    __

    Why: The enable_encrypt variable is indeed defined but not used in the copy task. While this doesn't break functionality, removing unused variables improves code clarity and maintainability.

    Low

    @paolomainardi paolomainardi requested a review from Copilot November 2, 2025 15:15
    @paolomainardi
    Copy link
    Member Author

    /improve

    Copy link

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR refactors package management and system configuration across several Ansible roles. The main changes focus on consolidating package installations, updating mkinitcpio configuration to use drop-in directory structure, and adding new copilot instructions documentation.

    • Migrated mkinitcpio configuration from a single file to a drop-in configuration file in /etc/mkinitcpio.conf.d/
    • Reorganized package installations by moving packages between roles (e.g., thefuck from productivity to base)
    • Added comprehensive Copilot instructions documenting the project's architecture and workflows

    Reviewed Changes

    Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

    Show a summary per file
    File Description
    playbooks/roles/system/tasks/kernel.yml Updates mkinitcpio configuration to use drop-in directory and fixes trailing whitespace
    playbooks/roles/system/files/sf_default_hooks.conf Adds new drop-in configuration file for mkinitcpio hooks
    playbooks/roles/sway/tasks/main.yml Reorganizes packages, adds ghostty terminal and woff2-font-awesome
    playbooks/roles/packages/tasks/productivity.yml Removes thefuck package (moved to base packages)
    playbooks/roles/packages/tasks/multimedia.yml Adds explicit removal of calf package and removes it from audio plugins
    playbooks/roles/packages/tasks/base.yml Adds multiple CLI tools and moves thefuck package from productivity role
    .github/copilot-instructions.md Adds comprehensive project documentation for Copilot

    💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    ignore_errors: yes

    - name: Copy provided mkinitcpio.conf
    - name: Copy provided default mkinitcpio default hooks.
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The word 'default' is duplicated in the task name. Should be 'Copy provided mkinitcpio default hooks.' instead of 'Copy provided default mkinitcpio default hooks.'

    Suggested change
    - name: Copy provided default mkinitcpio default hooks.
    - name: Copy provided mkinitcpio default hooks.

    Copilot uses AI. Check for mistakes.
    name:
    - ansible
    - bashtop
    - bc
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The package 'bc' is duplicated in the package list (appears on both line 27 and line 46).

    Copilot uses AI. Check for mistakes.
    - bat
    - bc
    - btop
    - thefuck
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The package 'thefuck' is duplicated in the package list (appears on both line 48 and line 53).

    Copilot uses AI. Check for mistakes.
    - bash-completion
    - dialog
    - dmidecode
    - glances
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The package 'glances' is being installed on line 36 but is also added to the removal list on line 8. This creates a conflict where the package is both marked for removal and installation.

    Copilot uses AI. Check for mistakes.
    paolomainardi and others added 2 commits November 2, 2025 16:17
    Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
    Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
    @paolomainardi paolomainardi requested a review from Copilot November 2, 2025 15:18
    Copy link

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.


    💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    - lm_sensors
    - coreutils
    - usbutils
    - dmidecode
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The package 'dmidecode' appears twice in the system packages list (lines 31 and 35). Remove one of the duplicate entries.

    Copilot uses AI. Check for mistakes.
    - bc
    - lm_sensors
    - coreutils
    - usbutils
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The package 'usbutils' appears twice in the system packages list (lines 30 and 54). Remove one of the duplicate entries.

    Copilot uses AI. Check for mistakes.
    - bash-completion
    - dialog
    - dmidecode
    - glances
    Copy link

    Copilot AI Nov 2, 2025

    Choose a reason for hiding this comment

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

    The package 'glances' is marked for removal at line 8 but is also included in the installation list at line 36. This creates conflicting intent. Either remove it from the installation list or remove the task that ensures it's absent.

    Suggested change
    - glances
    # - glances

    Copilot uses AI. Check for mistakes.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant