Simplify codebase: modernize loops, split templates, improve CI

[dguido]

Summary

• Delete 8 empty venv.yml files that were remnants of the pyproject.toml migration and remove their import_tasks references
• Consolidate server.yml async polling from 4 copy-paste blocks to 1 loop
• Wrap sequential VPN service imports in a block with single when condition
• Replace deprecated with_indexed_items with modern loop/loop_control syntax

Impact

• 65 fewer lines (119 removed, 54 added)
• 8 fewer files to maintain
• Removes Ansible deprecation warnings for with_indexed_items
• Cleaner async polling logic - easier to add/remove VPN services
• Sequential mode simplified - single when condition instead of repeating it 4x

Test plan

• ansible-playbook main.yml --syntax-check passes
• ansible-playbook users.yml --syntax-check passes
• ansible-playbook server.yml --syntax-check passes
• ansible-lint passes
• yamllint passes (only pre-existing warnings)
• All 91 unit tests pass
• Pre-commit hooks pass

🤖 Generated with Claude Code

[dguido]

Bug Fixes Applied

The Claude Code bot review was correct - there were bugs in my changes. I've pushed fixes in commit edc6e8a:

1. Template Variable Mismatch (Critical)

Issue: client.conf.j2 template still used item.1 but the loop was changed from with_indexed_items (which produces tuples) to loop (which yields values directly).

Fix: Changed item.1 to item in client.conf.j2:

-PrivateKey = {{ lookup('file', wireguard_pki_path + '/private/' + item.1) }}
+PrivateKey = {{ lookup('file', wireguard_pki_path + '/private/' + item) }}

2. Undefined Variable in Tagged Runs

Issue: When running with tags (e.g., --tags ipsec), variables like dns_job would be undefined because the DNS role was skipped.

Fix: Added | default({}) to all job variable references in _vpn_jobs:

-  - {name: dns, job: "{{ dns_job }}", tag: dns}
+  - {name: dns, job: "{{ dns_job | default({}) }}"}

3. Test Updates

Updated test_template_rendering.py to pass item as a string instead of a tuple, matching the new loop behavior.

All unit tests (101) and pre-commit hooks pass locally. Waiting for CI to confirm the integration tests pass as well.

[claude]

Code Review

This is an excellent cleanup PR that modernizes the codebase and reduces maintenance burden. The changes are well-executed and the code quality improvements are significant.

---

✅ Excellent Work

1. Loop Modernization (Deprecation Fix)

The migration from deprecated with_indexed_items to modern loop + loop_control.index_var is correctly implemented:

Key changes validated:

• roles/wireguard/tasks/main.yml: Uses index_var: index to set the index variable
• roles/wireguard/templates/client.conf.j2: Updated from item.1 → item (username)
• tests/unit/test_template_rendering.py: Updated test data from tuple to string
• The index variable remains available for wireguard_client_ip calculation

Why this matters:

• wireguard_client_ip depends on index (line 12 in roles/wireguard/defaults/main.yml)
• Old: item = (index, username) accessed as item.0 and item.1
• New: item = username, index is a separate variable via index_var
• Template correctly uses item directly for file paths and index for IP calculation

2. Async Polling Consolidation

The refactoring in server.yml from 4 duplicate async_status blocks to a single loop is a significant improvement:

Strengths:

• DRY principle applied correctly
• selectattr('job.ansible_job_id', 'defined') elegantly handles skipped services
• Better error reporting with per-service failure messages
• Easy to extend (add new VPN services)
• Cleaner output with loop_control.label

Example of improvement:

# Before: 4x copy-paste blocks (56 lines)
- async_status: {jid: "{{ dns_job.ansible_job_id }}"}
  register: dns_result
  until: dns_result.finished
  # ... repeated 4 times

# After: 1 loop (19 lines)
- async_status:
    jid: "{{ item.job.ansible_job_id }}"
  loop: "{{ _vpn_jobs | selectattr('job.ansible_job_id', 'defined') | list }}"

3. Sequential Mode Simplification

Wrapping sequential imports in a block with a single when condition eliminates repetition:

• Before: Condition repeated 4x on each import
• After: Single condition on block, cleaner structure
• Maintains correct tag propagation

4. Dead Code Removal

Deleting 8 empty venv.yml files is the right call:

• Leftover from pyproject.toml migration
• Comments in files confirm they're obsolete
• Reduces file count and maintenance burden

---

🔍 Minor Observations

Unused tag Field in server.yml

In server.yml:91-94, each job dict has a tag field that isn't used anywhere:

_vpn_jobs:
  - {name: dns, job: "{{ dns_job | default({}) }}"}  # 'tag' removed in your PR

This is correct - the field was never used, so it's good you didn't add it.

Test Coverage Gap

The PR states "All 91 unit tests pass", but consider adding a test specifically for:

• WireGuard config generation with multiple users (different index values)
• Verifying IP address calculation with the new loop syntax

This would catch any future regressions in the index handling.

---

📊 Impact Summary

Metric	Value
Lines removed	123
Lines added	60
Net reduction	-63 lines
Files deleted	8
Deprecation warnings fixed	✅ with_indexed_items
Code duplication reduced	✅ 4 → 1 async loops
Maintainability	Significantly improved

---

✅ Verification Checklist

According to the PR description, all checks pass:

• ansible-playbook main.yml --syntax-check
• ansible-playbook users.yml --syntax-check
• ansible-playbook server.yml --syntax-check
• ansible-lint
• yamllint
• Unit tests (91 passing)
• Pre-commit hooks

---

🎯 Verdict

LGTM ✅ - This is high-quality refactoring work that:

• Removes deprecated syntax correctly
• Reduces code duplication significantly
• Maintains all existing functionality
• Improves maintainability
• Follows project conventions

The migration from with_indexed_items to loop + index_var is particularly well-executed. The template changes correctly reflect the new variable structure, and the test updates confirm the changes work as expected.

No blocking issues found. This is ready to merge.

---

💡 Optional Future Enhancement

Consider adding an integration test that:

1. Generates WireGuard configs for 3+ users
2. Verifies each user gets a unique IP address
3. Confirms the IP calculation matches expected values

This would provide additional confidence that the index variable is working correctly across the entire workflow.

---

🤖 Generated with Claude Code