Refactor Debian Ansible role, add option to enable tool groups - #262
Conversation
WalkthroughNamespace many Ansible variables to debian_base_/debian_tools_/mac_base_*; replace role debian_developer with debian_tools; move and expand apt/Homebrew package lists; update playbooks, inventory, roles, and local docs to use new names and add aggregated Homebrew install groups for debian_tools. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(30,144,255,0.5)
Ansible Controller->>Inventory: read group_vars (debian/mac) & debian_tools_brew_install_groups
end
rect rgba(34,139,34,0.5)
Ansible Controller->>Hosts: run playbooks (plays -> debian_tools, debian_base, debian_samba_client)
end
rect rgba(255,140,0,0.5)
Hosts->>Apt: install packages from debian_base_apt_* / debian_tools_apt_packages
Hosts->>Homebrew: aggregate enabled debian_tools_brew_packages_* → install bottles & casks if non-empty
end
rect rgba(199,21,133,0.5)
Hosts->>LocalConfig: configure admin user, SSH, crowdsec, go-task using debian_base_* vars
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ansible/inventory/group_vars/debian/vars.yaml`:
- Around line 44-55: The debian_tools_apt_packages entry for "yq" is misleading:
the apt package is the Python kislyuk/yq, not mikefarah/yq; either update the
comment next to the "yq" list item to reference the correct kislyuk/yq project,
or remove the apt "yq" entry and instead add guidance to install mikefarah/yq
via Homebrew/snap/direct binary (or add a separate non-apt install step) if the
Go mikefarah/yq is required; locate the "debian_tools_apt_packages" list and
update the "yq" line accordingly.
In `@ansible/roles/debian_base/tasks/70-user.yaml`:
- Around line 11-15: The task "Set password for admin user '{{
debian_base_admin_user }}'" uses the undefined variable
debian_base_password_salt causing runtime failure; define
debian_base_password_salt alongside debian_base_admin_password in your
inventory/group vars (e.g., add debian_base_password_salt: "<your-salt>" in
ansible/inventory/group_vars/debian/vars.yaml or an encrypted vault) so the
password_hash filter has a salt, or alternatively update the task to use a
default salt via debian_base_password_salt|default('your-default-salt') if you
prefer an inline fallback.
🧹 Nitpick comments (5)
ansible/inventory/group_vars/mac/vars.yaml (1)
29-33: Consider migrating fromtfsectotrivy.The tfsec project has been consolidated into Trivy as part of Aqua Security's portfolio. While tfsec remains functional, trivy provides continued support and is the focus of ongoing development.
ansible/roles/debian_tools/tasks/20-homebrew.yaml (1)
23-31: Consider usingstate: presentfor idempotency.Using
state: latestwill upgrade packages on every run if newer versions are available upstream, which may cause unintended changes and slower playbook execution. Considerstate: presentif you prefer idempotent runs, or keeplatestif automatic upgrades are intentional.Also applies to: 33-42
ansible/roles/debian_base/tasks/60-go-task.yaml (1)
15-28: Consider handling unsupported architectures.The architecture detection sets
debian_base_task_archonly forarmv7l,x86_64, andaarch64. On other architectures, the variable remains undefined, which would cause the download task to fail with an unclear error. Consider adding a fallback or explicit failure with a descriptive message.Example: Add explicit failure for unsupported architectures
- name: Fail on unsupported architecture ansible.builtin.fail: msg: "Unsupported architecture: {{ ansible_architecture }}. Supported: armv7l, x86_64, aarch64" when: - debian_base_task_version_check.rc != 0 - ansible_architecture not in ['armv7l', 'x86_64', 'aarch64']ansible/roles/debian_base/tasks/50-crowdsec.yaml (1)
34-40: Consider addingno_log: trueto prevent API key exposure in logs.The task writes a sensitive API key to the configuration file. Without
no_log: true, the key value could be exposed in Ansible output logs.🔒 Proposed fix
- name: Set crowdsec api_key for iptables bouncer ansible.builtin.lineinfile: path: /etc/crowdsec/bouncers/crowdsec-firewall-bouncer.yaml regexp: "^api_key:" line: "api_key: {{ debian_base_crowdsec_local_api_key }}" when: debian_base_crowdsec_local_api_key|default('') != '' notify: Restart service crowdsec-firewall-bouncer + no_log: trueansible/inventory/group_vars/debian/vars.yaml (1)
3-5: Consider moving sensitive/personal data to secret.yaml.The admin email address is hardcoded here. While not strictly sensitive, you may want to keep all personal identifiers in one place for easier management.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
ansible/apply-adhoc.shansible/inventory/group_vars/debian/vars.yamlansible/inventory/group_vars/mac/vars.yamlansible/inventory/inventory.yamlansible/playbooks/cloud.yamlansible/playbooks/homelab.yamlansible/roles/debian_base/tasks/10-apt.yamlansible/roles/debian_base/tasks/40-network.yamlansible/roles/debian_base/tasks/50-crowdsec.yamlansible/roles/debian_base/tasks/60-go-task.yamlansible/roles/debian_base/tasks/70-user.yamlansible/roles/debian_base/tasks/main.yamlansible/roles/debian_developer/tasks/20-homebrew.yamlansible/roles/debian_developer/tasks/main.yamlansible/roles/debian_tools/tasks/10-apt.yamlansible/roles/debian_tools/tasks/20-homebrew.yamlansible/roles/debian_tools/tasks/main.yamlansible/roles/mac_base/tasks/10-homebrew.yaml
💤 Files with no reviewable changes (2)
- ansible/roles/debian_developer/tasks/20-homebrew.yaml
- ansible/roles/debian_developer/tasks/main.yaml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
YAML files must pass YAML linting and validation
Files:
ansible/roles/debian_tools/tasks/10-apt.yamlansible/roles/debian_tools/tasks/20-homebrew.yamlansible/roles/debian_base/tasks/main.yamlansible/roles/debian_base/tasks/40-network.yamlansible/roles/debian_base/tasks/50-crowdsec.yamlansible/roles/debian_tools/tasks/main.yamlansible/playbooks/cloud.yamlansible/roles/debian_base/tasks/10-apt.yamlansible/inventory/group_vars/mac/vars.yamlansible/playbooks/homelab.yamlansible/roles/mac_base/tasks/10-homebrew.yamlansible/roles/debian_base/tasks/70-user.yamlansible/roles/debian_base/tasks/60-go-task.yamlansible/inventory/inventory.yamlansible/inventory/group_vars/debian/vars.yaml
ansible/**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Ansible playbooks and roles must pass Ansible linting
Files:
ansible/roles/debian_tools/tasks/10-apt.yamlansible/roles/debian_tools/tasks/20-homebrew.yamlansible/roles/debian_base/tasks/main.yamlansible/roles/debian_base/tasks/40-network.yamlansible/roles/debian_base/tasks/50-crowdsec.yamlansible/roles/debian_tools/tasks/main.yamlansible/playbooks/cloud.yamlansible/roles/debian_base/tasks/10-apt.yamlansible/inventory/group_vars/mac/vars.yamlansible/playbooks/homelab.yamlansible/roles/mac_base/tasks/10-homebrew.yamlansible/roles/debian_base/tasks/70-user.yamlansible/roles/debian_base/tasks/60-go-task.yamlansible/inventory/inventory.yamlansible/inventory/group_vars/debian/vars.yaml
**/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Shell scripts must pass ShellCheck validation
Files:
ansible/apply-adhoc.sh
🧠 Learnings (2)
📚 Learning: 2025-01-25T11:16:21.899Z
Learnt from: bubacoder
Repo: bubacoder/infra PR: 89
File: ansible/bootstrap-ansible.sh:23-24
Timestamp: 2025-01-25T11:16:21.899Z
Learning: In the bootstrap-ansible.sh script, the `--ignore-installed` flag is only needed for pip installations on Ubuntu to fix PyYAML conflicts. The Debian installation works fine without this flag.
Applied to files:
ansible/roles/debian_base/tasks/10-apt.yamlansible/inventory/inventory.yamlansible/inventory/group_vars/debian/vars.yaml
📚 Learning: 2025-11-26T20:10:59.672Z
Learnt from: CR
Repo: bubacoder/infra PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T20:10:59.672Z
Learning: Use Ansible for infrastructure configuration management
Applied to files:
ansible/roles/debian_base/tasks/70-user.yamlansible/inventory/inventory.yaml
🧬 Code graph analysis (2)
ansible/roles/debian_tools/tasks/20-homebrew.yaml (1)
ansible/bootstrap-ansible.sh (3)
install_ansible_on_macos(67-74)install_ansible_on_ubuntu(15-26)install_ansible_on_debian(28-65)
ansible/roles/debian_tools/tasks/main.yaml (1)
ansible/bootstrap-ansible.sh (3)
install_ansible_on_macos(67-74)install_ansible_on_ubuntu(15-26)install_ansible_on_debian(28-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (27)
ansible/roles/mac_base/tasks/10-homebrew.yaml (2)
30-32: LGTM! Variable references align with the new naming convention.The variable names correctly match the definitions in
vars.yaml. The concatenation of tool lists is clear and maintainable.Note:
state: latestcan cause non-idempotent behavior (tasks may report changes even when packages are already up-to-date). This is pre-existing behavior, but consider usingstate: presentif idempotency is important for your workflow.
34-38: LGTM! Cask packages variable correctly renamed.The reference to
mac_base_brew_cask_packagesaligns with the new namespace convention.ansible/inventory/group_vars/mac/vars.yaml (4)
3-17: LGTM! Well-organized base tools list with consistent naming.The
mac_base_brew_base_toolsvariable follows the new namespace convention. The inline documentation URLs are a nice touch for maintainability.
19-26: LGTM! Cloud tools properly namespaced.The categorization by cloud provider (AWS, Azure, Generic) with inline comments is clear and maintainable.
36-43: LGTM! Kubernetes tools properly namespaced.Good selection of K8s ecosystem tools with helpful documentation links.
45-48: LGTM! Cask packages properly namespaced.The
mac_base_brew_cask_packagesvariable aligns with the new naming convention.ansible/apply-adhoc.sh (1)
7-7: LGTM!The usage example is correctly updated to reflect the role rename from
debian_developertodebian_tools.ansible/roles/debian_tools/tasks/20-homebrew.yaml (1)
3-14: LGTM!The dynamic package list construction using
set_factwith conditional ternary operators is clean. The defensive defaults (| default(false)for flags,| default([])for lists) ensure graceful handling when variables are undefined.ansible/roles/debian_tools/tasks/10-apt.yaml (1)
2-6: LGTM!The variable rename to
debian_tools_apt_packagesaligns with the consistent namespace convention introduced in this PR.Consider adding a
| default([])fallback if this task should be a no-op when the variable is undefined, rather than failing.ansible/roles/debian_base/tasks/60-go-task.yaml (1)
30-35: LGTM!The variable rename to
debian_base_go_task_versionwith an inline default follows the namespace convention and provides a safe fallback.ansible/roles/debian_base/tasks/main.yaml (1)
14-17: Variable rename is correct; task structure is already compliant with ansible-lint conventions.The rename to
debian_base_install_crowdsec_bounceraligns with the namespace convention. The task structure is correct:name→when→module→becomefollows ansible-lint'skey-orderrule and matches Ansible conventions. No changes needed.ansible/playbooks/cloud.yaml (1)
7-10: LGTM!The role rename from
debian_developertodebian_toolsis consistent with the PR's refactoring objectives. The role ordering is appropriate—markosamuli.linuxbrewis positioned beforedebian_tools, which is correct sincedebian_toolsmay depend on Homebrew for package installation.ansible/roles/debian_base/tasks/50-crowdsec.yaml (1)
26-32: LGTM!The variable renames to
debian_base_crowdsec_local_api_urlanddebian_base_crowdsec_local_api_keyare consistent with the new naming convention. The conditional logic correctly ensures the bouncer service is only enabled when both the API URL and key are configured.Also applies to: 42-49
ansible/roles/debian_base/tasks/10-apt.yaml (1)
32-41: LGTM!The variable renames to
debian_base_apt_packages,debian_base_apt_storage_packages, anddebian_base_install_storage_packagesare consistent with the new naming convention. The boolean filter chain| default(false) | ansible.builtin.boolcorrectly handles undefined variables.ansible/roles/debian_tools/tasks/main.yaml (1)
1-10: LGTM!The task structure is well-organized:
import_tasks(static) for apt packages ensures they're always processedinclude_tasks(dynamic) for Homebrew with a conditional gate is appropriate for optional functionalityThe
whencondition correctly triggers Homebrew installation if eitherdebian_tools_brew_install_groupsordebian_tools_brew_cask_packagesis defined, providing flexibility for different configurations.ansible/roles/debian_base/tasks/40-network.yaml (1)
8-11: LGTM!The variable rename to
debian_base_configure_hosts_fileis consistent with the new naming convention. The boolean filter chain correctly handles undefined variables with a safe default offalse.ansible/inventory/inventory.yaml (3)
24-25: LGTM!The
debian_tools_brew_install_groupsconfiguration forhivecorrectly overrides the default (false) for the kubernetes group only. This aligns with the group definitions invars.yaml.
28-33: LGTM!The variable renaming to
debian_base_prefix is consistent with the refactoring across the codebase. The crowdsec configuration and storage packages flag follow the new naming convention.
35-42: LGTM!The full brew install groups configuration for
nestenables all tool categories. This structure matches the defaults defined invars.yamland provides clear per-host customization.ansible/playbooks/homelab.yaml (1)
17-29: LGTM!The refactoring cleanly separates concerns:
markosamuli.linuxbrewinstalls Homebrew firstdebian_toolsinstalls the configured tool packagesdebian_samba_clientis now a separate play targeting onlynestThis modular structure improves maintainability and allows different hosts to opt into different tool groups via inventory configuration.
ansible/roles/debian_base/tasks/70-user.yaml (3)
2-9: LGTM!User creation task correctly uses the new
debian_base_prefixed variables. The task structure and parameters are appropriate.
17-31: LGTM!SSH key configuration block correctly uses the new variable names. The
ansible.builtin.boolfilter usage on line 18 is the modern fully-qualified form.
33-48: LGTM!Git global config task correctly uses the renamed variables. The
become_userproperly switches to the admin user for setting user-scoped git configuration.ansible/inventory/group_vars/debian/vars.yaml (4)
20-33: LGTM!The base apt packages list is well-organized with sections and helpful URL comments. Good documentation practice.
35-42: LGTM!Storage packages are properly gated behind
debian_base_install_storage_packagesflag, making them opt-in. The package selection is appropriate for storage management tasks.
57-66: LGTM!The brew install groups structure provides a clean opt-in pattern. All groups default to
false, allowing per-host customization in the inventory. This is a flexible and maintainable approach.
68-102: LGTM!Package group definitions are well-organized by category with helpful URL comments. The note about kubectl being managed by asdf (line 98) is a good clarification to prevent duplicate installations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
03cfbc4 to
123536f
Compare
123536f to
d5cfe3e
Compare
Summary by CodeRabbit
New Features
Refactor
Chore
✏️ Tip: You can customize this high-level summary in your review settings.