fix(ansible): skip missing user hooks source - #147
Merged
Conversation
There was a problem hiding this comment.
Review: fix(ansible): skip missing user hooks source
Strengths
- Correct Ansible pattern: Using
ansible.builtin.stat+register+when: <reg>.stat.existsis the idiomatic way to guard against missing source paths in Ansible — well applied here. - Least-privilege maintained: Both the stat task and the sync task carry
become: falseanddelegate_to: localhost, consistent with the existing task posture. - Test coverage matches the change: The test updates verify all three meaningful properties of the new stat task (path, delegate_to, become) and confirm the
whenguard was wired onto the sync task. - Minor DRY improvement in tests: Caching
_tasks()in a local variable (tasks) avoids parsing the YAML twice intest_playbook_syncs_user_hooks_with_delete— small but correct.
Issues
Critical (Must Fix)
None.
Important (Should Fix)
test_playbook_syncs_user_hooks_with_delete now tests two unrelated things
tests/test_sync_configs_playbook_hooks.py:27–38- The function was originally scoped to the sync-with-delete behaviour. It now also asserts properties of the stat task. If either concern breaks, the failure message points at the wrong test name, making diagnosis slower.
- Fix: Extract the stat assertions into a dedicated
test_playbook_checks_user_hooks_source_before_syncfunction.
Minor (Nice to Have)
when condition stored as a plain string in YAML
ansible/playbooks/sync_instance_configs_and_restart.yml:90when: user_hooks_source.stat.existsis correct and works, but Ansible also accepts the boolean expression formwhen: user_hooks_source.stat.exists | boolwhich makes intent more explicit in newer lint rules. Not a bug; lint-level only.
Assessment
Ready to merge? Yes
Reasoning: This is a focused, low-risk guard against a missing source directory that previously caused an unconditional rsync failure. The logic is correct, the tests cover the new behaviour, and there are no security or compatibility regressions.
added 2 commits
July 11, 2026 00:44
Review on PR #147 noted test_playbook_syncs_user_hooks_with_delete asserted two unrelated tasks, so a failure would point at the wrong test name.
dngrtech
force-pushed
the
fix/skip-missing-user-hooks-sync
branch
from
July 11, 2026 07:44
74a5dd4 to
9998f2b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
user-hooks/directory exists before synchronizinguser-hooks/creation and normal sync/delete behavior when a source existsRoot cause
Legacy instances may not have a local
user-hooks/directory. The unconditionalsynchronizetask then exits with rsync code 23, causing Ansible RC 2 and incorrectly leaving an otherwise-running instance inERROR.Test plan
pytest -q tests/test_sync_configs_playbook_hooks.py tests/test_service_enablement_playbooks.py(4 passed)ansible-playbook --syntax-check ansible/playbooks/sync_instance_configs_and_restart.yml -i "localhost,"SECRET_KEYmissing, Redis auth, existing script-validation failure)