Update Obsidian Vault backup from copy to in-place git repo; remove old config backups - #249
Conversation
WalkthroughAdds post-backup maintenance to Changes
Sequence DiagramsequenceDiagram
participant User
participant BackupTask as backup-vault-repo
participant Stop as stop-obsidian
participant Push as push-vault-repo
participant Start as start-obsidian
participant Obsidian as ObsidianService
participant Git as Git+pre-commit
User->>BackupTask: run backup-vault-repo
BackupTask->>Stop: invoke stop-obsidian
Stop->>Obsidian: stop service
BackupTask->>Push: invoke push-vault-repo
Push->>Obsidian: cd to VAULT_PATH (operate on vault)
Push->>Git: run pre-commit hooks
Git-->>Push: hooks complete (may modify files)
Push->>Push: git add (re-add after pre-commit), commit, push
Push-->>BackupTask: push complete
BackupTask->>Start: invoke start-obsidian (deferred)
Start->>Obsidian: start service
BackupTask-->>User: done
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Taskfile.yaml (1)
139-142: Verify backup creation before cleanup and clarify repository comment.The retention logic correctly keeps the 30 most recent backups. However, consider these points:
- The cleanup runs even if the tar command on line 138 fails, potentially removing old backups without creating a new one.
- The comment "they are still kept in the repository" is unclear since
../infra-config-backupappears to be outside the git repository.🔎 Consider adding error handling:
backup-config: desc: Create a compressed backup of the configuration directory cmds: - mkdir -p ../infra-config-backup - - cd config && tar -cjf "../infra-config-backup/infra-config-$(date +%Y-%m-%d_%H-%M-%S).tar.bz2" . + - | + cd config && tar -cjf "../infra-config-backup/infra-config-$(date +%Y-%m-%d_%H-%M-%S).tar.bz2" . || { echo "Backup creation failed"; exit 1; } - echo "Recent backups:" - ls -lh ../infra-config-backup/infra-config-*.tar.bz2 | tail -n 5 - # Remove old backups (they are still kept in the repository - see the push-config-repo task) + # Remove old backups (keeping 30 most recent) - ls -t ../infra-config-backup/infra-config-*.tar.bz2 | tail -n +31 | xargs -r rmdocker/tools/Taskfile.obsidian.yaml (1)
44-49: Service may remain stopped if backup fails.The orchestration correctly stops Obsidian before backing up to ensure data consistency. However, if
stop-obsidianorpush-vault-repofails, the service will remain stopped becausestart-obsidianwon't execute due to Task framework's default behavior of stopping on first error.Consider using deferred commands, which run after all other commands even if the task fails:
backup-vault-repo: desc: Stop Obsidian, commit and push changes to git, then start the service cmds: - task: stop-obsidian - defer: { task: start-obsidian } - task: push-vault-repoNote: The defer directive ensures deferred tasks are executed in last-in-first-out order. This is available in Task v3.38.0 and later.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Taskfile.yaml(1 hunks)docker/tools/Taskfile.obsidian.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
YAML files must pass YAML linting and validation
Files:
Taskfile.yamldocker/tools/Taskfile.obsidian.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (3)
docker/tools/Taskfile.obsidian.yaml (3)
7-9: Good documentation of the separate git-dir approach.The comments clearly explain why
.gitis placed outside the vault directory and provide a reference to the official documentation.
19-21: Pre-commit failures are silently ignored.The
|| trueon line 20 ensures the task continues even when pre-commit hooks fail. While this allows pre-commit to auto-fix files (captured by the secondgit add), it also means validation failures are silently ignored.Consider whether you want to:
- Keep this behavior to ensure backups always complete, or
- Remove
|| trueto enforce pre-commit validationThe current approach prioritizes backup completion over code quality checks. Verify this aligns with your requirements for the backup workflow.
34-42: Service lifecycle tasks look good.The
stop-obsidianandstart-obsidiantasks provide clean abstractions for service management. These integrate well with the updated backup workflow.
…ld config backups
7c3f0d6 to
22b5615
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Taskfile.yaml (2)
140-140: Handle the case when no backup files exist.The
lscommand will fail with an error if no matching backup files exist, which could happen on first run or if the backup directory is empty.🔎 Apply this diff to suppress errors when no files exist:
- - ls -lh ../infra-config-backup/infra-config-*.tar.bz2 | tail -n 5 + - ls -lh ../infra-config-backup/infra-config-*.tar.bz2 2>/dev/null | tail -n 5 || echo "No backups found."
142-142: Handle the case when no backup files exist.Similar to line 140, the
lscommand will fail with an error if no matching backup files exist. Whilexargs -rpreventsrmfrom running on empty input (good!), thelsitself will still output an error message.🔎 Apply this diff to suppress errors when no files exist:
- - ls -t ../infra-config-backup/infra-config-*.tar.bz2 | tail -n +31 | xargs -r rm + - ls -t ../infra-config-backup/infra-config-*.tar.bz2 2>/dev/null | tail -n +31 | xargs -r rmdocker/tools/Taskfile.obsidian.yaml (1)
16-21: LGTM! Pre-commit integration is correctly implemented.The addition of
pre-commit runwith a secondgit add --allis the correct pattern, since pre-commit hooks may auto-fix files that need to be re-staged. The|| trueappropriately allows the workflow to continue even if pre-commit reports issues.Minor consistency note: The trailing slash in
VAULT_PATHconstruction (line 16) differs from the style used inpush-config-repotask in Taskfile.yaml, which usesCONFIG_PATH="config"without a trailing slash. Consider removing the trailing slash for consistency:- VAULT_PATH="${STORAGE_OBSIDIAN}/${OBSIDIAN_VAULT_NAME}/" + VAULT_PATH="${STORAGE_OBSIDIAN}/${OBSIDIAN_VAULT_NAME}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Taskfile.yaml(1 hunks)docker/tools/Taskfile.obsidian.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
YAML files must pass YAML linting and validation
Files:
Taskfile.yamldocker/tools/Taskfile.obsidian.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (3)
docker/tools/Taskfile.obsidian.yaml (3)
7-9: LGTM!The updated comment clearly explains the rationale for keeping the
.gitfolder outside the vault and provides a helpful reference to the Git documentation.
34-42: LGTM!The new service control tasks provide clear lifecycle management for the Obsidian service. The task descriptions are clear and the implementation is straightforward.
44-49: LGTM! Excellent use of defer for resilient service management.The orchestration correctly stops Obsidian before committing changes and uses
deferto ensure the service is restarted even if the push operation fails. This pattern provides good resilience and prevents leaving the service in a stopped state.
…ld config backups
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.