feat: --dry-run CLI flag (authenticate, summarise, exit without writing)#459
Open
epheterson wants to merge 8 commits into
Open
feat: --dry-run CLI flag (authenticate, summarise, exit without writing)#459epheterson wants to merge 8 commits into
epheterson wants to merge 8 commits into
Conversation
Restores the unwired ``--dry-run`` flag documented in
NOTIFICATION_CONFIG.md, and extends it to a full pre-flight check:
authenticate against iCloud, summarise what the real loop would do
for each configured service, then exit cleanly without downloading,
deleting, or notifying.
## Motivation
Today there is no safe way to verify an icloud-docker install before
letting it loose on a fresh destination. A typo in the bind-mount
path or a misconfigured Apple ID currently means terabytes of iCloud
data get dumped into the wrong place before the user notices.
``python src/main.py --dry-run`` (or ``docker exec icloud icloud
--dry-run`` once the entrypoint forwards the flag) now does the
auth + enumeration once, prints a summary, and exits.
## Implementation
- ``src/main.py``: argparse wrapper, calls ``sync.sync(dry_run=args.dry_run)``.
- ``src/sync.py``: ``sync()`` gains a ``dry_run: bool = False`` kwarg
(default keeps existing behaviour). When True, the loop branches to
the new ``_perform_dry_run`` helper after auth and ``return``s
instead of entering the retry/sleep cycle.
- ``_perform_dry_run`` logs (INFO):
- Drive destination path + root-level item count, or "would be
skipped" if ``drive:`` is absent.
- Photos destination path + library names, or "would be skipped"
if ``photos:`` is absent.
- A trailing "DRY RUN complete — no files were written" line.
- 2FA-pending branch logs a hint to finish interactive auth first.
Enumeration failures are caught + logged as warnings — dry-run never
crashes the container.
## Tests
11 new tests in ``tests/test_dry_run.py``:
- 6 ``_perform_dry_run`` behaviour tests (drive count, photo libs,
per-service enumeration failure, completion line, skipped-service
announcement).
- 4 integration tests on ``sync.sync(dry_run=True)`` via mocks
(syncs not invoked, notifications not sent, loop not entered,
2FA-pending branch).
- 1 signature compat test (default kwarg).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_mismatch/not_found per library
Builds on top of PR 7 (dry-run). The shallow --dry-run answers
"can mandarons reach iCloud + see my libraries" but says nothing
about whether a real sync would re-download files a previous tool
already wrote. That's the question users migrating from
boredazfcuk (and any other downloader) actually have, and it's the
one that turns into hours of re-download if mandarons gets the
filename_format / folder_format / library_destinations wrong.
New CLI flag (only meaningful with --dry-run):
--check-files N # walk N photos per library (0 = all)
For each library, --dry-run --check-files=N reports:
would_skip file exists at the path mandarons would write,
and size matches iCloud's reported original size
→ real sync would skip it (success criterion).
size_mismatch file exists, different size → real sync would
re-download (often Apple-side edit; sometimes a
filename_format mismatch).
not_found target path empty → real sync would download as
new. Expected for genuinely-new photos; concerning
if the user has matching files via another path.
errors couldn't compute path / stat the file. Counted
separately so a sampling run doesn't silently
pretend everything's fine.
Up to three sample paths per status are emitted so the user can
verify "the path mandarons computed is the one I expected."
src/migration_check.py is a new module. The walking is sequential
(newest-first, matches icloudpy's iterator default). Users who want
to validate older photos pass a larger N — iCloud has no per-year
album, so there's no cheap shortcut to historical photos.
5 new tests in tests/test_migration_check.py covering: empty
library, all-present-at-correct-size, all-missing, mixed
(skip/mismatch/new with counts), sample=N caps the walk. Mocks
icloudpy PhotoAsset so no real network. All 5 pass alongside the
existing 11 dry-run tests.
Validated against a live install with 111K-photo shared library
and 30K-photo primary library: confirmed the new code path
authenticates, walks, computes paths, and reports correctly.
…nly) Closes the dry-run scope gap surfaced by Eric: --check-files only validated Photos, leaving drive.destination misconfiguration able to silently trigger a full Drive re-download on real sync. migration_check.py: - _check_one_drive_file: tri-state per Drive item with size match. Handles BOTH flat-file (most items, plus packages mandarons couldn't unpack — .key/.jmb/etc) and directory packages (.band etc). - _walk_drive_recursive: depth-first walk with shared sample cap. - check_drive / check_drive_migration: entry points matching the shape of check_library / check_migration so the sync.py dry-run path can treat both uniformly. sync.py _perform_dry_run: - Photos block unchanged. - New Drive block reports the same would_skip / size_mismatch / not_found / errors per Drive walk. Tests: 6 new TestCheckDrive cases (empty, all-match, mixed missing+ mismatch, recursive, unpacked-package-as-directory, sample cap). Existing 5 TestCheckLibrary cases unchanged. 11/11 pass. NOTE FOR PR SUBMISSION: this commit is on combined/all-features but the source-of-truth branch for upstream PR 7 is feat/dry-run — rebase before force-pushing the PR. Co-Authored-By: Claude <noreply@anthropic.com>
SLF001 (private member access) on test_dry_run.py tests that call sync._perform_dry_run — legitimate test access to a private function. EM101 on the in-test fake-API RuntimeError — string literal is fine in a test fixture. Both marked inline with # noqa per project convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI gates at 100% coverage; this PR was at 95.16% on initial submission. The gap was three areas — none of them low-effort: 1. **migration_check.py** orchestrator paths (`check_migration`, `check_drive_migration`) weren't exercised at all because the existing TestCheckLibrary class was `@unittest.skipUnless`'d on feat/photos-filename-format-simple being merged upstream. Replaced the skip with an in-test monkey-patch of the metadata-format filename generator so the tests run standalone against the bare metadata-format default. Added TestCheckOnePhotoEdges, TestCheckOneDriveFileEdges, TestWalkDriveRecursiveEdges, TestWalkDriveRecursiveExtraEdges, TestCheckDriveExceptionPath, TestCheckMigrationLibraryDestinationsBranch, TestCheckMigrationOrchestrator — covering the per-photo error branches, the per-drive-file error branches, the walker recursion + unquote-fallback + early-cap, the check_drive exception path, and the orchestrator's filename-format singleton side effect. 2. **sync.py `_perform_dry_run` `--check-files` integration** (lines 399–468 on origin) was uncovered. Added TestDryRunCheckFilesIntegration with five tests covering: happy path with both walkers, check_files=0 (`all`) log variant, photos walk exception → warning, drive walk exception → warning, check_files=None → walkers not invoked. 3. **migration_check.py** also had a hard call to `config_parser.get_photos_library_destinations` from PR 3 — added a `hasattr`-gated fallback so this PR is genuinely independent of PR 3 instead of silently importing-and-crashing. Verified locally in a python:3.10 docker container (mirroring CI): 482 passed, ruff clean, 100.00% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a CLI dry-run/preflight path for iCloud Docker, including authentication, service enumeration, and optional migration-style file existence checks before running the normal sync loop.
Changes:
- Adds argparse handling for
--dry-runand--check-files. - Threads dry-run behavior through
sync.sync()with a new_perform_dry_run()helper. - Introduces
migration_check.pyplus tests for photo/drive existence checking.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/main.py |
Adds CLI parsing for dry-run and file-check options. |
src/sync.py |
Adds dry-run execution path and logging/reporting logic. |
src/migration_check.py |
Adds photo and Drive migration/existence checking helpers. |
tests/test_dry_run.py |
Adds tests for dry-run behavior and sync short-circuiting. |
tests/test_migration_check.py |
Adds tests for migration/file checking logic. |
…contract Four substantive bugs from Copilot's review: 1. **src/main.py — CLI validation.** ``--check-files N`` was silently ignored when ``--dry-run`` wasn't set (started the normal sync loop instead), and negative values were treated like "walk everything" by the migration walkers (since the cap-check is ``if sample > 0``). Fail fast on both invalid combinations via argparse. 2. **src/migration_check.py — don't side-effect mkdir during dry-run.** Both ``check_migration`` and ``check_drive_migration`` were calling ``prepare_*_destination(config=config)`` which is a mutating helper that creates the destination directory tree on disk. That's exactly what ``--dry-run`` is supposed to catch BEFORE writing — a typo in the mount path would leave stub dirs on the host. Switched to the non-mutating ``get_root_destination_path`` + ``get_*_destination_path`` getters joined manually. 3. **src/sync.py — skip ``alive()`` on dry-run.** ``alive(config=config)`` registers the installation, sends a heartbeat, and persists the usage cache to disk. All three violate the "no side effects, no telemetry" contract of ``--dry-run``. Gated behind ``if not dry_run``. 4. **src/migration_check.py — walk the same album path the real sync uses.** ``_sync_all_photos_in_library`` in src/sync_photos.py iterates ``photos.libraries[library].all`` and writes under ``<library_dest>/all/<folder>/<filename>``. The checker was walking ``library.albums["All Photos"]`` and computing target paths under ``<library_dest>/<filename>`` directly — missing the ``/all/`` segment. Result: ``--dry-run --check-files`` reported existing default-sync files as ``not_found``, defeating the whole purpose of the migration pre-flight. Switched to ``library.all`` + appended ``/all`` to ``library_dest`` for the on-disk compare. Tests updated: - ``_fake_library`` now exposes ``.all`` alongside ``.albums``. - Photo-fixture tests write files under ``base/all/`` to match. - ``test_check_drive_migration_returns_none_when_destination_resolution_fails`` patches ``get_root_destination_path`` (the new non-mutating getter). - ``BoomLibrary`` test exposes a ``.all`` iterable. Verified on python:3.10 docker mirroring CI: ruff clean, 482/482 pass, 100.00% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… mode Per Copilot review feedback on the dry-run PR: ``NOTIFICATION_CONFIG.md`` already documented ``--dry-run`` as a way to test notification delivery, but on upstream main there was no such flag (the docs were aspirational). This PR adds ``--dry-run`` but its contract is the opposite of what the doc described — it suppresses notifications and walks no files. Updated the "Dry Run Mode" section to reflect the actual behaviour and point readers to the manual ``notify._send_*_no_throttle`` helpers for notification testing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Restores the unwired
--dry-runflag documented inNOTIFICATION_CONFIG.md, and extends it to a full pre-flight check:authenticate against iCloud, summarise what the real loop would do
for each configured service, then exit cleanly without downloading,
deleting, or notifying.
Motivation
Today there is no safe way to verify an icloud-docker install before
letting it loose on a fresh destination. A typo in the bind-mount
path or a misconfigured Apple ID currently means terabytes of iCloud
data get dumped into the wrong place before the user notices.
python src/main.py --dry-run(ordocker exec icloud icloud --dry-runonce the entrypoint forwards the flag) now does theauth + enumeration once, prints a summary, and exits.
Implementation
src/main.py: argparse wrapper, callssync.sync(dry_run=args.dry_run).src/sync.py:sync()gains adry_run: bool = Falsekwarg(default keeps existing behaviour). When True, the loop branches to
the new
_perform_dry_runhelper after auth andreturns insteadof entering the retry/sleep cycle.
_perform_dry_runlogs (INFO):skipped" if
drive:is absent.if
photos:is absent.Enumeration failures are caught + logged as warnings — dry-run never
crashes the container.
Tests
11 new tests in
tests/test_dry_run.py:_perform_dry_runbehaviour tests (drive count, photo libs,per-service enumeration failure, completion line, skipped-service
announcement).
sync.sync(dry_run=True)via mocks (syncsnot invoked, notifications not sent, loop not entered,
2FA-pending branch).
Full suite passes.