Show a helpful error when the plan does not include snapshots - #425
Conversation
9151984 to
74e77d7
Compare
skyrpex
left a comment
There was a problem hiding this comment.
I'm halfway reviewing the PR, good job! I have some concerns though:
- The detection used to know whether sandboxes are enabled or not is very brittle, as both share a response with a 404 status code and the only difference is a response with body text or not.
I wonder if we should, on top of your work, request a better mechanism from the LocalStack backends to detect whether features are enabled or not.
Good thinking @skyrpex 🙏🏼 The proper response should come from the emulator backend, I'm going to follow up on that with snapshot feature owners. Even if we fix this behaviour on the emulator side, we would still want to rely on this rule for compatibility for users stuck using an old emulator version. I think this is still an improvement. What do you think? |
There was a problem hiding this comment.
Solid, real-account-verified fix that collapses the raw status 404/403 leaks into a single "requires a paid plan" message across every snapshot path.
- thought: the empty-body-404 discriminator in
internal/emulator/aws/client.gois inherently reactive — a reverse proxy or gateway in front of an external--endpoint-urlemulator that answers a bare 404 would be surfaced as "requires a paid plan". You've documented this tradeoff (and theGET /_localstack/pods/{name}/versionsbare-404 footgun) thoroughly ininternal/snapshot/CLAUDE.md; the durable fix is a server-side 402/403 with a reason, which is worth thelocalstack-profollow-up you noted. Non-blocking. - praise:
TestSnapshotEndpointsTranslateEmptyBody404covering all 11 gated client methods with the paired narrow-discriminator guard, the 403-only-not-401 platform mapping so a re-login never reads as a billing problem, and integration tests that fail before the fix — exactly the coverage this class of change needs.
Automated review on behalf of @gtsiolis.
Generated by Claude Code
|
Agreed, thanks! |
Closes DEVX-1009 Co-Authored-By: Claude <noreply@anthropic.com>
The platform reports an unentitled plan as a 403 rather than the emulator's empty 404, so these two commands still leaked a raw status and JSON body. Co-Authored-By: Claude <noreply@anthropic.com>
74e77d7 to
84d0476
Compare
On a plan without snapshots, every
lstk snapshotcommand failed with a raw status code. Two different signals were leaking through:Now, for all of them:
save,load,load --dry-run,remove,list s3://…): the emulator answers with a bare404and no body when snapshots aren't licensed. All 11 client methods now recognise that and returnErrSnapshotFeatureUnavailable, rendered through one shared message.savehad no error translation at all before.list,show): the platform signals the same thing as a403, now mapped toapi.ErrCloudPodsForbiddenand rendered identically. Only403maps — a rejected token is a401and stays generic, so a re-login problem is never reported as a billing problem.": ".Scope: snapshot commands only.
lstk resetshares the state-reset request withload --merge=overwrite, so its error stays generic ("feature not available on this plan") rather than naming snapshots; giving it its own paid-plan message is a follow-up.lstk decides this from the response, not from the cached license: the license's product list is coarse and the snapshot entitlement string isn't verifiable, so checking it locally risks blocking paying customers. Trade-off —
lstk loadstill starts the emulator before failing.Verified against a real freemium account
Every command below was run against a live emulator on a freemium license, not just mocks. All seven emulator paths and both platform paths render the new message;
lstk resetcorrectly stays generic; exit code is1throughout.Raw probes confirmed the premise: all six pods routes and
state/resetreturnHTTP 404with a 0-byte body — byte-identical to a control route that doesn't exist.Testing this way is what surfaced the
403: it only reproduces against the real platform, so the original mock-only version of this PR shipped a half-fix that leftsnapshot listbroken.Automated coverage: all 11 emulator requests translate the empty
404, while a404carrying a message stays generic (keeps the check narrow);403vs401covered on the platform side; integration tests forloadandsave, both verified to fail before the fix.Review: advisable — new user-facing wording, and the emulator half keys on the shape of a response rather than an explicit signal. A server-side
402/403with a reason would remove that inference entirely and is worth a follow-up onlocalstack-pro.Closes DEVX-1009
Co-Authored-By: Claude noreply@anthropic.com
🤖 Generated with Claude Code