Add --from-snapshot flag to create droplets from snapshots#53
Add --from-snapshot flag to create droplets from snapshots#53gwpl wants to merge 5 commits intotrailofbits:mainfrom
Conversation
ret2libc
left a comment
There was a problem hiding this comment.
Is there any way to consolidate this with the existing "wake" code that does something very similar?
|
Always eager, Greg's AI Assistant reporting in! 🧬 Good call — there's definitely overlap between The key differences that kept us from directly reusing
That said, we could absolutely extract the shared plumbing (create-from-snapshot → wait-for-active → SSH-config → optional-Tailscale) into a helper function that both Want us to refactor it that way in this PR, or would you prefer to land the feature first and consolidate in a follow-up? |
|
Greg asked us to go ahead and refactor to consolidate the shared plumbing between |
|
Always eager, Greg's AI Assistant back with the refactor! 🔧 TL;DRExtracted What the helper does
Returns What remains command-specific
All 288 tests pass, ruff/ty clean. No behavioral changes. Does this align with what you had in mind @ret2libc? |
|
AI Assistant: Friendly ping! 👋 The refactor extracting |
Support creating droplets from existing snapshots without cloud-init. This enables the "golden image" workflow: configure one droplet, snapshot it, then spin up N identical clones. When --from-snapshot <id> is used: - Uses create_droplet_from_snapshot() API (no user_data sent) - Skips cloud-init rendering and completion monitoring - Still performs: wait for active, SSH config, project assignment, optional Tailscale setup Cloud-init is skipped because the current template is not idempotent — re-running it on a snapshot causes user creation failures, .zshrc overwrites, and an unconditional reboot. See issue trailofbits#52 for discussion of alternative approaches. (for Github WebUI issue linking: Closes trailofbits#52 ) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…code Both `create --from-snapshot` and `wake` perform the same post-creation plumbing: API call, wait-for-active, IP extraction, SSH config, and optional Tailscale setup. Extract this into a shared helper to eliminate ~90 lines of duplication. * New `_create_from_snapshot_and_setup()` near other helper functions * `create --from-snapshot` delegates to helper, then handles project assignment and summary messages * `wake` delegates to helper with tailscale_enabled=False, then handles its own Tailscale re-setup logic (10s sleep, was_tailscale_locked tag) and snapshot deletion prompt No behavioral changes -- both commands produce identical output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d52751e to
092a788
Compare
ret2libc
left a comment
There was a problem hiding this comment.
Thanks for the patience on the review! I think I'd like to see less duplicated code, because we have a lot of different paths and ideally the differences would mainly be in the UI rather than in the logic of how to turn on/off/wake/create instances.
Replace _create_from_snapshot_and_setup with a more generic _post_create_setup helper that handles post-creation steps (validate ID, wait for active, extract IP, SSH config, optional Tailscale) for ALL three droplet creation paths: - create (base image): API call → _post_create_setup → cloud-init → Tailscale - create --from-snapshot: API call → _post_create_setup (with Tailscale) - wake: API call → _post_create_setup → Tailscale (separate, conditional) Also consolidates project assignment (was duplicated in both create paths). The API call itself stays in each command function (it's the only truly different part), while the shared plumbing lives in one place. Addresses ret2libc's review: "I'd like to see less duplicated code, because we have a lot of different paths and ideally the differences would mainly be in the UI rather than in the logic." Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
AI Assistant consulting with Greg — we're like Refactoring done (7e461d9)Took your architectural direction to heart. The key change: replaced the snapshot-only Before: Each path had its own copy of wait-for-active → IP extraction → SSH config → Tailscale (100+ duplicated lines). After: Each command function only handles what's unique to it:
Project assignment was also consolidated — it was duplicated between the two Result: -75 net lines (59 added, 134 removed). All 288 tests pass. This aligns with the vision of "differences mainly in the UI rather than in the logic" — each command gathers its parameters and makes its API call, then the shared helper does the mechanical plumbing. |
Per ret2libc's feedback: Tailscale setup timing is a UI concern that differs per creation path, so it shouldn't live inside the shared helper. Moved setup_tailscale() calls into each branch explicitly: - Snapshot branch in create: runs Tailscale immediately after helper (no cloud-init to wait for) - Base image branch in create: runs Tailscale after cloud-init completes (unchanged behavior, but now removed dead tailscale_enabled=False arg) - wake: runs Tailscale separately with was_tailscale_locked check (already did) The helper is now purely mechanical plumbing: validate ID → wait active → IP extraction → SSH config. Returns (active_droplet, ip_address). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Per ret2libc's feedback: - Pull setup_tailscale out of both if/else branches into a single call after the if/else. The unified condition is cloud_init_done and ip_address (snapshot path sets cloud_init_done=True unconditionally, base-image path only when cloud-init succeeded). - Trim unnecessary docstring paragraph explaining why Tailscale is not in _post_create_setup — the bullet list is sufficient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
AI Assistant consulting with Greg — checking in after the refactor-ratchet cooled down! 🛠️ Friendly ping: the latest round of feedback from Apr 17 is addressed (trimmed docstring, unified Let us know if there's anything else to tidy up, or if it's ready to land! 🚀 |
ret2libc
left a comment
There was a problem hiding this comment.
3. Run the manual tests / add at least minimal automated coverage
The three manual checkboxes in the PR description are still unchecked, and there's no automated coverage for the new code path. At minimum, please run the listed manual tests before merge. A few cheap pytest cases would be even better
— happy with mocks here, e.g.:
- --from-snapshot + --image exits with code 1 and the expected message
- --from-snapshot calls create_droplet_from_snapshot and does not call render_cloud_init / create_droplet
- --from-snapshot + --no-tailscale does not call setup_tailscale
|
|
||
| # Get image (interactive if not provided) | ||
| if image is None: | ||
| # --from-snapshot and --image are mutually exclusive |
There was a problem hiding this comment.
- Move the mutual-exclusion check above the interactive prompts
Right now the --from-snapshot / --image conflict is detected after the region and size prompts run. A user passing both flags gets prompted for region and size before being told their flags conflict.
# Current location (after the size prompt):
if from_snapshot is not None and image is not None:
console.print("[red]Error: --from-snapshot and --image are mutually exclusive[/red]")
raise typer.Exit(1)
Hoist this check up to immediately after argument resolution (before the region/size/image prompts) so flag-conflict errors fire instantly.
| name=name, | ||
| region=region, | ||
| size=size, | ||
| image=image or "", |
There was a problem hiding this comment.
- Replace image or "" with proper type narrowing
droplet = api.create_droplet(
...
image=image or "",
...
)
The guard above already enforces image is not None on this branch (from_snapshot is None and image is None → exit). Sending "" to the DO API would round-trip to a less actionable error. Either:
assert image is not None # narrowed by the guard above
or restructure the guard so ty narrows the type. Let's keep the failure mode loud and local instead of papering over it.
Summary
AI Agent with Greg: We wanted to clone a perfectly configured droplet 10 times — like a VM templating system, but with
dropkitergonomics. Turns outdropkit createalways injects cloud-init, which on a snapshot triggers user creation failures,.zshrccarpet-bombing, and an unconditional reboot. Now there's a--from-snapshotflag that knows when to leave well enough alone. 🧬Closes #52
--from-snapshot <snapshot-id>flag todropkit createcreate_droplet_from_snapshot()(nouser_data), skips cloud-init rendering and monitoring--image(clear error if both provided)Usage: clone workflow
Why cloud-init must be skipped
The current cloud-init template is not idempotent — re-running on a snapshot causes:
users:directive fails (user already exists)write_files:overwrites.zshrc(loses customizations)runcmd:ends with unconditionalrebootDigitalOcean assigns a new droplet ID to snapshot-based instances, so cloud-init detects a "first boot" and re-runs everything. The
--from-snapshotflag avoids this by not sendinguser_dataat all.See #52 for discussion of alternative approaches (idempotent template,
--no-cloud-initflag, dedicatedclonecommand).Test plan
uv run ruff check dropkit/main.py— passeduv run ruff format --check dropkit/main.py— passeduv run ty check dropkit/main.py— passeduv run pytest tests/ -v— 288 passed, 31% coverage (above 29% minimum)dropkit create test --from-snapshot <id>— verify droplet created, SSH config added, no cloud-initdropkit create test --from-snapshot <id> --image ubuntu-25-10-x64— verify mutual exclusion errordropkit create test(no flag) — verify normal cloud-init flow unchanged🤖 Generated with Claude Code — your AI that believes in snapshot discipline: clone responsibly, cloud-init sparingly
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com