SQL for BYOVPC#620
Conversation
✅ Deploy Preview for rp-cloud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a new documentation page ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
||
| [NOTE] | ||
| ==== | ||
| The Redpanda Terraform provider does not support SQL customer-managed resource fields. Use the Cloud API or Cloud Console to supply these resources and enable SQL on a BYOVPC cluster. |
There was a problem hiding this comment.
There's PR that will add this in review.
redpanda-data/terraform-provider-redpanda#353
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/sql/pages/get-started/enable-sql-byovpc-aws.adoc (1)
364-369: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueClarify that scaling instructions on the BYOC SQL page apply to BYOVPC.
Line 366 links to
deploy-sql-cluster.adocfor scaling, which is now labeled "Enable Redpanda SQL on BYOC" (per nav.adoc line 335). When BYOVPC users click this link, they will land on a page titled for BYOC clusters, which may cause momentary confusion.However, the scaling mechanism is identical for both cluster types (adjusting
replicasin therpsqlblock), so the link target is functionally correct. Consider adding a brief note to clarify that the scaling instructions on that page apply to both BYOC and BYOVPC clusters, or alternatively, reference the specific section of the BYOC page (e.g.,deploy-sql-cluster.adoc#scale-redpanda-sql) if AsciiDoc anchors are available.🤖 Prompt for 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. In `@modules/sql/pages/get-started/enable-sql-byovpc-aws.adoc` around lines 364 - 369, In the "Next steps" section of the enable-sql-byovpc-aws.adoc file, the link labeled "Scale the SQL engine" currently points to deploy-sql-cluster.adoc which is titled for BYOC clusters. To clarify that the scaling instructions apply to both BYOC and BYOVPC, either add a brief inline note (such as "(applies to both BYOC and BYOVPC)" after the link text) or update the link to reference a specific scaling section anchor (e.g., deploy-sql-cluster.adoc#scale-redpanda-sql) if such anchors exist in the target document. This will prevent BYOVPC users from experiencing confusion when landing on a page titled for BYOC clusters.
🤖 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.
Nitpick comments:
In `@modules/sql/pages/get-started/enable-sql-byovpc-aws.adoc`:
- Around line 364-369: In the "Next steps" section of the
enable-sql-byovpc-aws.adoc file, the link labeled "Scale the SQL engine"
currently points to deploy-sql-cluster.adoc which is titled for BYOC clusters.
To clarify that the scaling instructions apply to both BYOC and BYOVPC, either
add a brief inline note (such as "(applies to both BYOC and BYOVPC)" after the
link text) or update the link to reference a specific scaling section anchor
(e.g., deploy-sql-cluster.adoc#scale-redpanda-sql) if such anchors exist in the
target document. This will prevent BYOVPC users from experiencing confusion when
landing on a page titled for BYOC clusters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 651baad6-bd95-472b-a754-72e12a7c8e95
📒 Files selected for processing (4)
modules/ROOT/nav.adocmodules/get-started/pages/cluster-types/byoc/aws/vpc-byo-aws.adocmodules/sql/pages/get-started/deploy-sql-cluster.adocmodules/sql/pages/get-started/enable-sql-byovpc-aws.adoc
8c51244 to
b98fe30
Compare
jacekseligarp
left a comment
jacekseligarp
left a comment
There was a problem hiding this comment.
imho all good, but I suggest to wait for @rpdevmp tests of BYOVPC RP SQL.
| } | ||
| } | ||
| } | ||
| rpsql = { |
There was a problem hiding this comment.
please confirm with @gene-redpanda which fields are required and if some fields have a built-in default
b98fe30 to
622a74d
Compare
Addresses rpdevmp's review comment distinguishing multi-AZ streaming clusters from the always-single-AZ SQL engine. Also updates the BYOC note to reflect that Redpanda SQL now runs on GCP in addition to AWS, alongside the BYOVPC guide pointer.
GCP BYOC/BYOVPC Redpanda SQL isn't customer-available yet (targeted end of July/early August). Only AWS ships with this PR.
Feediver1
left a comment
Feediver1
left a comment
There was a problem hiding this comment.
Final-pass review (docs-team-standards)
Overall: Thorough, SME-validated guide that's in strong shape at head — the final state is substantially better than the commit history suggests (the Terraform-provider path was fully incorporated after provider v2.1.0 shipped). One unresolved SME thread is the only thing I'd block on.
Jira ticket alignment
DOC-2227 (BYOVPC for Oxla AWS, epic ENG-1165) — ✅ fully addressed: enablement (new + existing cluster, three methods), verification, disable semantics, resource immutability, and cleanup.
SME thread verification
- ✅ @rpdevmp confirmed the API call and approved (7/17) after testing — @jacekseligarp's conditional approval ("wait for rpdevmp tests") is satisfied.
- ✅ rpdevmp's multi-AZ clarification (7/16) is incorporated at all four
<sql-az>bullets. - ✅ The early NOTE claiming "the Terraform provider does not support SQL customer-managed resource fields" was correctly removed — verified that provider v2.1.0 ships the
rpsql_*CMR fields (PR redpanda-data/terraform-provider-redpanda#353 merged just before the tag), so the What's New claim ("Console, Cloud API, or Terraform provider") and the page are consistent.
Must resolve before merge
- Open SME question on the new-cluster payload: @jacekseligarp asked (6/23) to confirm with @gene-redpanda which fields are required and which have built-in defaults for the
POST /v1/clustersrequest. No reply landed in the thread; rpdevmp's testing confirmed the existing-cluster PATCH path. Please resolve (or note where it was answered) before merging.
Suggestions
- The
<sql-az>bullet is duplicated verbatim 4× — a maintenance hazard (this PR itself had to fix it in four places after the SME comment proved the point). Consider a partial, or move the AZ semantics to a single NOTE above the tabs. <<On an existing cluster>>uses heading-text cross-reference form; it works, but the anchor form (<<on-an-existing-cluster>>) survives heading edits better.- Follow-up (not in this PR):
manage/terraform-provider.adoc's "Enable Redpanda SQL on a BYOC cluster" section could gain a one-line pointer to this new BYOVPC page, since the provider path now exists for both.
Verified clean
- Nav entries retitled consistently; What's New entry added;
sql/get-started/index.adocusespage-layout: indexso the new page auto-lists; the BYOC page retitle keeps its filename/anchors, so no inbound xrefs break. feature-flag-rpcn.adocinclude renders the correct BYOVPC/Premium-support gate (same as the three BYOVPC cluster pages) — the partial's filename is misleading legacy, worth a rename someday, not here.- Correct glossterm usage, 3 checkbox LOs with Bloom verbs, sentence-case H2/H3s, language specifiers on every code block.
- CodeRabbit's single nitpick (scaling clarity) is already covered by the final Next steps wording.
What works well
- The SME loop actually closed: tested before approval, API calls confirmed, AZ semantics corrected everywhere
- The Disable section is honest about destructive semantics (catalog state loss, in-flight query failure, what survives, fresh-engine re-enable) — exactly the customer-experience detail this page needed
🤖 Automated final-pass review via docs-team-standards pr-review.
|
|
||
| [IMPORTANT] | ||
| ==== | ||
| The SQL customer-managed resources (`rpsql_node_group_instance_profile`, `rpsql_cloud_storage_bucket`, `rpsql_security_group`) are immutable while SQL is enabled. To change any of these resources, disable SQL first. |
There was a problem hiding this comment.
| The SQL customer-managed resources (`rpsql_node_group_instance_profile`, `rpsql_cloud_storage_bucket`, `rpsql_security_group`) are immutable while SQL is enabled. To change any of these resources, disable SQL first. | |
| The SQL customer-managed resources (`rpsql_node_group_instance_profile`, `rpsql_cloud_storage_bucket`, `rpsql_security_group`) are immutable while SQL is enabled. To change any of these resources, you must first disable SQL. |
|
@kbatuigas We probably need the IAM permissions setup properly to allow RPSQL to communicate to glue: redpanda-data/terraform-aws-redpanda-byovpc#52 |
Introduces enable-sql-byovpc.adoc as an index page nesting enable-sql-byovpc-aws.adoc under it in nav.adoc, so a GCP sibling page can be added later without restructuring the nav tree or moving files.
5 participants
Description
Adds a how-to guide for enabling Redpanda SQL on BYOVPC clusters on AWS, and updates surrounding pages to distinguish the BYOC and BYOVPC paths.
New page —
sql/get-started/enable-sql-byovpc-aws.adoc:enable_redpanda_sql = truein the BYOVPC Terraform module to create the three SQL-specific AWS resources (IAM instance profile, S3 bucket, security group) and collect their ARNs.psql/SQL editor connection.<sql-az>parameter (per SME review: the engine always deploys to one AZ even on multi-AZ clusters; the AZ locks on first enable).Terraform provider support: the page documents the Terraform path throughout — provider v2.1.0 ships the
rpsql_*customer-managed resource fields (terraform-provider-redpanda#353).Surrounding updates:
nav.adoc: retitled the pair to "Enable Redpanda SQL on BYOC" / "Enable Redpanda SQL on BYOVPC".deploy-sql-cluster.adoc: retitled to "Enable Redpanda SQL on a BYOC Cluster"; NOTE directs BYOVPC users to the new guide.vpc-byo-aws.adoc: next-steps link to the new guide.Resolves DOC-2227
Review deadline:
Page previews
Checks