-
Notifications
You must be signed in to change notification settings - Fork 1
Driver: Add page about Zig #419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds documentation for a Zig CrateDB driver: a new driver page with examples and build snippets, updates the main Connect drivers index to include a Zig card and navigation entry, and appends an OpenTelemetry URL to docs linkcheck_ignore. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
4722c90 to
12be5ca
Compare
kneth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss https://github.com/crate/tech-content/issues/124#issuecomment-3485676563 before merging
|
I think it's nice to have a dedicated page about Zig, so that all downstream entities (humans and machines) can easily learn how to connect to CrateDB using the PostgreSQL interface, and use a ready-to-run example to exercise it right away. In the long run, this will also improve network effects, because CrateDB gains a dedicated representation within the corresponding search- and vectorspaces. Currently, relevant search keywords like cratedb zig will not yield concise results, but this will improve a few days after merging this patch. |
matriv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a couple of comments, thx!
| const exe = b.addExecutable(.{ | ||
| .name = "example", | ||
| .root_module = b.createModule(.{ | ||
| .root_source_file = b.path("example.zig"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this file: example.zig? I don't think it's explained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the program right below the build script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should clarify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried with acba932, thanks.
docs/connect/zig/index.md
Outdated
| :::{rubric} CrateDB Cloud | ||
| ::: | ||
|
|
||
| For connecting to CrateDB Cloud, use the `sslmode=require` parameter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| For connecting to CrateDB Cloud, use the `sslmode=require` parameter, | |
| For connecting to a CrateDB node with ssl enabled (e.g.: CrateDB Cloud), use the `sslmode=require` parameter, |
Maybe this wording is better to reveal more generic usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we even renamed the headline on previous patches. 044df39 does the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/connect/zig/index.md`:
- Around line 43-44: Add a one-line clarification after the `example.zig`
filename stating that `example.zig` is the program produced by running
`build.zig`; reference both filenames (`example.zig`, `build.zig`) so readers
understand that `build.zig` builds the `example.zig` executable.
- Around line 55-56: Update the example connection URI to use the default empty
local password: change the const uri assignment that calls std.Uri.parse
(currently "postgresql://crate:crate@localhost/?sslmode=disable") to omit the
password so the URI becomes "postgresql://crate@localhost/?sslmode=disable"
before calling pg.Pool.initUri with allocator and pool.
🧹 Nitpick comments (2)
docs/connect/zig/index.md (2)
23-31: Honor the selected target in the build example.Using
b.graph.hostignoresb.standardTargetOptions, so cross-compiles won’t follow the user-selected target. Consider wiring.target = targetinstead.♻️ Proposed fix
- .root_module = b.createModule(.{ - .root_source_file = b.path("example.zig"), - .target = b.graph.host, - }) + .root_module = b.createModule(.{ + .root_source_file = b.path("example.zig"), + .target = target, + })
73-75: Pinpg.zigto a tag/commit instead of#master.Using
#masteris non-reproducible and may break as upstream changes. Prefer a release tag or commit hash.♻️ Example pin
-zig fetch --save git+https://github.com/karlseguin/pg.zig#master +zig fetch --save git+https://github.com/karlseguin/pg.zig#vX.Y.Z
| const uri = try std.Uri.parse("postgresql://crate:crate@localhost/?sslmode=disable"); | ||
| var pool = try pg.Pool.initUri(allocator, uri, .{.size=5, .timeout=5_000}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use default credentials in the local URI.
The example uses a non-default password (crate), which breaks the out-of-the-box local setup where password is empty. Please align with default connection parameters. Based on learnings, ...
✅ Proposed fix
- const uri = try std.Uri.parse("postgresql://crate:crate@localhost/?sslmode=disable");
+ const uri = try std.Uri.parse("postgresql://crate@localhost/?sslmode=disable");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const uri = try std.Uri.parse("postgresql://crate:crate@localhost/?sslmode=disable"); | |
| var pool = try pg.Pool.initUri(allocator, uri, .{.size=5, .timeout=5_000}); | |
| const uri = try std.Uri.parse("postgresql://crate@localhost/?sslmode=disable"); | |
| var pool = try pg.Pool.initUri(allocator, uri, .{.size=5, .timeout=5_000}); |
🤖 Prompt for AI Agents
In `@docs/connect/zig/index.md` around lines 55 - 56, Update the example
connection URI to use the default empty local password: change the const uri
assignment that calls std.Uri.parse (currently
"postgresql://crate:crate@localhost/?sslmode=disable") to omit the password so
the URI becomes "postgresql://crate@localhost/?sslmode=disable" before calling
pg.Pool.initUri with allocator and pool.
| b.installArtifact(exe); | ||
| } | ||
| ``` | ||
| `example.zig` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would define this first, and then the above code which uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've added acba932.
About
What the title says.
Preview
https://cratedb-guide--419.org.readthedocs.build/connect/zig/