Use ClickHouse defaults matching out-of-the-box setup#1205
Merged
Conversation
So that the Playground at http://localhost:8123/play opens with no credentials prompt, the managed-container defaults change to the out-of-the-box ClickHouse setup: user=default, password=empty, database=default. The Docker entrypoint treats user=default + password=empty as a no-op, so the pre-existing default user with no password is used. ENVIO_CLICKHOUSE_DATABASE becomes required at runtime (no fallback to envio_indexer), so `envio start` users must supply all four ENVIO_CLICKHOUSE_* vars explicitly. Empty passwords now pass validation via a readAllowEmpty path that distinguishes unset from set-but-empty.
`process.env[k]` already returns undefined for unset vars, so the ternary was a no-op.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPreserves empty-string ClickHouse passwords, requires and validates an explicit ClickHouse database for storage, changes CLI local-dev ClickHouse defaults (password -> "" and database -> "default"), updates tests/E2E to set the database, adjusts binding log/messages wording, and improves Docker network teardown. ChangesClickHouse Environment Defaults and Database Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
User-facing terminology in config.yaml is `storage.clickhouse`, so the runtime error messages now match.
A stray container (test harness, manual `docker run`) attached to
`envio-network` was making `envio local docker down` fail with a hard
error. Volume cleanup had already succeeded, so the user's actual ask
(nuke stale state) was met, but the non-zero exit obscured that.
Now: on 403 ("has active endpoints"), inspect the network, force-
disconnect every attached container, and retry the remove. If a fresh
container has already reattached, log a warning and continue rather
than erroring — the volumes are already gone.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/docker_env.rs`:
- Around line 1223-1234: The current loop in inspect that unconditionally calls
docker.disconnect_network for every container_id can disconnect non-Envio
containers; change it to only disconnect endpoints that are provably Envio-owned
or stale: for each container_id from inspect.containers, query the container
metadata (e.g., via docker.inspect_container or equivalent) and check for an
Envio-specific label or name prefix (or other provenance marker your codebase
uses) before creating a NetworkDisconnectRequest and awaiting
disconnect_network; skip and do not force-disconnect any container that lacks
those Envio identifiers and instead leave it attached (optionally log/emit the
existing warning path). Ensure this logic ties into the existing
stop_and_remove() flow so you only target containers you created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23dc79c1-c07e-4839-99b4-3c68d38090cf
📒 Files selected for processing (1)
packages/cli/src/docker_env.rs
Force-disconnecting every container attached to envio-network was hostile to parallel dev sessions or any container the user had intentionally hooked up. Gate the disconnect on CONFIG_HASH_LABEL so we only touch Envio containers — anything else falls through to the existing warning path, which logs and continues without nuking the network.
test:deps runs after test:e2e and resumes from the Postgres checkpoint written by the previous step. With the CLI's new default-DB (\"default\"), the indexer would look for \"default.envio_checkpoints\" while the data lives under \"envio_indexer\". Pin the database name explicitly so both jobs target the same ClickHouse schema.
The 26.x ClickHouse entrypoint writes \`/etc/clickhouse-server/users.d/default-user.xml\` whenever \`CLICKHOUSE_USER\` or \`CLICKHOUSE_PASSWORD\` is set at all (even to \"default\"/empty). The override removes the image-shipped \`default\` user (configured with \`<no_password/>\`) and recreates it with \`<password></password>\`, which then rejects empty-password basic auth — breaking the very thing this PR set out to fix. Forward each env var only when it diverges from the image's own defaults so a vanilla \`envio dev\` leaves the pristine no-password default user intact.
The 26.x ClickHouse image ships its own \`/etc/clickhouse-server/users.d/default-user.xml\` that locks down the \`default\` user with a password element — so even with no CLICKHOUSE_USER/PASSWORD env vars set, empty-password Basic Auth requests from the Playground and the indexer get rejected. Skipping the env vars alone (the previous fix) isn't enough; we have to override the image's own users.d/ file. Write a tiny <no_password/> override to \`<project>/.envio/clickhouse-users.d/default-user.xml\` and bind-mount that directory at /etc/clickhouse-server/users.d when the user hasn't customized any ClickHouse env vars. Only applied to envio dev's managed container; external ClickHouse and user-customized setups go through unchanged.
The previous fix wrote the override file into <project>/.envio/ and bind-mounted it. That had several footguns: Docker Desktop file-sharing requirements on macOS, container restart failures if the host file disappears, ownership ambiguity with the codegen-owned .envio/ dir, and hardcoded XML that was overly broad (whole-user replace). This commit: - Shrinks the override to the minimum needed — replacing only <networks> on the default user, leaving <no_password/>, profile, quota, and access_management inherited from the image's users.xml. - Builds the override as an in-memory tar (via the `tar` crate) and PUTs it to /containers/.../archive between create and start, so the file lives in the container's writable layer with no host-side dependency. - Bumps ch_config_hash with a v2 marker so containers from older CLI versions get recreated automatically. - Adds a unit test that round-trips the tar to verify the entry path and content are what we expect. Preserves the existing gate: the override is only injected when all three ENVIO_CLICKHOUSE_USERNAME/PASSWORD/DATABASE values are at the image's own defaults. If the user customizes any of them, we pass their values through to the entrypoint untouched.
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
2 participants
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.
Changes ClickHouse configuration defaults to match the out-of-the-box ClickHouse setup, eliminating the need for credentials in local development while requiring explicit configuration in production.
Summary
Updates default ClickHouse credentials and database name to use the pre-existing
defaultuser (with no password) anddefaultdatabase that come with a fresh ClickHouse installation. This allowsenvio devto work without additional setup, whileenvio startcontinues to require explicit environment variable configuration.Key Changes
docker_env.rs): Changedch_passworddefault from"testing"to""(empty string) andch_databasedefault from"envio_indexer"to"default"Env.res): AddedreadAllowEmptyfunction to distinguish between unset environment variables (None) and explicitly set empty passwords (Some("")), allowing passwordless ClickHouse usersPgStorage.res): AddedENVIO_CLICKHOUSE_DATABASEto required environment variable validation forenvio start, ensuring users explicitly configure the database when not using defaultsSink.res): Clarified that database is not passed to the client constructor; each query qualifies the name explicitly or runsUSEfirstENVIO_CLICKHOUSE_DATABASEenvironment variable to test configurationsENVIO_CLICKHOUSE_DATABASEin the list of required variablesImplementation Details
The distinction between "unset" (
None) and "set but empty" (Some("")) for the password is critical: ClickHouse supports passwordless users, so an empty string is a valid credential value that must be preserved and not conflated with an unset environment variable.https://claude.ai/code/session_017sHyqsHsXQ7D3ZrxP5wcev
Summary by CodeRabbit
Bug Fixes
Tests
Chores