Add backup CLI, docs, and safer restore ordering#257
Add backup CLI, docs, and safer restore ordering#257osama1998H wants to merge 3 commits intofananimi:masterfrom
Conversation
- Introduced `pyzk-backup` as a console entry point for backup and restore operations, replacing the legacy `test_backup_restore.py` script. - Updated README.md to reflect new CLI usage and removed outdated script documentation. - Added comprehensive documentation for backup and restore processes in `docs/backup_restore.rst`. - Implemented backup and restore logic in `zk/backup.py` with JSON schema validation. - Created unit tests for backup and restore functionalities in `tests/test_backup_module.py`. - Enhanced device compatibility documentation in `docs/compatible_devices.rst`. - Added troubleshooting guide in `docs/troubleshooting.rst` for common issues. - Established a roadmap for future development in `ROADMAP.md`. - Included API design specifications in `docs/api_design.rst` for planned HTTP and real-time APIs. - Created agent guides in `AGENTS.md` and `CLAUDE.md` for better onboarding and usage instructions.
Refactor backup and restore functionality into a dedicated CLI tool
There was a problem hiding this comment.
Pull request overview
This PR adds a production-ready backup and restore CLI tool for ZK devices, along with comprehensive documentation improvements. The implementation properly orders the restore flow to validate backup data before performing destructive operations, addressing a critical safety concern. The changes include a new pyzk-backup console command, core backup/restore modules, unit tests, and extensive Sphinx documentation covering usage, troubleshooting, device compatibility, and future API design.
Changes:
- Added
pyzk-backupCLI tool with backup/restore functionality, replacing the legacy test script - Implemented safer restore ordering by validating backup data before device erasure
- Expanded documentation with backup/restore guides, troubleshooting, compatible devices list, and API design spec
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| zk/cli_backup.py | New CLI entry point for backup/restore operations with argparse interface |
| zk/backup.py | Core backup/restore module with export, save, load, and restore functions |
| tests/test_backup_module.py | Unit tests for Finger serialization and restore_backup function |
| test_backup_restore.py | Legacy script refactored to wrap the new CLI for backward compatibility |
| setup.py | Added console_scripts entry point for pyzk-backup command |
| docs/backup_restore.rst | New documentation for CLI and programmatic backup/restore usage |
| docs/troubleshooting.rst | New troubleshooting guide for common device connection and restore issues |
| docs/api_design.rst | Forward-looking spec for HTTP REST API (design only, no implementation) |
| docs/compatible_devices.rst | Moved device compatibility list from README to dedicated doc with submission template |
| docs/topic1.rst | Enhanced connection documentation with code examples and TCP/UDP guidance |
| docs/topic2.rst | Expanded user operations documentation with complete code examples |
| docs/topic3.rst | Added attendance log documentation with live capture examples |
| docs/topic4.rst | Added device maintenance examples (set time, poweroff, restart, clear data, unlock) |
| docs/topic5.rst | Improved IOT example with door lock implementation |
| docs/index.rst | Updated table of contents with new documentation sections |
| README.md | Updated backup/restore section to reference new CLI and documentation |
| ROADMAP.md | New roadmap tracking current and future development plans |
| CLAUDE.md | New agent guide for AI assistants (repository overview and conventions) |
| AGENTS.md | Duplicate of CLAUDE.md for alternative agent systems |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| return input(prompt) | ||
| except NameError: | ||
| return raw_input(prompt) |
There was a problem hiding this comment.
The _prompt_input function uses a try/except to handle Python 2's raw_input vs Python 3's input. However, it catches the generic NameError which could hide other issues. Consider being more specific by checking for raw_input explicitly or documenting that this is intentional for Python 2/3 compatibility.
| try: | |
| return input(prompt) | |
| except NameError: | |
| return raw_input(prompt) | |
| # Use Python version check instead of catching NameError to select | |
| # the appropriate input function, avoiding masking other NameError issues. | |
| if sys.version_info[0] < 3: | |
| return raw_input(prompt) | |
| return input(prompt) |
| def test_fp_version_mismatch(self): | ||
| conn = FakeConn(fp_version='9', serial='ABC') | ||
| data = self._sample_data() | ||
| with self.assertRaises(ZKErrorResponse): | ||
| restore_backup(conn, data, erase=False, high_rate=False, prompt_serial=False) |
There was a problem hiding this comment.
The test_fp_version_mismatch test verifies that restore fails with mismatched fingerprint versions, but it doesn't check that the device wasn't modified before the error. Consider adding assertions to verify that FakeConn.disabled and FakeConn.cleared remain False when the error is raised.
| Template downloads can be large. Increase ``timeout`` and avoid Wi-Fi if possible. | ||
|
|
||
| Restore fails with template version mismatch | ||
| ------------------------------------------- |
There was a problem hiding this comment.
Inconsistent formatting: The section header "Restore fails with template version mismatch" (line 31-32) uses dashes instead of equals signs, while other top-level sections use dashes. However, according to reStructuredText conventions, the number of underline characters should match the title length. The current line 32 has 44 dashes but the title on line 31 has 45 characters.
| ------------------------------------------- | |
| -------------------------------------------- |
| # Codex Agent Guide (pyzk) | ||
|
|
||
| ## Repo overview | ||
| - `zk/` is the library package. The main entry point is `ZK` in `zk/base.py`. | ||
| - `example/` contains minimal usage scripts. | ||
| - `test_machine.py` and `test_backup_restore.py` are hardware-facing CLIs. | ||
| - `docs/` is a Sphinx site (ReadTheDocs compatible). | ||
|
|
||
| ## Key modules | ||
| - `zk/base.py`: core protocol, connection handling, and device operations. | ||
| - `zk/user.py`: `User` model and packing helpers. | ||
| - `zk/finger.py`: `Finger` model and template packing helpers. | ||
| - `zk/attendance.py`: `Attendance` model. | ||
| - `zk/const.py`: protocol constants and flags. | ||
| - `zk/exception.py`: library exceptions. | ||
|
|
||
| ## Safety notes | ||
| - Destructive calls include `clear_data()`, `clear_attendance()`, `poweroff()`, and `restart()`. | ||
| - `unlock()` opens the door/relay on some devices. Treat it as a safety-sensitive operation. | ||
| - Prefer `disable_device()` before bulk reads/writes to avoid inconsistent data. | ||
| - Live capture (`live_capture()`) can hold device state; ensure you exit cleanly. | ||
|
|
||
| ## Common commands | ||
| - Run an example: `python example/get_users.py` (edit IP/port inside the script). | ||
| - Basic device probe: `python test_machine.py -a 192.168.1.201`. | ||
| - Backup/restore CLI (new): `pyzk-backup --help`. | ||
|
|
||
| ## Docs build | ||
| - Build Sphinx docs: `cd docs && make html`. | ||
| - Or: `sphinx-build -b html docs docs/_build/html`. | ||
|
|
||
| ## Hardware-dependent tests | ||
| - Any command that connects to a real device requires a reachable IP/port (default 4370). | ||
| - Use `--force-udp` if TCP is unreliable for a specific model. | ||
| - Time sync and firmware reads may differ by model/firmware version. | ||
|
|
||
| ## Conventions | ||
| - Keep new code Python 2/3 compatible unless explicitly dropping support. | ||
| - Avoid changing protocol details without referencing `docs/_static/Communication_protocol_manual_CMD.pdf`. |
There was a problem hiding this comment.
CLAUDE.md and AGENTS.md have identical content. Consider consolidating these into a single file or clearly documenting why both are needed. Having duplicate files can lead to maintenance issues where one is updated but not the other.
| def erase_device(conn, clear_attendance=False, prompt_serial=True): | ||
| serial = conn.get_serialnumber() | ||
| if prompt_serial and not _confirm_serial(serial): | ||
| raise ZKErrorResponse('Serial number mismatch') | ||
| conn.disable_device() | ||
| conn.clear_data() | ||
| if clear_attendance: | ||
| conn.clear_attendance() | ||
| return True | ||
|
|
There was a problem hiding this comment.
Missing test coverage: The backup module's erase_device function is not tested. Consider adding tests to verify that erase_device correctly calls disable_device, clear_data, and optionally clear_attendance, and that it validates the serial number when prompt_serial=True.
| 'users': [u.__dict__ for u in users], | ||
| 'templates': [t.json_pack() for t in templates] |
There was a problem hiding this comment.
Security consideration: The backup file contains user passwords in plaintext (line 55 in backup.py uses u.dict which includes the password field). Consider documenting this security risk in the backup_restore.rst documentation and recommending that backup files be stored securely.
| _validate_data(data) | ||
| fp_version = conn.get_fp_version() | ||
| if data['fp_version'] != fp_version: | ||
| raise ZKErrorResponse('Fingerprint version mismatch {} != {}'.format(fp_version, data['fp_version'])) | ||
|
|
||
| users = [User.json_unpack(u) for u in data['users']] | ||
| templates = [Finger.json_unpack(t) for t in data['templates']] | ||
|
|
||
| by_uid = {} | ||
| for t in templates: | ||
| by_uid.setdefault(t.uid, []).append(t) | ||
|
|
||
| if erase: | ||
| erase_device(conn, clear_attendance=clear_attendance, prompt_serial=prompt_serial) |
There was a problem hiding this comment.
The restore_backup function parses users and templates from JSON (lines 109-110) before the erase operation (line 117), but after the fp_version check (line 106). If json_unpack() raises an exception due to malformed data, the device state will be unchanged. However, consider also validating the structure of users and templates lists before the erase to catch issues like non-list types or missing required fields.
| def _print(msg): | ||
| try: | ||
| sys.stdout.write(msg + '\n') | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | ||
| pass | ||
| try: | ||
| conn.disconnect() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| try: | |
| conn.disconnect() | |
| except Exception: | |
| except Exception: | |
| # Ignore errors while re-enabling the device during cleanup. | |
| pass | |
| try: | |
| conn.disconnect() | |
| except Exception: | |
| # Ignore disconnect errors during cleanup; process is exiting anyway. |
| except Exception: | ||
| pass | ||
| try: | ||
| conn.disconnect() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| try: | |
| conn.disconnect() | |
| except Exception: | |
| except Exception: | |
| # Intentionally ignore errors while enabling the device during cleanup. | |
| pass | |
| try: | |
| conn.disconnect() | |
| except Exception: | |
| # Intentionally ignore errors while disconnecting during cleanup. |
Summary
pyzk-backupCLI and backup/restore moduleTesting