Options: Rename CRATEDB_MCP_HTTP_URL to CRATEDB_CLUSTER_URL#21
Conversation
... standardizing on common naming conventions.
WalkthroughThe changes update the environment variable used to configure the CrateDB instance URL from Changes
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
94-103: Consider adding a migration note for existing users.For users who might be currently using the old environment variable name, consider adding a transition note mentioning that
CRATEDB_MCP_HTTP_URLis no longer supported and should be replaced withCRATEDB_CLUSTER_URL.cratedb_mcp/settings.py (1)
6-6: Consider backward compatibility options.Since this is a breaking change for users who have set
CRATEDB_MCP_HTTP_URLin their environment, consider one of these approaches:
- Add a fallback to check the old environment variable if the new one isn't set
- Add a deprecation warning when detecting the old variable
Example implementation with fallback:
-HTTP_URL: str = os.getenv("CRATEDB_CLUSTER_URL", "http://localhost:4200") +HTTP_URL: str = os.getenv("CRATEDB_CLUSTER_URL", os.getenv("CRATEDB_MCP_HTTP_URL", "http://localhost:4200"))Example with deprecation warning:
-HTTP_URL: str = os.getenv("CRATEDB_CLUSTER_URL", "http://localhost:4200") +old_url = os.getenv("CRATEDB_MCP_HTTP_URL") +if old_url: + warnings.warn( + "Environment variable `CRATEDB_MCP_HTTP_URL` is deprecated and will be removed in a future version. " + "Use `CRATEDB_CLUSTER_URL` instead.", + category=DeprecationWarning, + stacklevel=2, + ) +HTTP_URL: str = os.getenv("CRATEDB_CLUSTER_URL", old_url or "http://localhost:4200")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGES.md(1 hunks)README.md(2 hunks)cratedb_mcp/settings.py(1 hunks)
🔇 Additional comments (6)
CHANGES.md (1)
4-5: Changelog entry correctly documents the environment variable renaming.The entry clearly states both the old and new environment variable names and provides context about standardizing naming conventions.
README.md (4)
94-94: Environment variable name consistently updated in documentation.The main configuration instruction has been updated to use the new environment variable name.
99-99: Example for CrateDB Cloud updated to use new environment variable.The export example for cloud connections has been correctly updated to use
CRATEDB_CLUSTER_URL.
102-102: Example for localhost updated to use new environment variable.The export example for localhost connections has been correctly updated to use
CRATEDB_CLUSTER_URL.
132-132: Claude Desktop JSON configuration updated to use new environment variable.The Anthropic Claude Desktop configuration example has been correctly updated to use
CRATEDB_CLUSTER_URL.cratedb_mcp/settings.py (1)
6-6: Environment variable correctly renamed in code.The environment variable has been updated from
CRATEDB_MCP_HTTP_URLtoCRATEDB_CLUSTER_URLwhile maintaining the same default value and variable name, which minimizes impact on the rest of the codebase.
|
@coderabbitai: Thanks for your suggestions about adding backward compatibility or emphasizing adjustments to option names: The project is still in its infancy, and the change log item has all relevant information. We think it is sufficient for this very early alpha state of the project. |
|
Tip For best results, initiate chat on the files or code changes. @amotl Thanks for providing that context! I completely understand your approach - for projects in early alpha stages, maintaining simplicity is often more valuable than implementing backward compatibility. The changelog entry you've included is indeed sufficient for communicating this change to users at this point in the project's lifecycle. I appreciate you sharing your reasoning. 🐰✨ |
kneth
left a comment
There was a problem hiding this comment.
It makes good sense to use the same environment variables. Maybe we should document them?
I've just started using them after renaming another one the other day just recently. The only one that is of general purpose now, is https://cratedb-toolkit.readthedocs.io/search.html?q=CRATEDB_CLUSTER_URL Can we agree on using it across all projects or snippets going forward? /cc @surister, @hlcianfagna, @hammerhead Footnotes
|
About
CrateDB Toolkit started to standardize on common naming conventions. In this case, its Cluster API and many subsystems it supports now understand the
CRATEDB_CLUSTER_URLenvironment variable. This patch also uses the same name here.