Repository navigation
Conversation
Added cifs-utils for SMB/CIFS file sharing support.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR adds Proton VPN installation capability to the Ansible dotfiles configuration, extends the package management tooling with additional utilities and tools, and introduces a NAS credentials template. The changes are modular: a new Proton VPN task file with conditional wiring, configuration variables, documentation updates, and a separate NAS support template with gitignore entry. ChangesSystem Configuration Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Contributor
Reviewer's GuideExtends the Linux Ansible setup by enriching apt and eget package sets (including Pearl tooling), adding an optional Proton VPN APT repo + desktop client flow behind tags/variables, and introducing an example NAS credentials file, with documentation updated for the new tags and behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
Author
|
/co Fix pre-commit formatting issues, nothing else. |
Contributor
|
Fixed pre-commit formatting issues by adding a missing newline at the end of |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
whencondition for importingapt-release-protonvpn.ymlis awkward and likely incorrect ((apt.releases.protonvpn | default(false)) if apt.releases is defined else false); consider simplifying to something likewhen: apt.releases is defined and (apt.releases.protonvpn | default(false) | bool)for clarity and correctness. - The Proton VPN repository
.debURL is pinned to a specific version (1.0.8) and path; consider parameterizing the version or building the URL from variables so it can be updated in one place rather than being hard-coded in the task. - The repeated
--tag pearl-wallet-v1.0.0 -a .tar.gz --file ... --to ... pearl-research-labs/pearlarguments for the pearl-relatedegetpackages could be consolidated via a shared variable or template to avoid duplication and make future updates less error-prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `when` condition for importing `apt-release-protonvpn.yml` is awkward and likely incorrect (`(apt.releases.protonvpn | default(false)) if apt.releases is defined else false`); consider simplifying to something like `when: apt.releases is defined and (apt.releases.protonvpn | default(false) | bool)` for clarity and correctness.
- The Proton VPN repository `.deb` URL is pinned to a specific version (`1.0.8`) and path; consider parameterizing the version or building the URL from variables so it can be updated in one place rather than being hard-coded in the task.
- The repeated `--tag pearl-wallet-v1.0.0 -a .tar.gz --file ... --to ... pearl-research-labs/pearl` arguments for the pearl-related `eget` packages could be consolidated via a shared variable or template to avoid duplication and make future updates less error-prone.
## Individual Comments
### Comment 1
<location path=".ansible/playbooks/setup-linux.yml" line_range="48-52" />
<code_context>
become: true
tags: [apt, brave]
+ - name: Import Proton VPN installation tasks
+ ansible.builtin.import_tasks: tasks/apt-release-protonvpn.yml
+ when: (apt.releases.protonvpn | default(false)) if apt.releases is defined else false
+ become: true
+ tags: [apt, protonvpn, vpn]
</code_context>
<issue_to_address>
**suggestion:** Simplify the `when` expression to avoid the ternary and potential edge cases when `apt.releases` is missing or falsy.
The current `when` clause uses a Jinja ternary:
```yaml
(apt.releases.protonvpn | default(false)) if apt.releases is defined else false
```
A clearer and more idiomatic alternative is:
```yaml
when: apt.releases is defined and (apt.releases.protonvpn | default(false))
```
This relies on short-circuiting, keeps the logic in standard `when` form, and is easier to read and maintain.
```suggestion
- name: Import Proton VPN installation tasks
ansible.builtin.import_tasks: tasks/apt-release-protonvpn.yml
when: apt.releases is defined and (apt.releases.protonvpn | default(false))
become: true
tags: [apt, protonvpn, vpn]
```
</issue_to_address>
### Comment 2
<location path=".ansible/playbooks/tasks/apt-release-protonvpn.yml" line_range="2-5" />
<code_context>
+---
+- name: Download Proton VPN stable release package
+ ansible.builtin.get_url:
+ url: https://repo.protonvpn.com/debian/dists/stable/main/binary-all/protonvpn-stable-release_1.0.8_all.deb
+ dest: /tmp/protonvpn-stable-release_1.0.8_all.deb
+ mode: '0644'
+
</code_context>
<issue_to_address>
**suggestion:** Consider parameterizing the Proton VPN release version and download path for maintainability.
Both the URL and `dest` hard-code `1.0.8`, so each new release will require updating this task in multiple places. Defining a variable such as `protonvpn_release_version: '1.0.8'` and reusing it in both fields would simplify upgrades and reduce mistakes. You could also consider making the download directory configurable to stay consistent with other APT bootstrap tasks.
</issue_to_address>
### Comment 3
<location path=".ansible/playbooks/tasks/eget.yml" line_range="86-89" />
<code_context>
jq: >-
-a {{ 'linux64' if eget_arch == 'x86_64' else 'linux-arm64' if eget_arch == 'arm64' else 'linux-arm' }}
jqlang/jq
+ oyster: "--tag pearl-wallet-v1.0.0 -a .tar.gz --file oyster --to oyster pearl-research-labs/pearl"
pandoc: "-a linux-{{ 'amd64' if eget_arch == 'x86_64' else 'arm64' }}.tar.gz jgm/pandoc"
+ pearld: "--tag pearl-wallet-v1.0.0 -a .tar.gz --file pearld --to pearld pearl-research-labs/pearl"
+ prlctl: "--tag pearl-wallet-v1.0.0 -a .tar.gz --file prlctl --to prlctl pearl-research-labs/pearl"
rg: "-a {{ eget_plain_arch }}-unknown-linux-musl.tar.gz BurntSushi/ripgrep"
tealdeer: "-a tealdeer-linux-{{ eget_arch }}-musl -a ^sha256 dbrgn/tealdeer"
</code_context>
<issue_to_address>
**suggestion:** Avoid repeating the Pearl wallet version string across multiple eget entries.
`oyster`, `pearld`, and `prlctl` all hardcode `--tag pearl-wallet-v1.0.0`, duplicating the version and increasing maintenance overhead. Please define a single variable (e.g. `pearl_wallet_version`) and interpolate it into these entries to keep the Pearl tooling version consistent and easier to update.
Suggested implementation:
```
oyster: "--tag {{ pearl_wallet_version }} -a .tar.gz --file oyster --to oyster pearl-research-labs/pearl"
```
```
pearld: "--tag {{ pearl_wallet_version }} -a .tar.gz --file pearld --to pearld pearl-research-labs/pearl"
```
```
prlctl: "--tag {{ pearl_wallet_version }} -a .tar.gz --file prlctl --to prlctl pearl-research-labs/pearl"
```
Define `pearl_wallet_version` once in an appropriate variables scope so it’s available to this task, for example:
- In the same play under `vars:`:
`pearl_wallet_version: pearl-wallet-v1.0.0`
- Or in a group/host var file used by this play.
Ensure the variable name matches exactly (`pearl_wallet_version`) and that Jinja2 interpolation is enabled for this YAML section (which is standard for Ansible task files).
</issue_to_address>
### Comment 4
<location path=".ansible/README.md" line_range="41" />
<code_context>
| `nvidia` | Handles NVIDIA/CUDA related installations. |
+| `protonvpn` | Installs Proton VPN app from the official Proton APT repository. |
| `upgrade` | Performs `apt upgrade` and updates `eget` packages. |
+| `vpn` | Runs VPN related tasks. |
| `vscode` | Installs Visual Studio Code. |
</code_context>
<issue_to_address>
**nitpick (typo):** Consider hyphenating “VPN related” to “VPN-related”.
In this row, update the description to “Runs VPN-related tasks.” to match standard hyphenation and keep the table wording consistent.
```suggestion
| `vpn` | Runs VPN-related tasks. |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+48
to
+52
| - name: Import Proton VPN installation tasks | ||
| ansible.builtin.import_tasks: tasks/apt-release-protonvpn.yml | ||
| when: (apt.releases.protonvpn | default(false)) if apt.releases is defined else false | ||
| become: true | ||
| tags: [apt, protonvpn, vpn] |
Contributor
There was a problem hiding this comment.
suggestion: Simplify the when expression to avoid the ternary and potential edge cases when apt.releases is missing or falsy.
The current when clause uses a Jinja ternary:
(apt.releases.protonvpn | default(false)) if apt.releases is defined else falseA clearer and more idiomatic alternative is:
when: apt.releases is defined and (apt.releases.protonvpn | default(false))This relies on short-circuiting, keeps the logic in standard when form, and is easier to read and maintain.
Suggested change
| - name: Import Proton VPN installation tasks | |
| ansible.builtin.import_tasks: tasks/apt-release-protonvpn.yml | |
| when: (apt.releases.protonvpn | default(false)) if apt.releases is defined else false | |
| become: true | |
| tags: [apt, protonvpn, vpn] | |
| - name: Import Proton VPN installation tasks | |
| ansible.builtin.import_tasks: tasks/apt-release-protonvpn.yml | |
| when: apt.releases is defined and (apt.releases.protonvpn | default(false)) | |
| become: true | |
| tags: [apt, protonvpn, vpn] |
Comment on lines
+2
to
+5
| - name: Download Proton VPN stable release package | ||
| ansible.builtin.get_url: | ||
| url: https://repo.protonvpn.com/debian/dists/stable/main/binary-all/protonvpn-stable-release_1.0.8_all.deb | ||
| dest: /tmp/protonvpn-stable-release_1.0.8_all.deb |
Contributor
There was a problem hiding this comment.
suggestion: Consider parameterizing the Proton VPN release version and download path for maintainability.
Both the URL and dest hard-code 1.0.8, so each new release will require updating this task in multiple places. Defining a variable such as protonvpn_release_version: '1.0.8' and reusing it in both fields would simplify upgrades and reduce mistakes. You could also consider making the download directory configurable to stay consistent with other APT bootstrap tasks.
Comment on lines
+86
to
+89
| oyster: "--tag pearl-wallet-v1.0.0 -a .tar.gz --file oyster --to oyster pearl-research-labs/pearl" | ||
| pandoc: "-a linux-{{ 'amd64' if eget_arch == 'x86_64' else 'arm64' }}.tar.gz jgm/pandoc" | ||
| pearld: "--tag pearl-wallet-v1.0.0 -a .tar.gz --file pearld --to pearld pearl-research-labs/pearl" | ||
| prlctl: "--tag pearl-wallet-v1.0.0 -a .tar.gz --file prlctl --to prlctl pearl-research-labs/pearl" |
Contributor
There was a problem hiding this comment.
suggestion: Avoid repeating the Pearl wallet version string across multiple eget entries.
oyster, pearld, and prlctl all hardcode --tag pearl-wallet-v1.0.0, duplicating the version and increasing maintenance overhead. Please define a single variable (e.g. pearl_wallet_version) and interpolate it into these entries to keep the Pearl tooling version consistent and easier to update.
Suggested implementation:
oyster: "--tag {{ pearl_wallet_version }} -a .tar.gz --file oyster --to oyster pearl-research-labs/pearl"
pearld: "--tag {{ pearl_wallet_version }} -a .tar.gz --file pearld --to pearld pearl-research-labs/pearl"
prlctl: "--tag {{ pearl_wallet_version }} -a .tar.gz --file prlctl --to prlctl pearl-research-labs/pearl"
Define pearl_wallet_version once in an appropriate variables scope so it’s available to this task, for example:
- In the same play under
vars::
pearl_wallet_version: pearl-wallet-v1.0.0 - Or in a group/host var file used by this play.
Ensure the variable name matches exactly (pearl_wallet_version) and that Jinja2 interpolation is enabled for this YAML section (which is standard for Ansible task files).
| | `nvidia` | Handles NVIDIA/CUDA related installations. | | ||
| | `protonvpn` | Installs Proton VPN app from the official Proton APT repository. | | ||
| | `upgrade` | Performs `apt upgrade` and updates `eget` packages. | | ||
| | `vpn` | Runs VPN related tasks. | |
Contributor
There was a problem hiding this comment.
nitpick (typo): Consider hyphenating “VPN related” to “VPN-related”.
In this row, update the description to “Runs VPN-related tasks.” to match standard hyphenation and keep the table wording consistent.
Suggested change
| | `vpn` | Runs VPN related tasks. | | |
| | `vpn` | Runs VPN-related tasks. | |
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 Sourcery
Extend Linux setup playbook with additional apt and eget tools and introduce Proton VPN installation support.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Documentation