fix(mcp): resolve incorrect cloud auth help text env vars#880
Conversation
There was a problem hiding this comment.
Pull request overview
This PR corrects environment variable references in cloud authentication help text. The original docstrings incorrectly referenced AIRBYTE_CLIENT_ID, AIRBYTE_CLIENT_SECRET, AIRBYTE_WORKSPACE_ID, and AIRBYTE_API_ROOT, when they should reference AIRBYTE_CLOUD_CLIENT_ID, AIRBYTE_CLOUD_CLIENT_SECRET, AIRBYTE_CLOUD_WORKSPACE_ID, and AIRBYTE_CLOUD_API_ROOT.
Key changes:
- Created centralized
CLOUD_AUTH_TIP_TEXTandWORKSPACE_ID_TIP_TEXTconstants with correct environment variable names - Moved authentication help text from individual function docstrings to the
extra_help_textparameter of the@mcp_tooldecorator - Simplified function docstrings by removing redundant authentication information
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@a/fix(mcp)--resolve-incorrect-cloud-auth-help-text-env-vars' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@a/fix(mcp)--resolve-incorrect-cloud-auth-help-text-env-vars'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughThis PR adds two help-text constants ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Would you like me to scan for any registrations in the file that look like they might have been missed? wdyt? Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
airbyte/mcp/cloud_ops.py (1)
938-973: Should these custom source definition functions also include the auth help text?I noticed that
list_custom_source_definitions,update_custom_source_definition, andpermanently_delete_custom_source_definitiondon't have theextra_help_text=CLOUD_AUTH_TIP_TEXTparameter, while most other cloud operations do. Since these functions also useCloudWorkspaceand require the same authentication, should they be consistent with the rest, wdyt?If you'd like to add it:
@mcp_tool( domain="cloud", read_only=True, idempotent=True, open_world=True, + extra_help_text=CLOUD_AUTH_TIP_TEXT, ) def list_custom_source_definitions(And similarly for the other two functions.
Also applies to: 975-1032, 1034-1099
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/cloud_ops.py(43 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:0-0
Timestamp: 2024-10-08T15:34:31.026Z
Learning: In this codebase, when using `click` for command-line interfaces, detailed help text is provided via `click`'s `help` parameter, and docstrings are kept concise to avoid redundancy.
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:0-0
Timestamp: 2024-10-06T22:13:05.169Z
Learning: In this codebase, when using `click` for command-line interfaces, detailed help text is provided via `click`'s `help` parameter, and docstrings are kept concise to avoid redundancy.
📚 Learning: 2024-10-08T15:34:31.026Z
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-08T15:34:31.026Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.
Applied to files:
airbyte/mcp/cloud_ops.py
⏰ 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). (6)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (3)
airbyte/mcp/cloud_ops.py (3)
183-183: Nice simplification of docstrings!The docstring changes look great. They're now concise while the detailed authentication guidance is properly provided via the
extra_help_textparameter. This aligns perfectly with the codebase pattern of keeping docstrings brief and providing detailed help through decorator parameters.Based on learnings, this approach avoids redundancy between docstrings and help text.
Also applies to: 253-253, 323-323, 375-375, 438-438, 491-491, 553-553, 602-602, 647-647, 683-683, 719-719, 767-767, 829-829, 1313-1313, 1395-1395, 1482-1482
647-659: Good defensive coding with the name property access!The changes to explicitly access and store the
nameproperty before accessing_connector_infolook solid. This ensures the connector info is properly populated before use, which prevents potential attribute errors. Thecastalso helps with type checking clarity.Also applies to: 683-695
944-952: Nice use of the constant for consistency!Using
WORKSPACE_ID_TIP_TEXThere keeps the workspace ID description consistent across the codebase. Great refactor!
Aldo Gonzalez (aldogonzalez8)
left a comment
Aldo Gonzalez (aldogonzalez8)
left a comment
There was a problem hiding this comment.
APPROVED
Aaron ("AJ") Steers (aaronsteers)
deleted the
a/fix(mcp)--resolve-incorrect-cloud-auth-help-text-env-vars
branch
Correct the environment variable references in the cloud authentication help text for improved clarity.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.