From 81690dae0fafb28c937d6791cdffc1bd8637efaf Mon Sep 17 00:00:00 2001
From: Erich Blume <blume.erich@gmail.com>
Date: Fri, 13 Feb 2026 16:54:42 -0800
Subject: [PATCH] Review add-ansible-role doc (#185)

## Summary
- Replace `op item get --fields` with `op read` for secrets (matches playbook and CLAUDE.md guidance)
- Change `tags: [<role>]` to `tags: <role>` to match actual playbook style
- Remove redundant `listen:` from handler example, add `changed_when: true`
- Name handler after specific service (e.g. `Restart <service>`) to match real roles
- Add `last-reviewed: 2026-02-13` frontmatter

## Also noted (not fixed here)
Two other docs still use the old `op item get` pattern:
- `docs/how-to/troubleshooting.md:72` (ArgoCD login command)
- `docs/how-to/gandi-operations.md:35` (Gandi token export)

These can be fixed in their own review cycles.

Reviewed-on: https://forge.ops.eblu.me/eblume/blumeops/pulls/185
---
 .../review-add-ansible-role.doc.md            |  1 +
 docs/how-to/add-ansible-role.md               | 21 ++++++++++---------
 2 files changed, 12 insertions(+), 10 deletions(-)
 create mode 100644 docs/changelog.d/review-add-ansible-role.doc.md

diff --git a/docs/changelog.d/review-add-ansible-role.doc.md b/docs/changelog.d/review-add-ansible-role.doc.md
new file mode 100644
index 0000000..0f11bd1
--- /dev/null
+++ b/docs/changelog.d/review-add-ansible-role.doc.md
@@ -0,0 +1 @@
+Review add-ansible-role doc: fix secrets to use `op read`, match tag format to playbook, fix handler pattern, add last-reviewed date.
diff --git a/docs/how-to/add-ansible-role.md b/docs/how-to/add-ansible-role.md
index cad4095..a7c2ded 100644
--- a/docs/how-to/add-ansible-role.md
+++ b/docs/how-to/add-ansible-role.md
@@ -1,6 +1,7 @@
 ---
 title: Add Ansible Role
-modified: 2026-02-07
+modified: 2026-02-13
+last-reviewed: 2026-02-13
 tags:
   - how-to
   - ansible
@@ -44,24 +45,24 @@ role_port: 8080
     src: config.j2
     dest: "{{ role_data_dir }}/config"
     mode: '0644'
-  notify: Restart service
+  notify: Restart <service>
 
 - name: Deploy LaunchAgent plist
   ansible.builtin.template:
     src: launchagent.plist.j2
     dest: ~/Library/LaunchAgents/mcquack.<service>.plist
     mode: '0644'
-  notify: Restart service
+  notify: Restart <service>
 ```
 
 ```yaml
 # ansible/roles/<role>/handlers/main.yml
 ---
-- name: Restart service
+- name: Restart <service>
   ansible.builtin.shell: |
     launchctl unload ~/Library/LaunchAgents/mcquack.<service>.plist 2>/dev/null || true
     launchctl load ~/Library/LaunchAgents/mcquack.<service>.plist
-  listen: Restart service
+  changed_when: true
 ```
 
 ## Add Role to Playbook
@@ -72,7 +73,7 @@ Edit `ansible/playbooks/indri.yml`:
   roles:
     # ... existing roles ...
     - role: <role>
-      tags: [<role>]
+      tags: <role>
 ```
 
 ## Add Secrets (if needed)
@@ -84,19 +85,19 @@ If the role needs secrets from 1Password, add pre_tasks:
     # ... existing pre_tasks ...
     - name: Fetch <role> secret
       ansible.builtin.command:
-        cmd: op --vault vg6xf6vvfmoh5hqjjhlhbeoaie item get <item-id> --fields <field> --reveal
+        cmd: op read "op://vg6xf6vvfmoh5hqjjhlhbeoaie/<item-id>/<field>"
       delegate_to: localhost
       register: _role_secret
       changed_when: false
       no_log: true
       check_mode: false
-      tags: [<role>]
+      tags: <role>
 
     - name: Set <role> secret fact
       ansible.builtin.set_fact:
         role_secret_var: "{{ _role_secret.stdout }}"
       no_log: true
-      tags: [<role>]
+      tags: <role>
 ```
 
 Then use `role_secret_var` in your role with a guard:
@@ -105,7 +106,7 @@ Then use `role_secret_var` in your role with a guard:
 # In role's tasks, fetch if not already set (allows running with --tags)
 - name: Fetch secret if not set
   ansible.builtin.command:
-    cmd: op --vault vg6xf6vvfmoh5hqjjhlhbeoaie item get <item-id> --fields <field> --reveal
+    cmd: op read "op://vg6xf6vvfmoh5hqjjhlhbeoaie/<item-id>/<field>"
   delegate_to: localhost
   register: _role_secret
   changed_when: false