vikunja/service: install Vikunja natively on the guest #2
No reviewers
Labels
No labels
needs-info
needs-triage
ready-for-agent
ready-for-human
wontfix
needs-info
needs-triage
ready-for-agent
ready-for-human
review/merge-ready
review/needs-fix
review/needs-human
review/needs-review
wayfinder:grilling
wayfinder:map
wayfinder:prototype
wayfinder:research
wayfinder:task
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
olympus/infra-tracker!2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "hermes/service-stack"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The service layer is applied and verified against the live guest. Vikunja runs at
https://vikunja.thepit.space/once the Edge exists; over the LAN it is serving on10.12.0.142:3456now.What landed
vikunja/service/— an Ansible role that installs Vikunja natively (no container): PostgreSQL 17 from the Debian repo, the upstreamv2.7.0binary via a checksum-pinned download, a config rendered from a template, and a sandboxed systemd unit. Plusmake service-check/make serviceand thenpm/folder still to come.Verified on the guest (vmid 142)
get_urlchecksumsystemctl status vikunjaactive (running), enabled, starts at bootGET /200, frontend servedGET /api/v1/info200,frontend_url: https://vikunja.thepit.space/vikunja doctorrc=0)pit200, token issued201— and deleted againmake servicerunchanged=0Decisions worth your eye
inappropriate ioctl for device, so it goes through argv (--password, task isno_log) — same trade-off the forgejo role records.defaults/main.ymlafter measuring it. Bumping the version means updating both the version and that hash.vault_vikunja_smtp_passwordis set.mail.smtp2go.com:2525is wired in; add the password and the next run turns it on.ansible-vaultfile; the vault password lives in the root.envand the Makefile derives the gitignoredvault-passfrom it. No secret value appears in the diff (checked).Not done yet
npm/) — the proxy hostvikunja.thepit.space→10.12.0.142:3456, websockets on. The public URL will not answer until this exists, and it needs the LE cert you are creating in the NPM UI.pitis the only account; add more viavault_vikunja_account_passwords.Code review — two axes
Fixed point:
main(cf685d9). Diff:git diff main...HEAD(13 files, 469 insertions, 3 commits:9f4fca0,60c8f06,510e0a4).Standards sources found:
README.md→## Conventionsonly (no CONTRIBUTING/CODING_STANDARDS/AGENTS.md/.ansible-lint/.yamllint in the repo), so the Fowler smell baseline carries the rest of that axis. No issue-tracker file (docs/agents/issue-tracker.mdabsent), so the Spec axis reviews against this PR's body + the three commit messages.Standards
Blocking
B1.
make service/make service-checkprint every vaulted secret to the terminal — the rendered config embeds secrets (config.yml.j2L5secret:, L15password:, L28password:), and the template task has nono_log(roles/vikunja/tasks/main.ymlL111-118). The Makefile runs exactly that task with--check --diff(MakefileL79-81, L101-105);templateis check-mode-safe, so it runs and--diffdumps the rendered file. The DB password, JWT secret, and SMTP password leak to the terminal/logs before the operator is asked for the confirm word.postgresql_userand account-create correctly carryno_log: true(L96, L242); this one was missed. Fix:no_log: trueon the template task and/or drop--difffromservice_rehearsal. (Commit510e0a4turned the mailer on, so the SMTP password is live in this dump too.)Significant
S2.
host_key_checking = accept_newsilently disables host-key checking —vikunja/service/ansible.cfg:4. The option is a boolean; any non-truthy string coerces toFalse.ansible-config dumpconfirmsaccept_new,accept-new, andbogusall resolve toHOST_KEY_CHECKING = Falsewith no warning, so Ansible passesStrictHostKeyChecking=no— which also accepts changed keys (MITM exposure). For realaccept-newsemantics:[ssh_connection] ssh_args = -o StrictHostKeyChecking=accept-new.Nits
ansible.cfgL6vault_password_file/ L2inventoryare CWD-relative;ansible-playbookfrom the repo root won't load them and vault decryption fails.vault_pass_sync(MakefileL54-62) writes thenchmod 600— the file is briefly at umask (644).umask 077before the write closes it.unarchive ... creates:with the versioned filename leaves the old/opt/vikunja/vikunja-vX-linux-amd64behind on a version bump.Claim checks
vault.ymlwith the repovault-pass; no vaulted value appears ingit diff main...hermes/service-stack.user list --emailrc-gate,/usr/bin/psqlstat,vikunja_bin.stat.exists);migraterelies on achanged_whenstdout heuristic. Not executed.mainuntouched; every IP is RFC1918 LAN; the only public name isvikunja.thepit.space(permitted);vault-passgitignored and never committed.Spec
(a) Missing / partial
vault_vikunja_account_passwords” is half-wired:tasks/main.ymlindexesvault_vikunja_account_passwords[item.item.username]with nodefault, so adding an account needs edits in two places and a username in only one fails the run.apt: name: postgresql— no version pin/assert; fine on Debian 13, silently a different major elsewhere.vault_vikunja_smtp_passwordis set” is stale vs HEAD: commit510e0a4stores that secret, somailer.enablednow resolves true (the body's verified table predates this).(b) Not asked for (scope creep)
config.yml.j2/vars.ymlalready hard-codeipextractionmethod: "xff"andtrustedproxies: 10.12.0.113/32(NPM, which does not exist yet).files.maxsize: "20MB", the.gitignore*.plaintextstaging rule, andhost_key_checking = accept_new— none appear in the spec.(c) Implemented but wrong
ansible.cfghost_key_checking = accept_newis not a valid boolean → coerced toFalse(host-key checking disabled outright, not “accept new”).migratechanged_when: "'Ran all migrations successfully' not in vikunja_migrate.stdout"can never be true — Vikunja logs that exact line on every successful run regardless of whether migrations applied. So the change signal the “idempotent (a second run changes nothing)” claim rests on is vacuous for this task.vikunja_log_diris created and added toReadWritePaths, but nothing setslog.path; logs actually go to<rootpath>logs(/var/lib/vikunja/logs), leaving/var/log/vikunjaunused.Confirmed true (independently checkable spec claims): pinned sha256 matches the live v2.7.0 archive (
569861fd…);user list --emailexits 1 when absent / 0 when present;vault.ymlis ciphertext-only andvault-pass/.envare gitignored and untracked.Summary — Standards: 1 blocking + 1 significant + 3 nits; worst = B1 (secrets printed by the rehearsal/apply gate). Spec: 3 missing/partial, 2 scope-creep, 3 implemented-but-wrong; worst = the
migratechanged_whenmaking the idempotency signal vacuous (plus theaccept_newmisconfiguration shared with Standards).Review fixes are in, verified against the live guest.
mainis untouched atcf685d9.Blocking
B1 — secrets printed by the rehearsal. Real, and reproduced: a probe playbook running
template --check --diffprinted the rendered file verbatim (+secret: "SUPERSECRETVALUE123"). On the live host it showedokonly because the config already matched, so the leak was latent, not absent. Fixed withno_log: trueon the template task. I first dropped--difffromservice_rehearsal, then reverted it: withno_logthe secret-bearing diff is suppressed and the harmless diffs stay visible. Verified: no secret value appears in the rehearsal output.Significant
S2 —
host_key_checking = accept_new. Confirmed a boolean coercion:accept_new,accept-new, andbogusall resolve toFalse, so Ansible was passingStrictHostKeyChecking=no— which also accepts a changed key. Replaced withssh_args = -o StrictHostKeyChecking=accept-new.Implemented but wrong
migratechanged_whenwas vacuous. Verified: Vikunja logs"Ran all migrations successfully."on a fresh DB and on a no-op run alike — the line never varies. Nowchanged_when: false, which is what a run-every-time command with nothing to report is.vikunja_log_dirunused. Correct, andlog.pathwas indeed missing — but setting it doesn't help:log.standarddefaults tostdout, so logs go to the journal either way. Rather than create a directory nothing writes to, I removed the dir, the write grant, andlog.path. Confirmed on the guest: logs land in the journal; the directory is gone.Spec gaps
files.maxsize(the 20MB default is unchanged) and the*.plaintextignore rule.Nits
vault_pass_syncwrites underumask 077rather than chmod-after-write, so the file is never briefly world-readable.DEFAULT_VAULT_PASSWORD_FILEresolves to the absoluteservice/vault-passfrom any working directory (checked), so it's left as is with a comment.A regression I introduced
The
creates:nit made me delete the versioned binary after install — which broke the next run, because thecopystill read it as its source (Source /opt/vikunja/vikunja-v2.7.0-linux-amd64 not found). That failed a real run. Rewrote it: read the installed version, install only when the pinned version differs. Both the nit and the regression are gone.Also
The amd64 assert I added carried its own
[DEPRECATION WARNING]: INJECT_FACTS_AS_VARS(top-level fact injection, removed in ansible-core 2.24). Switched toansible_facts["architecture"].Verification
Two consecutive real runs on vmid 142:
changed=3thenchanged=0,failed=0, nofatal.ansible-playbook --syntax-checkpasses. No secret value in the rehearsal output or anywhere in the worktree; every value in the vault is untouched.Code review — two axes (re-review)
Fixed point:
main(cf685d9). Diff:git diff main...HEAD(13 files, 528 insertions, 5 commits:9f4fca0,60c8f06,510e0a4,2bec6bb,a247388). This re-run also checks the prior review (#issuecomment-647) against the fix commits and the author's reply (#issuecomment-650).Standards sources:
README.md→## Conventionsonly (no CONTRIBUTING / CODING_STANDARDS / AGENTS.md / .ansible-lint / .yamllint in the repo), so the Fowler smell baseline carries the rest. No issue-tracker file, so the Spec axis reviews against the PR body + the five commit messages.Standards
Documented-standard violations: none. Convention (
README.md:120-124): branch/PR obeyed; no address committed. Every IP is private (10.12.0.142/24,10.12.0.1, proxy10.12.0.113/32);vikunja.thepit.spaceis a public service domain andmail.smtp2go.coman upstream mirror — both explicitly allowed. Vault stays ciphertext.Baseline smells (all judgement calls):
when: not ansible_check_mode or vikunja_bin.stat.exists | default(false)repeats 6× (tasks/main.ymlL204, L212, L227, L250, L273;handlers/main.ymlL7). A single set-fact gate could drive them all.runuser -u {{ vikunja_user }} -- {{ vikunja_binary }} …argv repeats 3× (L228-237, L251-263, L275-291). Extract a shared prefix.{{ vikunja_install_dir }}/vikunja-v{{ vikunja_version }}-linux-amd64(plus the/tmpzip name) recurs 5× (L93, L100, L104, L109, L121); onevikunja_release_*var would carry the clump.vikunja_version not in (vikunja_bin_version.stdout | default(''))(tasks/main.ymlL86-87)."2.7.0"is a substring of e.g."12.7.0", so an install could be silently skipped. Parse the version and match exactly.vikunja_pgregisters astatof/usr/bin/psql(L128-131); the name doesn't say "postgres installed".inventories/prod/hostsalso names both the group and the hostvikunja(Ansible warns on the collision).Prior Standards findings — resolution:
no_log: true(L164-166). Verified:ansible-playbook --check --diffwithno_logprints onlychanged, no diff and no secret.ansible.cfgusesssh_args = -o StrictHostKeyChecking=accept-new. Cross-checked againstplugins/connection/ssh.py:StrictHostKeyChecking=nois added only whenhost_key_checking is False, and the dump shows it defaulting True./tmp,ansible-config dumpshows bothDEFAULT_HOST_LISTandDEFAULT_VAULT_PASSWORD_FILEresolving to the absoluteservice/paths — Ansible resolves cfg paths relative to the cfg file.vault_pass_syncchmod) — RESOLVED. Now(umask 077; printf … > $(VAULT_PASS))(Makefile:60).creates:leftover) — RESOLVED. Version-aware install plus an explicitfile: state=absent(L118-122).set_factgate (L84-87) coerces to a real bool (bump→True, same→False); download/unarchive/copy/delete share one condition, so there is no stale copy source and no version accumulation. The only caveat is the substring match (smell #4).Spec
(a) Missing / partial
vault_vikunja_account_passwords" — still half-wired. Accounts live in two files:defaults/main.ymlvikunja_accountsandvault.ymlvault_vikunja_account_passwords. The new assert (item.username in vault_vikunja_account_passwords,tasks/main.ymlL32-42) catches a username missing its password — it converts a deep undefined-var crash into a clear failure, but the two-place edit and the undocumented recipe remain. Partially resolved.apt: name: postgresql(tasks/main.ymlL13) with no version pin or assert; the README asserts "PostgreSQL 17" (L68) that nothing verifies. The amd64 assert (L22-28) does not cover this. Not resolved.(b) Not asked for (scope creep)
config.yml.j2still hard-codesipextractionmethod: "xff"andtrustedproxies: "10.12.0.113/32"(L7-8;vars.ymlL8) for the NPM Edge that "will not answer until this exists". Not asked for, and dead config while the proxy is absent. Not resolved.files.maxsizeand the.gitignore*.plaintextrule — dropped, resolved.host_key_checking accept_new— reframed into the validssh_args -o StrictHostKeyChecking=accept-new(ansible.cfgL14). Partially (an SSH-policy line remains, now in correct form).(c) Implemented but wrong
host_key_checking accept_new(invalid boolean, coerced to False) → removed, replaced withssh_args. Resolved.migratechanged_when(dead branch) → now explicitchanged_when: falsewith rationale (L241). Resolved.vikunja_log_dircreated and added toReadWritePathswith nothing writing it → dir removed;ReadWritePaths={{ vikunja_home }}. Resolved.Remaining findings to raise:
README.md:68vstasks/main.yml:13).xff+trustedproxiesprovisioned for a proxy that does not exist — unrequested, andipextractionmethod: xffwith no live proxy is the kind of wrong-when-inert config worth deferring to thenpm/PR.Verdict: 5 of 8 prior Spec findings resolved; account wiring partial, PG pin missing, xff/trustedproxies creep open.
Summary — Standards: 0 documented-standard breaches, 5 judgement-call smells (all prior Standards findings resolved); worst = the substring version gate (#4), which could silently skip an install. Spec: 3 open findings (PG pin, account wiring, xff/trustedproxies creep), 5 of 8 prior items resolved; worst = the unpinned PostgreSQL version contradicting the README's "PostgreSQL 17" claim.
Re-review fixes are in, verified against the live guest.
mainis untouched atcf685d9; head is710f668.The worst finding — real bug, fixed
Version gate (smell #4). Confirmed by check:
"2.7.0" in "v12.7.0"isTrue, so a 12.x binary on disk would have skipped an install it should have done. Now it parses the version and compares exactly. Verified against the real two-linevikunja versionoutput, the12.7.0case, and garbage — correct version → no reinstall,12.7.0→ reinstall, garbage → reinstall.Spec findings
postgresql-17instead of barepostgresql, plus an assert that the 17 tree is present. The README claimed 17 and nothing verified it.xff+trustedproxiescreep. Dropped. Provisioned for an Edge that does not exist, andipextractionmethod: xffwith no proxy behind it is wrong while inert. Nowdirect, with a note to switch toxff+trustedproxieswhennpm/lands.vikunja_trusted_proxiesremoved fromgroup_vars.vikunja_accounts, password in the vault) — that is inherent to keeping the password a secret in the vault — but the README now documents the recipe, and the assert added last round catches a missing password with a clear message.Smells
not ansible_check_mode or vikunja_bin.stat.exists | default(false)copies are now onevikunja_installedfact, used by every guard and the handler.runuser -u … -- …blocks are onevikunja_cliprefix; each call appends its own subcommand.vikunja_release_binary/vikunja_release_zip.vikunja_pg's task renamed to say it checks PostgreSQL, not "the database".The group/host
vikunjacollision in the inventory is left deliberately: it is infra-forge's convention, Ansible only warns, and renaming either side risks two hosts sharing an address if the group and host diverge.A bug I introduced and caught
My first PostgreSQL assert shelled out to
psql --version. It passed on a direct run but failed the--checkrehearsal —command/shelltasks skip in check mode, so the registered variable never existed and the assert read''. That is precisely the gate run before an apply. Replaced with astatof/usr/lib/postgresql/17, which runs in check mode. I also found a stale release zip left in/tmpfrom that same gap (the cleanup was gated oninstall_needed) and made it unconditional.Verification
Check mode passes, then two real runs on vmid 142:
changed=1thenchanged=0,failed=0, nofatal, no deprecation./opt/vikunjaholds the single stable binary,/tmphas no leftover zip, and the config now carriesipextractionmethod: "direct". Every value in the vault is untouched.Code review — two axes (re-review, round 3)
Fixed point:
main(cf685d9). Diff:git diff main...HEAD(13 files, 554 insertions, 6 commits:9f4fca0,60c8f06,510e0a4,2bec6bb,a247388,710f668). This re-run checks the round-2 review (#issuecomment-651) against the fix commit710f668and the author's reply (#issuecomment-653).Standards sources:
README.md→## Conventionsonly (no CONTRIBUTING / CODING_STANDARDS / AGENTS.md / .ansible-lint / .yamllint). No issue-tracker file, so Spec reviews against the PR body + the six commit messages.Standards
Documented-standard violations: none. Both conventions hold — work is on a branch/PR (
mainuntouched atcf685d9), and no address is committed (only private10.12.0.142, already onmain;vikunja.thepit.spaceis an explicitly-allowed public domain).Baseline smells (all judgement calls):
vikunja_installedfact; the postgres path re-inlines the raw stat 3× —tasks/main.ymlL154, L165, L184:when: not ansible_check_mode or vikunja_pg.stat.exists | default(false). Round-2 smell #1 unifiedvikunja_binbut notvikunja_pg.become: true/become_user: postgres/become_method: ansible.builtin.surepeats verbatim on both the role and db tasks.confirm_wordre-implements the read/compare/abort block already insideconfirm_and_apply; the latter could call the former.vikunja_pgstill names a/usr/bin/psqlstat, andvikunja_installed(binary present) vsvikunja_release_installed(version string) read alike but mean different things.tasks/main.ymlis 306 lines; its six numbered sections could beinclude_tasks.config.yml.j2consumesvikunja_public_url/vikunja_domain, supplied only bygroup_vars; absent fromdefaults/main.yml, so the role is not self-contained.[vikunja]and hostvikunjashare a name (Ansible warns). The cited "infra-forge convention" is not a standard documented in this repo.Round-2 Standards findings — resolution:
vikunja_binguard — RESOLVED (vikunja_installedfact), but the analogousvikunja_pgguard remains 3×.runuser …argv — RESOLVED (vikunja_cliprefix indefaults/main.yml).vikunja_release_binary/vikunja_release_zip).version v(\d+\.\d+\.\d+)+ exact!= vikunja_version).vikunja_pgregister and the group/host collision remain).no_log: trueon the template task, L205); S2 host-key coercion — RESOLVED ([ssh_connection] ssh_args = -o StrictHostKeyChecking=accept-new).Spec
(a) Missing / partial: none. Every deliverable in the body is present;
npm/is explicitly deferred ("still to come").(b) Scope creep: none remaining. The added pieces (
.env.exampleANSIBLE_VAULT_PASSWORD,vault-passignore, amd64 assert, mailer) are each named in the body or commit messages.(c) Implemented but wrong:
--check"gate" exercises little. Commit60c8f06: "The check run stays the gate: it is what the word confirms against." Butcommand/shelltasks skip under--check(independently verified: a probecommandreportsskipped=True), so on the guestmake service-checkskipsmigrate(L265) and both account tasks (L280, L291) — the failure-prone half. The operator confirms a rehearsal that never rehearsed migrations or account creation. Minor corollary: since the account-list task also skips,item.rc | default(1)would mark every account "missing" — harmless only because the create task skips too.Round-2 Spec findings — resolution:
postgresql-{{ vikunja_db_major }}+ a check-mode-safestat /usr/lib/postgresql/17assert, L177-192); README:68's "PostgreSQL 17" is now enforced.xff+trustedproxiescreep — RESOLVED (ipextractionmethod: "direct", notrustedproxies/vikunja_trusted_proxiesleft in the tree; matches the upstream default).Summary — Standards: 0 documented-standard breaches, 7 judgement-call smells (4 of 5 prior smells resolved, 1 partial); worst = the residual
vikunja_pgguard duplication. Spec: 0 missing, 0 scope creep, 1 implemented-but-wrong (the thin--checkgate); all 3 round-2 Spec findings resolved. Both axes clean of hard/blocking issues.Merge recommendation: yes — merge. No blocking or significant findings remain on either axis; the open items are judgement-call smells and one non-blocking gap where the
--checkgate can't rehearse the migrate/account steps (worth a follow-up, not a hold).