Add snowflake-next preview emulator type - #464
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
27d3311 to
796e607
Compare
gtsiolis
left a comment
There was a problem hiding this comment.
This wires the snowflake-next preview emulator type end to end — reachable only via --type/config (never the first-run picker), with its port remap and PostgreSQL persistence handled on the start path, plus integration tests and every non-compiler-checked map/slice touch-point updated.
- suggestion(non-blocking): on
internal/emulator/snowflake/snowflake.go/internal/container/start.go— the--persistuid-1000 caveat on Linux native Docker is well documented in code, but has no user-facing signal: when it bites, the user sees only an opaque PostgreSQL "refuses to start" surfacing as a health-check timeout. A note/warning on--persistforsnowflake-nexton Linux pointing at host-directory ownership would save a confusing debugging session. Preview-acceptable to defer. - question(non-blocking): on
cmd/status.go—snowflake-nextmaps tosnowflake.NewClient(), but per the comment you added ininternal/endpoint/target.gothe preview's health payload carries a version and noservicesmap. Doeslstk statusrender correctly against that payload, or could it under-report? I did not verify this against a running preview image (the licensed image wasn't available to me). - praise: thorough TDD — the integration tests pin the port remap, both persistence paths, the first-run picker omission, and the IaC-rejection naming, and every entry the compiler can't catch (
knownImages,emulatorHealthPaths,ContainerPort,SelfValidatesLicense,emulatorDisplayNames, thecmd/status.goclient map,tipsForType) is covered.
Automated review on behalf of @gtsiolis.
Generated by Claude Code
… user Co-Authored-By: Claude <noreply@anthropic.com>
…leanup Co-Authored-By: Claude <noreply@anthropic.com>
peter-smith-phd
left a comment
There was a problem hiding this comment.
I understand what the change is trying achieve, but could we please have some upfront discussion on how to solve it? (Please forgive me if I missed the Slack message). I want to make sure we have a clean/elegant solution to this, rather than just asking Claude to make a whole ton of changes.
Some important questions I have:
- Are customers actively using the new Snowflake emulator, or is this for internal use only? What's the timeline for making the new Snowflake emulator the default?
- This PR touches 21 files and changes 601 lines of code, which seems excessive given that we're essentially making 3 small configuration changes. When I look at the code changes, I see a lot of places where a list of 3 things becomes a list of 4 things. That implies we should be centralizing those values in a common place, not just repeating the list everywhere.
|
There was a problem hiding this comment.
This adds a snowflake-next preview emulator type, reachable only through --type/config and deliberately never offered by the first-run picker; the plumbing is careful and the genuinely tricky edge cases (uid-1000 state dir, anonymous-volume-per-start leak, remote-payload collapse) are each handled and pinned by a test. I verified CI is green on 5814071 and that discovery is centralized via KnownEmulatorTypes()/KnownImageRepos().
- suggestion(non-blocking): per-type behavior still fans out across many sites —
emulatorDisplayNames,emulatorHealthPaths,ContainerPort,SelfValidatesLicense, thecmd/status.goclient map,tipsForType, andknownImages. Discovery is now centralized, but adding a type remains a set of compiler-unchecked edits in ~7 files (echoing @peter-smith-phd); a follow-up folding per-type config into one table would make the next type a single entry. - thought(non-blocking):
prepareNextStateDirininternal/container/start.gochmods the state dir0777(world-writable) under the managed volume. The rationale is sound (the emulator runs as uid 1000 and lstk cannot chown to a uid it does not own), but on a shared host it exposes the PostgreSQL cluster dir to other local users — worth noting since it is a security-relevant widening, even if acceptable for a dev tool. - thought(non-blocking): a remote preview instance collapses to
EmulatorSnowflakeininternal/endpoint/target.go, solstk status --endpoint-urlreports "Snowflake", not "Snowflake Preview", for a preview instance. It's documented and tested (TestStatusEndpointURLSnowflakePreviewPayload) and only the display name differs — just confirming that display divergence is intended. - praise: the documentation and test coverage of the failure modes is exceptional — the anonymous-volume-per-start leak, the uid-1000 PGDATA initialization failure on native Linux Docker, and the widen-an-existing-dir upgrade path are each explained on the declaration and pinned by a dedicated test.
Automated review on behalf of @gtsiolis.
Generated by Claude Code
Motivation
lstk only knows the Python Snowflake emulator:
type = "snowflake"meanslocalstack/snowflake. Running the rewritten emulator meant hand-overridingimage/tagand remapping env — a recipe that lived in a Slack thread, and which every tester had to rediscover.Solution
Adds
snowflake-nextas a preview emulator type, solstk -t snowflake-nextjust works and records the choice in config like any other type.Two deliberate calls:
The picker does not offer it.
SelectableEmulatorTypesstill lists AWS/Snowflake/Azure, so a first-time user's choice stays a GA product;KnownEmulatorTypes()(selectable + previews) is what config and--typeaccept. At GA this type is retired andsnowflaketakes over the image.lstk adapts the image instead of the image adapting to lstk. Unlike the other emulators it listens on 8080, ignores
GATEWAY_LISTEN, and declares its ownVOLUME:SNOWFLAKE_LISTEN_ADDR=0.0.0.0:8080VOLUME /var/lib/snowflake-rsPGDATA=/var/lib/snowflake-rs/data--persist; pointed at a container-local path otherwiseCovering that
VOLUMEis not cosmetic: lstk recreates the container on every start anddocker rmkeeps anonymous volumes, so an uncovered declaration stranded a full PostgreSQL cluster per start. And because the cluster is always written to disk (there is no in-memory mode),--persistis expressed as wherePGDATApoints rather than an on/off switch — so the default still gives a clean emulator on restart, like the others.Found and fixed along the way: the IaC proxies enumerated only the selectable types when naming a running non-AWS emulator, so
lstk terraformagainst a running preview reported the misleadingLocalStack AWS Emulator is not running. Same fix for the emulator list handed to extensions.Verified end-to-end against
localstack/snowflake-next:latest: health answers on the configured port over http and https, thesnowflake.subdomain resolves, and the container self-validates its license.lstk -t snowflake-next,status(reports0.1.0+a139ef6),volume path, andstopall exercised by hand on a fresh install.Docs
User-facing, worth documenting:
snowflake-next, a preview of the next Snowflake emulator. Selected withlstk start --type snowflake-next/lstk -t snowflake-next, ortype = "snowflake-next"inconfig.toml. Displayed as "LocalStack Snowflake Preview Emulator".--typehelp text now lists it;lstk start --helpgained a paragraph explaining it.config.tomlcomment mentions it next to the supported types.--persist, andlstk volume clearresets it. Nothing for the user to do on Linux — the emulator runs as uid 1000 and lstk makes the state directory writable by it; if that ever fails, the start warns and names the directory.--endpoint-urlcan target a remote snowflake-next on an image carrying localstack/snowflake-rs#2116. It is reported under the GA Snowflake emulator's name, since the two are indistinguishable from the health payload.image/tag/SNOWFLAKE_LISTEN_ADDR/PGDATAconfig from the preview period is no longer needed and should be replaced bytype = "snowflake-next". A customimageis still a hard error when switching types.The default image is
localstack/snowflake-next:latest, same tag convention as the other emulators.Review
Human review advised — new user-facing emulator type, and the persistence/mount semantics are a judgement call worth a second opinion.
Todo
--endpoint-urlagainst a remote snowflake-next: localstack/snowflake-rs#2116 (LAV-1678) is merged, so the preview now reportsservices: {"snowflake": "available"}— a shape lstk already classifies, no lstk change needed. Added an integration test pinningstatusagainst that payload and refreshed the now-stale comment ininternal/endpoint/target.go. It resolves to the GA snowflake type: the two are indistinguishable from the payload, and every remote path treats them identically, so only the display name differs.--persiston Linux where the host user is not uid 1000 — it was broken, and CI caught it:TestStartSnowflakeNextPersistsStateIntoVolumeWithPersistfailed on ubuntu (uid 1001) while both non-persist siblings passed. lstk created the bind-mounted state dir0755, so the emulator's uid 1000 could not createPGDATAin it and PostgreSQL never initialized. lstk cannot chown to a uid it does not own, soprepareNextStateDirnow widens the mode and warns if that fails.Towards LAV-595
Co-Authored-By: Claude noreply@anthropic.com