feat: ClickHouse database setup SQL on the warehouse card and docs - #8112
feat: ClickHouse database setup SQL on the warehouse card and docs#8112Zaimwa9 wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
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:
📝 WalkthroughWalkthroughAdds customer-managed ClickHouse support for experiment warehouses, including generated database/table DDL, updated database defaults, setup documentation, and tests. Adds expandable SQL setup help with syntax highlighting and clipboard copying to the ClickHouse configuration form and connection card. Estimated code review effort: 3 (Moderate) | ~20 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
front-end
github-actions
Bot
added
docs
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis judgement: 🟠 Fix before mergeA small, well-shaped UI + docs addition that copies the existing
🟠 Majors
🧹 Nits
📝 Walkthrough
🧪 How to verify
Product take: Removes a genuine onboarding dead end — before this, a BYO-ClickHouse customer had no idea what table Flagsmith expected. Modest in size but high leverage for the experimentation beta, provided the database name in the snippet and the form agree. Big picture: The events schema is now maintained in three places — the TS constant, the docs code block, and whatever component actually writes the table (not in this repo). Nothing links them, so a column added upstream silently leaves customers with a table that no longer matches. The cheapest boundary is one source for the DDL in the repo (generate or include the docs block from the constant) plus a test pinning the database/table name to 🧭 Assumptions & unverified claims
Handing customers a |
github-actions
Bot
added
feature
Docker builds report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea600869-bc74-4701-a542-6a60f9f2387c
📒 Files selected for processing (4)
docs/docs/experimentation/connect-a-warehouse.mdfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.ts
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18830 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #18830 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18830 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #18830 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18829 (attempt 2)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #18829 (attempt 2)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18829 (attempt 2)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18829 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18829 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #18829 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #18824 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #18824 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
|
…th_exp, snippet in form
5a61559 to
8df5da2
Compare
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 845b411a-f36f-4c65-91fc-8d9ce22df030
📒 Files selected for processing (12)
docs/docs/experimentation/connect-a-warehouse.mdfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.scssfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseSetupSql.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/warehouseFormUtils.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts
github-actions
Bot
added
feature
|
@themis-blindfold review |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.ts (2)
1-5: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate or safely quote the database identifier.
The database field is only checked for non-emptiness before being interpolated into executable DDL. A crafted value can alter the copied SQL an administrator runs. Restrict it to valid ClickHouse identifiers or quote and escape it centrally; add malicious-input coverage.
29-41: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftStatic onboarding SQL breaks custom database onboarding. The helper hardcodes
flagsmith_exp, while the create form allows a different database after Step 1.
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.ts#L29-L41: accept the configured database and generate grants and DDL from it.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsx#L160-L167: provide the live database value to the onboarding SQL, rather than calling the static helper.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsx (1)
162-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the actual SQL placeholders in the instructions.
The script contains
<USER>and<CHANGE_ME_PASSWORD>, not<USERNAME>and<YOUR_SECURED_PASSWORD>.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsx (1)
1-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBoth new SQL components bypass the required component-folder structure.
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsx#L1-L30: move the component into its named folder with barrel and co-located SCSS.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsx#L1-L35: move the component into its named folder with barrel and co-located SCSS.As per coding guidelines, “Each new component must live in its own folder with a barrel
index.ts, aComponentName/ComponentName.tsxfile, [and] co-locatedComponentName.scss.”Source: Coding guidelines
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsx (1)
17-17: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the visibility override union into a named type.
Define, for example,
type VisibilityOverride = boolean | null, then use it in the state declaration.As per coding guidelines, “Extract inline union types into named types.”
Source: Coding guidelines
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts (1)
14-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe specialised warning path lacks punctuation handling and regression coverage.
frontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts#L14-L15: append terminal punctuation todetailbefore adding the next sentence.frontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/warehouseFormUtils.test.ts#L13-L24: add a missing-table detail without terminal punctuation and assert a sentence break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f0a3f2f-9772-477f-8c94-9d69548cdd63
📒 Files selected for processing (12)
docs/docs/experimentation/connect-a-warehouse.mdfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/ClickHouseConfigForm.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseConnectionCard.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.scssfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetup.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSetupSqlHelp.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/WarehouseSqlSnippet.tsxfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/clickhouseSetupSql.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/__tests__/warehouseFormUtils.test.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseConfig.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/clickhouseSetupSql.tsfrontend/web/components/pages/environment-settings/tabs/warehouse-tab/warehouseFormUtils.ts
| const [username, setUsername] = useState(defaults.username) | ||
| const [password, setPassword] = useState('') | ||
| const [secure, setSecure] = useState(defaults.secure) | ||
| const [secure] = useState(defaults.secure) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not make persisted TLS-off connections impossible to secure.
Existing secure: false configurations render as a disabled off switch, so users cannot enable TLS while editing and unrelated saves preserve the insecure setting. Restore the control or deliberately migrate and enforce secure: true.
Also applies to: 227-230
| const showSendFirstEvent = isFlagsmith | ||
| ? !isPending && !isConnected | ||
| : !connection.total_events_received |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
The else branch covers every non-flagsmith type, not just ClickHouse. Observed: annotate_warehouse_event_stats only populates stats for flagsmith and clickhouse (api/experimentation/services.py:917-935), so total_events_received stays null for a stored Snowflake connection and this button renders permanently, while handleSendTestEvent chains into test-warehouse-connection, which returns 400 for other types (api/experimentation/views.py:165-171) — Predicted: a "Failed to send test event" toast on every click.
| const showSendFirstEvent = isFlagsmith | |
| ? !isPending && !isConnected | |
| : !connection.total_events_received | |
| const showSendFirstEvent = isFlagsmith | |
| ? !isPending && !isConnected | |
| : connection.warehouse_type === 'clickhouse' && | |
| !connection.total_events_received |
| {isEdit && ( | ||
| <WarehouseSetupSqlHelp | ||
| database={database.trim() || CLICKHOUSE_DEFAULTS.database} | ||
| showInitially={testState === 'errored'} |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
The panel opens for failures it can't help with, then closes the moment the user reacts to it — setField resets testState to 'idle' on every keystroke (line 84), so editing the Database field re-collapses the SQL, and an auth or timeout failure expands it pointlessly; a sticky "last failure was a missing events table" flag would fit better than live test state.
|
|
||
| ## Bring your own ClickHouse | ||
|
|
||
| Instead of the managed Flagsmith warehouse, you can store experiment events in your own **ClickHouse** instance. |
There was a problem hiding this comment.
🧹 Nit · ⚡ Quick win
"Instead of the managed Flagsmith warehouse" promises isolation the results path does not provide. Observed: every results and exposure query runs against EXPERIMENTATION_CLICKHOUSE_URL via _get_clickhouse_client (api/experimentation/services.py:120-131, api/experimentation/results_query.py), and the only thing read from the customer's instance is an event count — Predicted: a reader choosing this for data residency would conclude their events stay off Flagsmith infrastructure, when experiments only produce results from the managed warehouse. "In addition to" plus a pointer to the results caveat at line 109 would match the code.
⚖️ Themis review: 🧹 Ship it, nits insideThe two-step onboarding flow is a real improvement over the previous single form, and the naming is now coherent:
🧹 Nits
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: the docs-parity test already guards the SQL copies; a render test for Product take: Self-serve setup for bring-your-own ClickHouse — the SQL is now one copy button away in the product instead of a support conversation, and the numbered steps make the ordering (provision, then connect) obvious. That is a solid capability improvement for the experimentation beta, above routine polish. The rough edge is the fixed Big picture: The canonical 🧭 Assumptions & unverified claims
A setup script that documents itself, tested against the docs that document it — turtles, but well-stacked ones. · reviewed at 8df5da2 |
github-actions
Bot
added
feature
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Surfaces the one-off ClickHouse database setup SQL (
eventstable DDL):total_events_receivedis currently always null for ClickHouse connections — a backend follow-up will extend event stats to ClickHouse so the collapse becomes effective.)flagsmith_exp: form default and placeholder updated (backend default to follow in the backend PR).How did you test this code?
Unit tests (including snippet interpolation pinned to the form's default database), lint, typecheck (no new errors against the baseline). Manually: checked the section renders on card and form for ClickHouse only, interpolates the Database field value, copies the SQL, and collapses/expands on click.