feat(cloud): Add CloudWorkspace.from_env() classmethod#909
Conversation
…construction Co-Authored-By: AJ Steers <aj@airbyte.io>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 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@devin/1765488100-cloudworkspace-from-env' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1765488100-cloudworkspace-from-env'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
airbyte/cloud/workspaces.py (2)
116-121: Consider documenting all required environment variables in the docstring.The docstring mentions
AIRBYTE_CLOUD_WORKSPACE_IDandAIRBYTE_CLOUD_API_URL, butAIRBYTE_CLOUD_CLIENT_IDandAIRBYTE_CLOUD_CLIENT_SECRETare also required (they're resolved without defaults in lines 141-142). Including all four env vars in the Args section would give users a complete picture of what they need to set, wdyt?Something like:
Args: workspace_id: The workspace ID. If not provided, will be resolved from the `AIRBYTE_CLOUD_WORKSPACE_ID` environment variable. + client_id: The client ID will be resolved from the + `AIRBYTE_CLOUD_CLIENT_ID` environment variable. + client_secret: The client secret will be resolved from the + `AIRBYTE_CLOUD_CLIENT_SECRET` environment variable. api_root: The API root URL. If not provided, will be resolved from the `AIRBYTE_CLOUD_API_URL` environment variable, or default to the Airbyte Cloud API.
103-144: Consider adding unit tests for the new classmethod.The PR checklist notes that no unit tests were added. Since this is a new public API method, adding tests would help ensure the environment variable resolution works correctly and document the expected behavior. This could include tests for:
- Successful creation when all env vars are set
- Error handling when required env vars are missing
- Precedence when both env vars and explicit args are provided
Would you like me to help draft some unit tests for this method?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/cloud/workspaces.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/cloud/workspaces.py (2)
airbyte/cloud/auth.py (4)
resolve_cloud_api_url(25-33)resolve_cloud_client_id(17-22)resolve_cloud_client_secret(9-14)resolve_cloud_workspace_id(36-41)tests/integration_tests/cloud/conftest.py (1)
workspace_id(49-50)
⏰ 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 (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (1)
airbyte/cloud/workspaces.py (1)
48-53: LGTM! Clean imports for environment resolution.The imports are well-organized and all four resolver functions are used in the new
from_envclassmethod below.
There was a problem hiding this comment.
Pull request overview
This PR adds a from_env() classmethod to CloudWorkspace that enables creating workspace instances from environment variables, providing a more ergonomic alternative to explicitly passing credentials while maintaining backward compatibility with the strict constructor.
Key Changes:
- Added
from_env()classmethod that resolves credentials from environment variables - Imported credential resolution functions from
airbyte.cloud.auth - Comprehensive docstring with usage examples and error documentation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
airbyte/cloud/workspaces.py (1)
103-151: Consider adding a small test suite aroundfrom_envbehavior?Since this is now a primary ergonomic entrypoint, would it be worth adding a few tests to lock in behavior—for example: (1) happy path when all four env vars are set, (2) failure when required secrets are missing, and (3) precedence when both an explicit
workspace_idandAIRBYTE_CLOUD_WORKSPACE_IDare present? That could help prevent regressions as the auth helpers evolve over time, even if you’re manually validating today. Wdyt?
🧹 Nitpick comments (1)
airbyte/cloud/workspaces.py (1)
103-151: Clarify env‑vs‑argument precedence andSecretStringhandling infrom_env?This factory looks great for ergonomics; a couple of small clarifications might help future readers:
Because the
resolve_cloud_*functions take aninput_valuefallback, it seems like env vars will generally win over the explicitworkspace_id/api_rootarguments when both are present, whereas the current docstring reads as if the arguments are primary and env vars are only used when args are omitted. Would it be worth tweaking the docstring to explicitly state the precedence (e.g., “env vars are used if set; arguments act as fallbacks when the env var is missing”), and to call out thatAIRBYTE_CLOUD_WORKSPACE_IDis effectively required unless you passworkspace_id? Wdyt?
resolve_cloud_client_id/resolve_cloud_client_secretalready returnSecretString, and__post_init__wrapsself.client_id/self.client_secretinSecretStringagain. Depending on howSecretStringis implemented, double‑wrapping could be a no‑op or could accidentally wrap a masked string. Would it make sense to either (a) guard the wrapping with anisinstance(..., SecretString)check in__post_init__, or (b) havefrom_envpass the underlying raw value instead, so we only wrap once? Could you confirm which behavior is safest with the currentSecretStringimplementation?
Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/cloud/workspaces.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-11T22:05:15.550Z
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:26-26
Timestamp: 2024-10-11T22:05:15.550Z
Learning: In the PyAirbyte project, when reviewing Python code and encountering a TODO comment without an issue link, I should post a friendly reminder to resolve it before merging, instead of suggesting to add an issue link.
Applied to files:
airbyte/cloud/workspaces.py
📚 Learning: 2024-10-06T23:44:31.534Z
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
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/cloud/workspaces.py
🧬 Code graph analysis (1)
airbyte/cloud/workspaces.py (2)
airbyte/cloud/auth.py (4)
resolve_cloud_api_url(25-33)resolve_cloud_client_id(17-22)resolve_cloud_client_secret(9-14)resolve_cloud_workspace_id(36-41)tests/integration_tests/cloud/conftest.py (1)
workspace_id(49-50)
⏰ 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 (All, Python 3.11, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (1)
airbyte/cloud/workspaces.py (1)
48-53: Imports cleanly align with the new env‑resolver usage.Using the dedicated
resolve_cloud_*helpers here keepsCloudWorkspaceconsistent with the rest of the cloud auth surface, and the imported symbols match whatfrom_envactually uses. Looks good to me.
Summary
Adds a
CloudWorkspace.from_env()classmethod that creates a workspace instance by resolving credentials from environment variables. This provides an ergonomic alternative to the constructor while keeping the original constructor strict about requiring explicit credentials.This addresses a pattern seen in airbyte-ops-mcp where callers were attempting to construct
CloudWorkspacewithout credentials, expecting env var resolution.Review & Testing Checklist for Human
from_env()methodAIRBYTE_CLOUD_WORKSPACE_ID,AIRBYTE_CLOUD_API_URL) match the actual constants inairbyte/constants.pyCloudWorkspace.from_env()to verify it works end-to-endTest Plan
AIRBYTE_CLOUD_CLIENT_ID,AIRBYTE_CLOUD_CLIENT_SECRET, andAIRBYTE_CLOUD_WORKSPACE_IDenv varsCloudWorkspace.from_env()and verify it returns a valid workspaceworkspace.connect()to verify credentials workNotes
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.