Ansible: Support more SMB servers, refactor homebrew and asdf install - #354
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (11)
WalkthroughThe PR removes the standalone ChangesDebian tooling and Samba updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ansible/roles/debian_samba_client/tasks/20-samba-server.yaml`:
- Around line 2-11: The credentials file task for samba_server.hostname is
rendering admin_password directly in copy.content, which can leak secrets in
task output or diffs. Add no_log: true to the task that creates the SMB
credentials file so the sensitive content is suppressed during execution, while
keeping the existing when: admin_password is defined guard and the current copy
behavior unchanged.
- Around line 13-38: The tasks that create the mount base and share directories
are still running even when the CIFS mount in Mount shares from {{
samba_server.hostname }} is skipped by the admin_password condition. Move the
directory creation logic into the same conditional guard as the
ansible.posix.mount task, or otherwise ensure both Create mount base directory
for {{ samba_server.hostname }} and Create mount directories for {{
samba_server.hostname }} only run when the SMB mount will actually be performed.
In `@ansible/roles/debian_tools/tasks/20-homebrew.yaml`:
- Around line 15-17: The Homebrew install step is executing the upstream
installer from HEAD, which is not reproducible and creates a supply-chain risk.
Update the ansible.builtin.shell task in the Homebrew install block to use a
pinned installer revision or a locally vendored, checksummed script instead of
fetching HEAD at runtime. Keep the change confined to the Install Homebrew task
so provisioning remains deterministic and safer.
In `@ansible/roles/debian_tools/tasks/30-asdf.yaml`:
- Around line 9-27: The current guard in the asdf install task only checks
whether the binary exists, so `debian_tools_asdf_version` changes are ignored
after the first run. Update the logic around the `stat` check and the
`ansible.builtin.unarchive` task in `30-asdf.yaml` so it verifies the installed
asdf version matches `debian_tools_asdf_version` before skipping the download,
and only reuse the existing binary when the pinned version is already present.
- Around line 29-35: The PATH setup in the asdf-related task only adds the shims
directory, so interactive shells may still miss the asdf binary itself. Update
the lineinfile entry in the asdf task to add both ~/.local/bin and the shims
path to PATH, keeping the same shell startup file handling and using the
existing task name and lineinfile configuration as the place to adjust.
In `@ansible/roles/debian_tools/tasks/main.yaml`:
- Around line 7-17: The setup docs are still pointing users at the old Homebrew
role path, so update the documentation to make debian_tools the single
entrypoint for Homebrew setup and remove the separate debian_homebrew inclusion.
Adjust the relevant setup instructions in docs/setup.md to match the new role
structure introduced by debian_tools and its include_tasks flow for Homebrew, so
fresh installs follow the correct path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6167182e-61ef-4322-8792-d180482da4cd
📒 Files selected for processing (14)
ansible/inventory/group_vars/debian/vars.yamlansible/inventory/inventory.yamlansible/playbooks/cloud.yamlansible/playbooks/homelab.yamlansible/roles/common/tasks/asdf.yamlansible/roles/debian_homebrew/tasks/main.yamlansible/roles/debian_samba_client/defaults/main.yamlansible/roles/debian_samba_client/tasks/10-samba.yamlansible/roles/debian_samba_client/tasks/20-samba-server.yamlansible/roles/debian_samba_client/tasks/main.yamlansible/roles/debian_tools/tasks/20-homebrew.yamlansible/roles/debian_tools/tasks/30-asdf.yamlansible/roles/debian_tools/tasks/main.yamlansible/roles/mac_base/tasks/main.yaml
💤 Files with no reviewable changes (3)
- ansible/roles/debian_homebrew/tasks/main.yaml
- ansible/playbooks/cloud.yaml
- ansible/playbooks/homelab.yaml
Comment on lines
+15
to
+17
| - name: Install Homebrew | ||
| ansible.builtin.shell: | | ||
| /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the Homebrew installer instead of executing HEAD.
curl .../HEAD/install.sh | bash makes every provision depend on whatever script upstream serves at runtime. That is unreproducible and creates a direct remote-code-execution supply-chain risk on all target hosts. Fetch a pinned revision or checksummed local copy first, then execute that.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ansible/roles/debian_tools/tasks/20-homebrew.yaml` around lines 15 - 17, The
Homebrew install step is executing the upstream installer from HEAD, which is
not reproducible and creates a supply-chain risk. Update the
ansible.builtin.shell task in the Homebrew install block to use a pinned
installer revision or a locally vendored, checksummed script instead of fetching
HEAD at runtime. Keep the change confined to the Install Homebrew task so
provisioning remains deterministic and safer.
Comment on lines
+9
to
+27
| - name: Check if asdf binary is already installed | ||
| ansible.builtin.stat: | ||
| path: "{{ ansible_facts['env']['HOME'] }}/.local/bin/asdf" | ||
| register: _debian_tools_asdf_installed | ||
| become: false | ||
|
|
||
| - name: Download and install asdf binary | ||
| ansible.builtin.unarchive: | ||
| src: >- | ||
| https://github.com/asdf-vm/asdf/releases/download/{{ debian_tools_asdf_version }}/asdf-{{ debian_tools_asdf_version }}-linux-{{ | ||
| 'amd64' if ansible_facts['architecture'] == 'x86_64' else 'arm64' | ||
| }}.tar.gz | ||
| dest: "{{ ansible_facts['env']['HOME'] }}/.local/bin" | ||
| remote_src: true | ||
| include: | ||
| - asdf | ||
| mode: "0755" | ||
| become: false | ||
| when: not _debian_tools_asdf_installed.stat.exists |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The version pin never reapplies after the first install.
This guard only checks whether ~/.local/bin/asdf exists. Once the file is present, changing debian_tools_asdf_version will not fetch the new release, so the inventory pin is ignored on later runs. Compare the installed version against debian_tools_asdf_version before skipping the download.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ansible/roles/debian_tools/tasks/30-asdf.yaml` around lines 9 - 27, The
current guard in the asdf install task only checks whether the binary exists, so
`debian_tools_asdf_version` changes are ignored after the first run. Update the
logic around the `stat` check and the `ansible.builtin.unarchive` task in
`30-asdf.yaml` so it verifies the installed asdf version matches
`debian_tools_asdf_version` before skipping the download, and only reuse the
existing binary when the pinned version is already present.
bubacoder
force-pushed
the
feature/ansible-refactor
branch
from
June 27, 2026 09:49
abf4718 to
7a31083
Compare
bubacoder
force-pushed
the
feature/ansible-refactor
branch
from
June 27, 2026 11:11
7a31083 to
74e602c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
asdfinstallation and shell setup for Debian hosts.