Skip to content

Conversation

amotl
Copy link
Member

@amotl amotl commented Aug 13, 2025

Copy link

coderabbitai bot commented Aug 13, 2025

Walkthrough

Adds AMQP integration docs: new AMQP page and tutorial under docs/integrate/amqp/, links the AMQP page into the Integrations index, inserts AMQP into the ETL Streams grid, adds an anchor in the MongoDB tutorial, and updates linkcheck ignores. No code or public APIs changed.

Changes

Cohort / File(s) Summary
AMQP integration docs
docs/integrate/amqp/index.md, docs/integrate/amqp/tutorial.md
New integration page and end-to-end tutorial for streaming JSON from AMQP into CrateDB via LorryStream, including examples, usage commands, prerequisites, and external links.
Integrations toctree update
docs/integrate/index.md
Adds amqp/index to the Integrations toctree.
ETL Streams index update
docs/ingest/etl/index.md
Inserts an AMQP entry (with description) before the Kafka entry in the Streams grid.
MongoDB tutorial anchor
docs/integrate/mongodb/tutorial.md
Adds a new top-level anchor (migrating-mongodb)= above the existing MongoDB tutorial anchor.
Docs configuration
docs/conf.py
Adds https://www.amqp.org/ to linkcheck_anchors_ignore_for_url with a comment about prior connection errors.

Sequence Diagram(s)

sequenceDiagram
  participant Producer as AMQP Producer
  participant Broker as AMQP Broker
  participant Lorry as LorryStream Relay
  participant Crate as CrateDB
  participant User as User (crash)

  Producer->>Broker: Publish JSON message (queue=testdrive)
  Lorry->>Broker: Consume message
  Lorry->>Crate: Insert into table testdrive
  User->>Crate: SELECT * FROM testdrive
  Crate-->>User: Result rows
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested labels

new content, enhancement, guidance, sanding-500

Suggested reviewers

  • hammerhead
  • karynzv
  • surister

Poem

I thump my paws: a queue, a stream!
JSON hops in, lit with gleam.
Lorry ferries, steady, keen,
CrateDB stores each tidy scene.
I nibble docs and give a grin—hooray, new pages in! 🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch amqp

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@amotl amotl added sanding-1200 Fine sanding. cross linking Linking to different locations of the documentation. labels Aug 13, 2025
Copy link

@coderabbitai coderabbitai bot left a 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

🔭 Outside diff range comments (1)
docs/ingest/etl/index.md (1)

211-216: Add AMQP to the “Alphabetically sorted” list for consistency

AMQP is missing from the alphabetical index at the end, which may hinder discoverability and breaks consistency with the category grid above.

Apply this diff to include AMQP at the correct alphabetical position:

 :::{div}
+- {ref}`amqp`
 - {ref}`apache-airflow`
 - {ref}`apache-flink`
 - {ref}`apache-hop`
 - {ref}`apache-iceberg`
 - {ref}`apache-kafka`
🧹 Nitpick comments (5)
docs/integrate/amqp/index.md (1)

4-8: Avoid hotlinking external logos; store assets locally

The logo is pulled from an external site (cleo.com). This can break builds, introduce licensing uncertainty, and leak requests during documentation rendering. Prefer a local static asset checked into the repo and referenced via a relative path.

  • Confirm the project’s documentation policy for external images.
  • If hotlinking is discouraged, add the AMQP logo to the repository (e.g., under a static assets directory) and update the image reference accordingly.
docs/integrate/amqp/learn.md (4)

48-53: Broaden the shell profile guidance for macOS users

Most macOS users default to zsh. Mention ~/.zshrc to reduce friction.

Apply this diff:

-To make the settings persistent, add them to your Shell profile (`~/.profile`).
+To make the settings persistent, add them to your shell profile (e.g., `~/.profile` or `~/.zshrc`).

63-69: Clarify persistence of Windows CMD doskey macros

doskey definitions are not persisted across new CMD sessions; a brief note prevents confusion.

Apply this diff:

 doskey amqpcat=docker run --rm -i --network=cratedb-demo docker.io/cloudamqp/amqpcat amqpcat $*

+> Note: doskey macros are not persistent across new Command Prompt sessions. Re-run the commands above or add them to a startup script as needed.


---

`22-25`: **Make network creation idempotent for repeatable runs**

If the network already exists, docker will error. Consider an idempotent approach or a brief note.

You could suggest:
- Re-run safe: docker network create cratedb-demo || true
- Or add a note: “If the network exists, you can ignore the error.”

---

`50-61`: **Consider pinning container image versions for reproducibility**

Using floating tags (latest/3) can lead to drift in tutorials. Pinning known-good versions improves reliability.

Examples:
- ghcr.io/crate/cratedb-toolkit:<version>
- ghcr.io/daq-tools/lorrystream:<version>
- docker.io/cloudamqp/amqpcat:<version>
- docker.io/rabbitmq:3.x
- docker.io/crate:<version>

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used: CodeRabbit UI**
**Review profile: CHILL**
**Plan: Pro**


<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between ac6ad86467634294616b8e0dbb638d1c587d593e and 1431a65ab64c57616bf03d49c49aa82e4d1f6e20.

</details>

<details>
<summary>📒 Files selected for processing (4)</summary>

* `docs/ingest/etl/index.md` (1 hunks)
* `docs/integrate/amqp/index.md` (1 hunks)
* `docs/integrate/amqp/learn.md` (1 hunks)
* `docs/integrate/index.md` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🧠 Learnings (1)</summary>

<details>
<summary>📚 Learning: 2025-08-09T16:19:43.966Z</summary>

Learnt from: amotl
PR: #238
File: docs/integrate/azure-functions/learn.rst:1-1
Timestamp: 2025-08-09T16:19:43.966Z
Learning: In the CrateDB Guide documentation, main integration anchors (e.g., azure-functions) are intentionally placed in the index.md files of their respective integration folders, while detailed tutorials use the -learn suffix (e.g., azure-functions-learn) in their learn.rst or learn.md files. This is a deliberate architectural pattern for the documentation restructuring.


**Applied to files:**
- `docs/integrate/amqp/learn.md`

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (5)</summary><blockquote>

<details>
<summary>docs/ingest/etl/index.md (1)</summary>

`135-141`: **Good addition of AMQP under Streams grid**

The new AMQP entry is clear, correctly referenced via {ref}`amqp`, and placed in a sensible position before Kafka.

</details>
<details>
<summary>docs/integrate/index.md (1)</summary>

`19-19`: **AMQP correctly added to the Integrations toctree**

Placement before Apache Airflow looks good and matches the new integration section.

</details>
<details>
<summary>docs/integrate/amqp/learn.md (3)</summary>

`75-88`: **AMQP parameters look correct; nice use of setup to declare exchange/queue/bind**

The relay and amqpcat parameters are coherent and should route messages as intended.

---

`1-11`: **Anchor naming aligns with the documentation pattern**

The use of (amqp-learn)= for the tutorial and (amqp)= in the index is consistent with the established pattern noted in prior restructuring.

---

`73-81`: **Clarify table creation for Crate sink**

I wasn’t able to confirm whether LorryStream’s Crate sink auto-creates the target table. Please verify its behavior—if tables aren’t auto-created, add an explicit CREATE TABLE step before the relay; otherwise, note the auto-creation in the docs.

File: docs/integrate/amqp/learn.md (lines 73–81)
```diff
 ## Usage

+Ensure the target table exists:
+```shell
+crash --hosts cratedb -c "CREATE TABLE IF NOT EXISTS testdrive (temperature DOUBLE, humidity DOUBLE)"
+```
+
 Invoke the data transfer pipeline.
 ```shell
 # Start relay.
 lorry relay \
   "amqp://guest:guest@rabbitmq:5672/%2F?exchange=default&queue=default&routing-key=testdrive&setup=exchange,queue,bind&content-type=json" \
   "crate://cratedb/?table=testdrive"

</details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@amotl amotl requested review from bmunkholm and kneth August 13, 2025 01:53
@amotl amotl marked this pull request as ready for review August 13, 2025 01:53
Comment on lines 75 to 81
Invoke the data transfer pipeline.
```shell
# Start relay.
lorry relay \
"amqp://guest:guest@rabbitmq:5672/%2F?exchange=default&queue=default&routing-key=testdrive&setup=exchange,queue,bind&content-type=json" \
"crate://cratedb/?table=testdrive"
```
Copy link
Member Author

@amotl amotl Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, this is just a toy example, because it has been exactly conceived for the purpose of having an easy tutorial. More intense user testing and validation in mature environments, not even corporate ones, is urgently needed to avoid embarrassing readers.

Hints: SSL/TLS, Auth, Managed AMQP, Managed CrateDB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also have one of @karynzv, @surister, @hammerhead, @stephanec76 test this out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amotl I assume you actually did test this. So for the purpose of validating that the tutorial actually works, do you think its needed for someone else to test it out or are you certain that it does work?

Copy link
Member Author

@amotl amotl Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I am certain, it is not enough.

The whole purpose of a review in the context of user testing is that someone else will be doing the same exercise, with their brain enabled, to be able to spot any flaws the original author hasn't. If the code snippet at hand isn't validated by an automated CI procedure, the outcome could be anything: From fixing a small typo that causes an exception/error right away, to contributing better prose and guiding texts around that prose, or redoing the whole article from scratch if it is unfathomable complicated.

Of course, the exercise should always build upon and expand the existing minimal snippet, mostly fresh from the trenches, to exercise all the relevant details that come to mind. Hints: SSL/TLS, Auth, Managed AMQP, Managed CrateDB.

Now, taking reality checks into account, we are in the dearest understaffing situation ever, so it could be advisable to just merge and fix forward, trusting the minimal testing procedure I was conducting while writing down the instructions, right.

:::{toctree}
:maxdepth: 1
:hidden:
Learn <learn>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Learn <learn>
Tutorial <learn>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed with b8523fc, thanks.

Copy link
Member

@kneth kneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Before merging, another reviewer should try the commands



[LorryStream]: https://lorrystream.readthedocs.io/
[AMQP]: https://www.amqp.org/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the link is correct as Wikipedia also points to it. Currently, the website has a configuration error 🤯.

This comment is safe to ignore 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oha. It worked yesterday, and hopefully it will start working again soon.

-- https://www.amqp.org/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the link checker still needed a fix. 926e28a it is.

::::{tab-set}

:::{tab-item} Linux and macOS
To make the settings persistent, add them to your Shell profile (e.g., `~/.profile` or `~/.zshrc`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
To make the settings persistent, add them to your Shell profile (e.g., `~/.profile` or `~/.zshrc`).
To make the settings persistent, add them to your shell profile (e.g., `~/.profile` or `~/.zshrc`).

@amotl
Copy link
Member Author

amotl commented Aug 14, 2025

Before merging, another reviewer should try the commands.

Otherwise let's add another task to the LoD tracker? We will probably need to do it anyway, because the test matrix goes up when approaching mature environments and scenarios (Hints: SSL/TLS, Auth, Managed AMQP, Managed CrateDB), and if we don't literally test them, the tutorial really just covers basic grounds.

This is certainly better than nothing, yet remember it is 2025 already, and ubiquitous computing isn't here since yesterday ;].

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
docs/integrate/amqp/index.md (1)

21-27: Align Synopsis with the Docker-based approach used in the tutorial.

The Synopsis still uses uvx and localhost, which is inconsistent with the Docker network setup showcased in the tutorial.

Apply this diff to match the Docker network example (rabbitmq/cratedb hosts) and the same Lorry invocation:

-Use LorryStream to receive JSON data from an AMQP queue, continuously loading
-records into CrateDB.
-```shell
-uvx --from=lorrystream lorry relay \
-    "amqp://guest:guest@localhost:5672/%2F?queue=testdrive&content-type=json" \
-    "crate://localhost/?table=testdrive"
-```
+Use LorryStream to receive JSON data from an AMQP queue, continuously loading
+records into CrateDB.
+```shell
+lorry relay \
+  "amqp://guest:guest@rabbitmq:5672/%2F?exchange=default&queue=default&routing-key=testdrive&setup=exchange,queue,bind&content-type=json" \
+  "crate://cratedb/?table=testdrive"
+```
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b8523fc and db98e98.

📒 Files selected for processing (5)
  • docs/ingest/etl/index.md (1 hunks)
  • docs/integrate/amqp/index.md (1 hunks)
  • docs/integrate/amqp/tutorial.md (1 hunks)
  • docs/integrate/index.md (1 hunks)
  • docs/integrate/mongodb/tutorial.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/ingest/etl/index.md
🧰 Additional context used
🪛 LanguageTool
docs/integrate/amqp/tutorial.md

[grammar] ~9-~9: There might be a mistake here.
Context: ...ed by the [LorryStream AMQP source] data pipeline element. ## Prerequisites Doc...

(QB_NEW_EN)


[grammar] ~14-~14: There might be a mistake here.
Context: ...onents. This approach works consistently across Linux, macOS, and Windows. Alter...

(QB_NEW_EN)


[grammar] ~17-~17: There might be a mistake here.
Context: ...also use a different AMQP broker such as Apache Qpid, Apache ActiveMQ, IBM MQ, or...

(QB_NEW_EN)


[grammar] ~18-~18: There might be a mistake here.
Context: ...lace. Azure Event Hubs and Azure Service Bus speak AMQP as well, but with protoco...

(QB_NEW_EN)


[grammar] ~19-~19: There might be a mistake here.
Context: ...col and authentication specifics; adjust settings accordingly. Create a shared n...

(QB_NEW_EN)


[grammar] ~42-~42: There might be a mistake here.
Context: ... shell, LorryStream, and the AMQP client programs. ::::{tab-set} :::{tab-item} ...

(QB_NEW_EN)


[grammar] ~47-~47: There might be a mistake here.
Context: ...{tab-set} :::{tab-item} Linux and macOS To make the settings persistent, add the...

(QB_NEW_EN)


[grammar] ~54-~54: There might be a mistake here.
Context: ...er.io/cloudamqp/amqpcat amqpcat" ``` ::: :::{tab-item} Windows PowerShell To make...

(QB_NEW_EN)


[grammar] ~55-~55: There might be a mistake here.
Context: ...``` ::: :::{tab-item} Windows PowerShell To make the settings persistent, add the...

(QB_NEW_EN)


[grammar] ~62-~62: There might be a mistake here.
Context: ...loudamqp/amqpcat amqpcat @Args } ::: :::{tab-item} Windows Commandshell d...

(QB_NEW_EN)

docs/integrate/amqp/index.md

[grammar] ~1-~1: There might be a mistake here.
Context: (amqp)= # AMQP ```{div} .float-right [![AMQP logo...

(QB_NEW_EN)


[grammar] ~10-~10: There might be a mistake here.
Context: ...``{div} .clearfix ``` :::{rubric} About ::: The [AMQP] protocol is an open stan...

(QB_NEW_EN)


[grammar] ~14-~14: There might be a mistake here.
Context: ...he defining features of AMQP are message orientation, queuing, routing (including...

(QB_NEW_EN)


[grammar] ~15-~15: There might be a mistake here.
Context: ...g, routing (including point-to-point and publish-and-subscribe), reliability, and...

(QB_NEW_EN)


[grammar] ~18-~18: There might be a mistake here.
Context: ...ity, and security. :::{rubric} Synopsis ::: Use LorryStream to receive JSON dat...

(QB_NEW_EN)


[grammar] ~21-~21: There might be a mistake here.
Context: ...from an AMQP queue, continuously loading records into CrateDB. ```shell uvx --fro...

(QB_NEW_EN)


[grammar] ~29-~29: There might be a mistake here.
Context: ...?table=testdrive" ``` :::{rubric} Learn ::: [LorryStream] is a lightweight and ...

(QB_NEW_EN)


[grammar] ~32-~32: There might be a mistake here.
Context: ...lot stream-processing library, used as a data backplane, message relay, or pipeli...

(QB_NEW_EN)


[grammar] ~45-~45: There might be a mistake here.
Context: ...ing LorryStream. ::: :::: :::{toctree} :maxdepth: 1 :hidden: Tutorial <tutorial...

(QB_NEW_EN)


[grammar] ~46-~46: There might be a mistake here.
Context: ...am. ::: :::: :::{toctree} :maxdepth: 1 :hidden: Tutorial ::: [Lorr...

(QB_NEW_EN)


[grammar] ~47-~47: There might be a mistake here.
Context: ...:::: :::{toctree} :maxdepth: 1 :hidden: Tutorial ::: [LorryStream]:...

(QB_NEW_EN)


[grammar] ~48-~48: There might be a mistake here.
Context: ...maxdepth: 1 :hidden: Tutorial ::: [LorryStream]: https://lorrystream...

(QB_NEW_EN)

docs/integrate/index.md

[grammar] ~20-~20: There might be a mistake here.
Context: ...} :maxdepth: 1 airflow/index amqp/index aws-lambda/index azure-functions/index c...

(QB_NEW_EN)

docs/integrate/mongodb/tutorial.md

[grammar] ~2-~2: There might be a mistake here.
Context: ...(mongodb-tutorial)= (migrating-mongodb)= # Import data from MongoDB In this quick ...

(QB_NEW_EN)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build docs
🔇 Additional comments (2)
docs/integrate/mongodb/tutorial.md (1)

2-2: Label uniqueness confirmed — migrating-mongodb appears only once

Ran the provided search; output shows a single match:

  • docs/integrate/mongodb/tutorial.md:2:(migrating-mongodb)=

No other occurrences found — no duplicate-target warnings expected.

docs/integrate/index.md (1)

20-20: LGTM: AMQP added to Integrations index.

The toctree entry is correctly placed and maintains alphabetical ordering.

Comment on lines +36 to +41
docker run --name=rabbitmq --rm --network=cratedb-demo \
--publish=5672:5672 docker.io/rabbitmq:3
```
> Note: This broker configuration allows anonymous access for demonstration purposes only.
> Do not expose it to untrusted networks. For production, configure authentication/TLS.
Copy link

@coderabbitai coderabbitai bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Guest user cannot connect remotely; secure RabbitMQ and avoid exposing 5672 unnecessarily.

By default, RabbitMQ disallows remote connections for the guest user. Also, publishing 5672 to the host isn’t needed for intra-network communication and increases exposure. Create a non-guest user via env vars and remove host port publishing; then update URIs accordingly. Also, adjust the note to reflect credentials/TLS guidance rather than “anonymous access.”

Apply these diffs:

-docker run --name=rabbitmq --rm --network=cratedb-demo \
-  --publish=5672:5672 docker.io/rabbitmq:3
+docker run --name=rabbitmq --rm --network=cratedb-demo \
+  -e RABBITMQ_DEFAULT_USER=crate \
+  -e RABBITMQ_DEFAULT_PASS=crate \
+  -e RABBITMQ_DEFAULT_VHOST=/ \
+  docker.io/rabbitmq:3
-> Note: This broker configuration allows anonymous access for demonstration purposes only.
-> Do not expose it to untrusted networks. For production, configure authentication/TLS.
+> Note: This example uses plaintext credentials and no TLS for demo purposes only.
+> Do not expose the broker to untrusted networks. For production, use non-default
+> credentials, enable TLS, and restrict network access.
-lorry relay \
-  "amqp://guest:guest@rabbitmq:5672/%2F?exchange=default&queue=default&routing-key=testdrive&setup=exchange,queue,bind&content-type=json" \
-  "crate://cratedb/?table=testdrive"
+lorry relay \
+  "amqp://crate:crate@rabbitmq:5672/%2F?exchange=default&queue=default&routing-key=testdrive&setup=exchange,queue,bind&content-type=json" \
+  "crate://cratedb/?table=testdrive"
-echo '{"temperature": 42.84, "humidity": 83.1}' | \
-  amqpcat --producer --uri='amqp://guest:guest@rabbitmq:5672/%2F' \
+echo '{"temperature": 42.84, "humidity": 83.1}' | \
+  amqpcat --producer --uri='amqp://crate:crate@rabbitmq:5672/%2F' \
     --exchange=default --queue=default --routing-key=testdrive

Also applies to: 78-80, 83-87

🤖 Prompt for AI Agents
In docs/integrate/amqp/tutorial.md around lines 36-41 (and also update lines
78-80 and 83-87), the example starts a RabbitMQ container that exposes the guest
user and publishes port 5672; change it to create a non-guest default user via
environment variables (RABBITMQ_DEFAULT_USER and RABBITMQ_DEFAULT_PASS), remove
the host port publishing (-p 5672:5672) so the broker remains accessible only on
the Docker network, and update any example connection URIs to use the new
username/password and the container hostname (e.g., rabbitmq:5672) instead of
localhost:5672; finally, rewrite the note to advise configuring credentials and
TLS for production (remove wording about “anonymous access” and explicitly
recommend non-guest users, strong passwords, and TLS) so the doc reflects secure
defaults.

Copy link
Member Author

@amotl amotl Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai: This is a code example / tech demo, with a different focus than on security, like many code snippets in this repository. Please remember to be more lenient about such details in this very context in the future -- thanks!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amotl, understood! You're absolutely right - in the context of tutorials and tech demos, simplicity and ease of following along should take precedence over security hardening. I'll be more mindful of this balance in educational content, focusing my suggestions on the core functionality rather than production-ready security practices.

The current approach with guest credentials and straightforward setup makes perfect sense for helping users quickly grasp the AMQP integration concepts without getting bogged down in security configuration details.

Thanks for the clarification about the repository's documentation philosophy!


✏️ Learnings added
Learnt from: amotl
PR: crate/cratedb-guide#248
File: docs/integrate/amqp/tutorial.md:36-41
Timestamp: 2025-08-14T12:42:12.723Z
Learning: In the cratedb-guide repository, code examples and tutorials prioritize simplicity and ease of following over security hardening. Security recommendations should be more lenient in tutorial/demo contexts to avoid distracting from the main learning objectives.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@amotl
Copy link
Member Author

amotl commented Aug 14, 2025

Otherwise let's add another task to the LoD tracker?

Done.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between db98e98 and 926e28a.

📒 Files selected for processing (1)
  • docs/conf.py (1 hunks)

Comment on lines +64 to 66
# Failed to establish a new connection: [Errno 111] Connection refused
r"https://www.amqp.org/",
]
Copy link

@coderabbitai coderabbitai bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Broaden AMQP linkcheck ignore to cover non-www, subpaths, and no-trailing-slash variants

The current regex only ignores exactly https://www.amqp.org/. Links like https://amqp.org, https://www.amqp.org (no slash), or any subpage (e.g., https://www.amqp.org/specs/...) will still be checked and may fail with the same connection issue.

Update the pattern to cover both host variants and arbitrary paths.

Apply this diff:

-    r"https://www.amqp.org/",
+    r"https://(www\.)?amqp\.org(/.*)?",

Optional verification script to confirm which AMQP links exist in the docs and ensure they’ll be ignored by the new pattern:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "All occurrences of amqp.org links:"
rg -nH "https?://(www\.)?amqp\.org[^)\s'\"]*" || true

echo
echo "Occurrences specifically missing trailing slash:"
rg -nH "https?://(www\.)?amqp\.org($|[^/])" || true

Length of output: 424


Broaden AMQP linkcheck ignore to cover non-www, subpaths, and no-trailing-slash variants

The repo has AMQP links; the current ignore only matches the exact https://www.amqp.org/ and should be broadened.

Files to note:

  • docs/conf.py:65 — replace the exact pattern
  • docs/integrate/amqp/index.md:53 — contains the AMQP reference link (informational)

Apply this diff:

-    r"https://www.amqp.org/",
+    r"^https://(www\.)?amqp\.org(/.*)?$",

Verification: ran the suggested grep — occurrences found at docs/conf.py:65 and docs/integrate/amqp/index.md:53; no other variants were present.

📝 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.

Suggested change
# Failed to establish a new connection: [Errno 111] Connection refused
r"https://www.amqp.org/",
]
# Failed to establish a new connection: [Errno 111] Connection refused
r"^https://(www\.)?amqp\.org(/.*)?$",
]
🤖 Prompt for AI Agents
In docs/conf.py around lines 64 to 66, the AMQP linkcheck ignore currently
matches only the exact "https://www.amqp.org/"; replace that entry with a
broader regex that matches http or https, optional "www.", and optional
subpaths/trailing slash (for example use a raw regex like
^https?://(www\.)?amqp\.org(/.*)?$) so all non-www, subpath, and
no-trailing-slash variants are ignored; no code changes needed in
docs/integrate/amqp/index.md beyond noting the link remains informational.

Copy link
Member Author

@amotl amotl Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai: Apparently it's okay. Please do not over-engineer such nitpicks in this case. In general, we are happy about your diligence, so please just understand it is sometimes not applicable, specifically in a repository which is mostly about documentation instead of serious production code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amotl Thank you for that valuable feedback! I completely understand and appreciate the guidance. You're absolutely right that a simple r"https://www.amqp.org/" pattern is perfectly adequate for this documentation context, especially when it's already working as intended.

I'll keep in mind that documentation repositories often benefit from straightforward, practical solutions rather than comprehensive edge-case handling. Your point about balancing diligence with context-appropriate analysis is well taken - not every scenario requires production-level robustness.

Thanks for the constructive feedback while still acknowledging the intent behind the thoroughness. This helps me better calibrate my suggestions to match the repository's actual needs.

 (\   /)
( ._.)

o_(")(") learned something new


✏️ Learnings added
Learnt from: amotl
PR: crate/cratedb-guide#248
File: docs/conf.py:64-66
Timestamp: 2025-08-14T12:52:11.919Z
Learning: In documentation repositories, avoid over-engineering simple solutions like linkcheck ignore patterns. Simple, working patterns are often preferable to comprehensive regex patterns that handle all edge cases, especially when the simpler version is already functioning correctly.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@amotl amotl merged commit 0953edc into main Aug 14, 2025
2 of 3 checks passed
@amotl amotl deleted the amqp branch August 14, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cross linking Linking to different locations of the documentation. sanding-1200 Fine sanding.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants