Skip to content

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Nov 19, 2025

Description

This PR updates configuration comments and user documentation for both the MCP server and Presto:

  • Adds explicit host and port example fields (commented out) to the mcp_server and presto sections in clp-config.yaml.
  • Removes outdated instructions and aligns wording across guides for consistency.
    • Simplifies and clarifies the MCP server setup instructions in guides-mcp-server, emphasizing uncommenting the block and replacing defaults when needed.
    • Updates the Presto usage guide to include concrete default example values (localhost, 8889) and clarifies that users may replace them if needed.

These changes improve clarity for users configuring CLP components.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

task docs:serve and inspected the docs.

Summary by CodeRabbit

  • Documentation
    • Enhanced MCP server configuration guidance with clearer setup instructions and explicit host/port configuration examples, replacing previous multi-step workflows.
    • Improved Presto configuration documentation with concrete defaults and guidance on updating settings with actual cluster values.
    • Added reference configuration examples for host and port settings.

✏️ Tip: You can customize this high-level summary in your review settings.

…erver` and `presto` in `clp-config.yaml`; Clarify configuration steps in the manuals.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

Documentation and configuration template updates clarify MCP server and Presto setup instructions. The changes add host/port example values to the configuration template and revise user guides to provide explicit configuration steps rather than referential instructions.

Changes

Cohort / File(s) Summary
Configuration template example
components/package-template/src/etc/clp-config.yaml
Added commented host and port example values under mcp_server and presto sections to illustrate available configuration options.
MCP server setup documentation
docs/src/user-docs/guides-mcp-server/index.md
Revised guidance to require explicitly uncommenting and configuring the mcp_server block with host and port values directly, replacing previous two-step uncomment/locate approach. Reformatted and clarified note explaining that omitting or commenting the section prevents MCP server startup.
Presto configuration documentation
docs/src/user-docs/guides-using-presto.md
Updated instructions to emphasize updating Presto host and port with actual cluster values; modified YAML example to show concrete default values instead of referencing descriptions in other sections.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

These are primarily documentation and configuration template updates with consistent, straightforward changes across files. Review focuses on verifying accuracy of examples and clarity of instructional messaging.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately summarizes the main changes: adding commented default values to the config file and clarifying configuration steps in documentation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch nullable-config-docs

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f18e59 and 99079ac.

📒 Files selected for processing (3)
  • components/package-template/src/etc/clp-config.yaml (2 hunks)
  • docs/src/user-docs/guides-mcp-server/index.md (2 hunks)
  • docs/src/user-docs/guides-using-presto.md (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: davemarco
Repo: y-scope/clp PR: 1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.

Applied to files:

  • components/package-template/src/etc/clp-config.yaml
  • docs/src/user-docs/guides-using-presto.md
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.

Applied to files:

  • components/package-template/src/etc/clp-config.yaml
📚 Learning: 2025-10-02T15:48:58.961Z
Learnt from: 20001020ycx
Repo: y-scope/clp PR: 1368
File: components/clp-mcp-server/clp_mcp_server/__init__.py:11-15
Timestamp: 2025-10-02T15:48:58.961Z
Learning: In the clp-mcp-server component (components/clp-mcp-server/clp_mcp_server/__init__.py), the default host binding of 0.0.0.0 is intentional because the server is designed to be deployed in Docker containers where this binding is necessary to accept external connections.

Applied to files:

  • components/package-template/src/etc/clp-config.yaml
  • docs/src/user-docs/guides-mcp-server/index.md
⏰ 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). (2)
  • GitHub Check: package-image
  • GitHub Check: package-image
🔇 Additional comments (4)
docs/src/user-docs/guides-mcp-server/index.md (1)

24-38: Clarify host binding guidance for Docker deployments.

The YAML example specifies host: "localhost", but according to the project learnings, the MCP server is designed to bind to 0.0.0.0 by default when deployed in Docker containers to accept external connections. Binding to localhost (or 127.0.0.1) in Docker restricts access to the container itself only. This may confuse users configuring the server for external agent connectivity.

Consider adding a clarifying note explaining the host options:

    mcp_server:
       host: "localhost"
       port: 8000
       # other settings
    ```

+   :::{note}
+   For development, `"localhost"` works if running agents on the same machine. For Docker deployments allowing external agent connections, use `"0.0.0.0"` instead. For remote deployments, specify the actual server IP address.
+   :::

Alternatively, align the default example with the actual code default (0.0.0.0) documented in the learnings.

components/package-template/src/etc/clp-config.yaml (2)

71-73: Align MCP server host default with codebase and Docker best practices.

Similar to the guide, this template example shows host: "localhost" but learnings indicate the MCP server defaults to binding 0.0.0.0 in Docker. Consider updating the comment to reflect the actual codebase default, or add clarification about when to use each value.

Suggested change:

 #mcp_server: null
-##  host: "localhost"
+##  host: "0.0.0.0"
 ##  port: 8000

Alternatively, keep "localhost" with a comment explaining it's a development default and users should adjust for Docker deployments.


133-135: Presto configuration example is clear and consistent.

The presto section now includes concrete host and port examples (localhost:8889), which align well with the corresponding user guide updates and the learning context about configurable host/port settings.

docs/src/user-docs/guides-using-presto.md (1)

60-67: Presto configuration guidance is clear and well-structured.

The rewritten instruction effectively guides users to update host and port with actual cluster values while maintaining concrete default examples. The YAML example with localhost:8889 defaults is appropriate and consistent with earlier learnings about Presto configuration. The note clarifying that "replacement is necessary if needed" sets proper expectations.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

@junhaoliao junhaoliao marked this pull request as ready for review November 19, 2025 22:59
@junhaoliao junhaoliao requested a review from a team as a code owner November 19, 2025 22:59
Copy link
Member Author

Choose a reason for hiding this comment

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

@rishikeshdevsot may i get your approval for the modifications in this docs if they look good to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

@davemarco may i get your approval for the modifications in this docs if they look good to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think haiqi originally did not want to put the "default" port since there is no error when starting the package if the port is incorrect. Like he wanted to force the user to input their specific port

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm.. even if we ask users to provide a port, if we don't do any checks (i guess we really can't), there is no guarantee that the port is valid?

fwiw, i believe providing a default helps avoid collision with ports of other services in the CLP package / other well-known ports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants