Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds keychain support to the SSH role for better SSH key management across different operating systems. Keychain is a front-end to ssh-agent that helps manage SSH keys persistently across shell sessions.
- Installs keychain package on Debian-based Linux systems
- Configures shell integration for keychain on Linux and native macOS keychain support
- Updates SSH client configuration to automatically add keys to agent and use keychain features
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| roles/ssh/tasks/main.yml | Adds keychain installation and shell configuration tasks, updates SSH config block |
| roles/ssh/tasks/install_Debian.yml | Implements keychain package installation for Debian/Ubuntu systems |
| roles/ssh/tasks/configure_shell.yml | Configures shell RC files for keychain integration on Linux and macOS |
| playbook-devel.yml | Adds SSH role to the development playbook |
Comments suppressed due to low confidence (1)
roles/ssh/tasks/main.yml:5
- The task name mentions 'Linux only' but the when condition only checks for Debian family. Consider renaming to 'Install keychain (Debian/Ubuntu)' to be more accurate.
- name: Install keychain (Linux only)
|
|
||
| - name: Install keychain (Linux only) | ||
| ansible.builtin.include_tasks: "install_{{ ansible_os_family }}.yml" | ||
| when: ansible_os_family == 'Debian' |
There was a problem hiding this comment.
The condition only supports Debian family systems, but other Linux distributions like RedHat, SUSE, or Arch might also benefit from keychain. Consider expanding support or making the limitation clearer in documentation.
| when: ansible_os_family == 'Debian' | |
| when: ansible_os_family in ['Debian', 'RedHat', 'Suse', 'Arch'] |
|
|
||
| - name: Get shell configuration | ||
| ansible.builtin.set_fact: | ||
| shell_info: "{{ ansible_user_shell | shell_config }}" |
There was a problem hiding this comment.
The 'shell_config' filter is used but may not be a standard Ansible filter. This could cause the task to fail if the filter is not defined or available.
| shell_info: "{{ ansible_user_shell | shell_config }}" | |
| shell_info: > | |
| {{ | |
| { | |
| '/bin/bash': {'rc_file': '.bashrc'}, | |
| '/bin/zsh': {'rc_file': '.zshrc'} | |
| }[ansible_user_shell] | default({'rc_file': '.profile'}) | |
| }} |
|
|
||
| - name: Setup keychain for SSH keys (Linux) | ||
| ansible.builtin.lineinfile: | ||
| path: "{{ ansible_user_dir }}/{{ shell_info.rc_file }}" |
There was a problem hiding this comment.
The variable 'ansible_user_dir' is used but this is not a standard Ansible fact. Consider using 'ansible_env.HOME' instead, which is the standard way to reference the user's home directory.
| path: "{{ ansible_user_dir }}/{{ shell_info.rc_file }}" | |
| path: "{{ ansible_env.HOME }}/{{ shell_info.rc_file }}" |
|
|
||
| - name: Setup SSH agent for keychain (macOS) | ||
| ansible.builtin.lineinfile: | ||
| path: "{{ ansible_user_dir }}/{{ shell_info.rc_file }}" |
There was a problem hiding this comment.
Same issue as above - 'ansible_user_dir' should likely be 'ansible_env.HOME' to reference the user's home directory correctly.
| path: "{{ ansible_user_dir }}/{{ shell_info.rc_file }}" | |
| path: "{{ ansible_env.HOME }}/{{ shell_info.rc_file }}" |
| - name: Setup keychain for SSH keys (Linux) | ||
| ansible.builtin.lineinfile: | ||
| path: "{{ ansible_user_dir }}/{{ shell_info.rc_file }}" | ||
| regexp: "^eval `keychain --eval --agents ssh" |
There was a problem hiding this comment.
The regular expression is incomplete and missing the closing backtick, which could lead to unexpected matching behavior. Consider using a more complete pattern or escaping special characters properly.
| regexp: "^eval `keychain --eval --agents ssh" | |
| regexp: "^eval \\`keychain --eval --agents ssh\\`" |
1 participant
No description provided.