Make hosted forge repo writable for merges#736
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new canonical git directory variable and pre-start setup script were introduced to the pika-news systemd service. The script ensures proper ownership and permissions on the git repository before service startup, configuring git settings via command-line flags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
| Group = serviceGroup; | ||
| WorkingDirectory = serviceStateDir; | ||
| EnvironmentFile = [ config.sops.templates."pika-news-env".path ]; | ||
| ExecStartPre = [ prepareCanonicalRepo ]; |
There was a problem hiding this comment.
🔴 ExecStartPre script runs chown as unprivileged user, will fail and block service startup
The prepareCanonicalRepo script calls chown -R ${serviceUser}:${serviceGroup} "$repo" (line 18), but ExecStartPre runs under the same User/Group as the main service (pika-news). On Linux, only root (or a process with CAP_CHOWN) can execute chown. Since the script uses set -euo pipefail, the chown failure causes the script to exit non-zero, which prevents the service from starting whenever the repo directory exists.
The fix is to prefix the command with + so systemd runs it with full root privileges before dropping to the service user: ExecStartPre = [ "+${prepareCanonicalRepo}" ];.
| ExecStartPre = [ prepareCanonicalRepo ]; | |
| ExecStartPre = [ "+${prepareCanonicalRepo}" ]; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
pika-newsowns it and can write new git objectsWhy
Manual
phdogfooding found thatph closeworked butph mergeconsistently failed. The hosted service user could delete branch refs, but it could not write new objects under/var/lib/pika-news/pika.git/objects, so forge merges failed before the ref transaction.Verification
Summary by CodeRabbit