feat(cloud): add shared Cloud client auth foundation#1027
feat(cloud): add shared Cloud client auth foundation#1027Aaron ("AJ") Steers (aaronsteers) wants to merge 16 commits into
Conversation
🤖 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. 💡 Show Tips and TricksTesting 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/1779160376-cloud-auth-foundation' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1779160376-cloud-auth-foundation'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds YAML-backed Cloud credential resolution, a CloudClient facade and CloudOrganization, refactors CloudWorkspace and MCP to use the new client/credentials, expands cloud package exports, and adds unit tests for credential flows. ChangesCloud Credential & Client Abstraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (1)
airbyte/mcp/cloud.py (1)
1326-1332: 💤 Low valueBoth approaches are functionally equivalent, but consider using the factory method for consistency.
I checked the implementation:
from_explicit_credentials()ultimately calls the direct constructor after wrapping credentials, and the direct constructor's__post_init__does the same wrapping. So there's no behavioral difference between the two approaches.However,
_get_cloud_clientat line 282 usesCloudClient.from_explicit_credentials(...)while this function usesCloudClient(...)directly. Since the factory method is already the pattern used elsewhere in the module, would it make sense to align them for consistency, wdyt?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/mcp/cloud.py` around lines 1326 - 1332, The call creates a CloudClient via its constructor but elsewhere (e.g. _get_cloud_client) the module uses the factory CloudClient.from_explicit_credentials for consistency; change the instantiation to use CloudClient.from_explicit_credentials(client_id=client_id, client_secret=client_secret, bearer_token=bearer_token, public_api_root=api_root, config_api_root=config_api_root).get_organization(organization_id=organization_id, organization_name=organization_name) so the factory path is used (it ultimately wraps credentials the same way) and maintains the module-wide pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@airbyte/mcp/cloud.py`:
- Around line 1326-1332: The call creates a CloudClient via its constructor but
elsewhere (e.g. _get_cloud_client) the module uses the factory
CloudClient.from_explicit_credentials for consistency; change the instantiation
to use CloudClient.from_explicit_credentials(client_id=client_id,
client_secret=client_secret, bearer_token=bearer_token,
public_api_root=api_root,
config_api_root=config_api_root).get_organization(organization_id=organization_id,
organization_name=organization_name) so the factory path is used (it ultimately
wraps credentials the same way) and maintains the module-wide pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20a6be19-03fb-4999-b67b-3be1dfe62dfd
📒 Files selected for processing (10)
airbyte/_util/api_imports.pyairbyte/cloud/__init__.pyairbyte/cloud/connections.pyairbyte/cloud/connectors.pyairbyte/cloud/credentials.pyairbyte/cloud/sync_results.pyairbyte/cloud/workspaces.pyairbyte/constants.pyairbyte/mcp/cloud.pytests/unit_tests/test_cloud_credentials.py
There was a problem hiding this comment.
Pull request overview
This PR extracts and introduces a shared, non-CLI “Cloud/auth foundation” for PyAirbyte Cloud usage, centered around a new authenticated CloudClient plus credential-file/env resolution utilities, and updates MCP + Cloud objects to route through the shared foundation.
Changes:
- Added
airbyte.cloud.CloudClient(co-located withCloudWorkspace) as the authenticated facade for Cloud/self-managed API interactions, including workspace/org discovery helpers. - Added
airbyte.cloud.credentialsfor credentials-file handling, resolution precedence (explicit/env/file), client-credentials login, and logout. - Updated MCP Cloud tooling and Cloud entity wrappers to use shared construction paths and added
get_info()helpers for metadata retrieval.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
airbyte/_util/api_imports.py |
Exposes WorkspaceResponse for typing/public use. |
airbyte/cloud/__init__.py |
Exports CloudClient from the Cloud package surface. |
airbyte/cloud/connections.py |
Adds CloudConnection.get_info() convenience wrapper. |
airbyte/cloud/connectors.py |
Adds CloudConnector.get_info() convenience wrapper. |
airbyte/cloud/credentials.py |
New credential resolution + credentials-file login/logout utilities. |
airbyte/cloud/sync_results.py |
Adds SyncResult.get_info() convenience wrapper. |
airbyte/cloud/workspaces.py |
Adds CloudClient; refactors CloudWorkspace to shared credential resolution and adds workspace/job helpers. |
airbyte/constants.py |
Adds AIRBYTE_CLOUD_ORGANIZATION_ID env var constant. |
airbyte/mcp/cloud.py |
Routes MCP Cloud client/workspace/org construction through CloudClient. |
tests/unit_tests/test_cloud_credentials.py |
Adds unit coverage for non-interactive credentials login writing token file. |
Comments suppressed due to low confidence (4)
airbyte/cloud/workspaces.py:311
CloudClient.get_organization()raisesAirbyteMissingResourceError(resource_type="organization")without including the requestedorganization_id/organization_nameinresource_name_or_idorcontext. This makes the error much less actionable (and loses detail compared to the previous implementation inairbyte/mcp/cloud.py). Consider including the identifier that was searched for (and possibly a guidance message) when raising.
if organization_id:
matching_organizations = [
organization
for organization in organizations
if organization.organization_id == organization_id
]
else:
matching_organizations = [
organization
for organization in organizations
if organization.organization_name == organization_name
]
if not matching_organizations:
raise AirbyteMissingResourceError(resource_type="organization")
if len(matching_organizations) > 1:
raise exc.PyAirbyteInputError(
message="Organization name matches multiple organizations."
)
airbyte/cloud/workspaces.py:243
CloudClient.list_workspaces()callsapi_util.list_workspaces(workspace_id="", ...). Whileworkspace_idis currently only used for error context inapi_util, passing an empty string can lead to confusing error output. Consider updating the wrapper to pass a meaningful value (or adjustingapi_util.list_workspacesto not require a workspace_id) so error context remains useful.
def list_workspaces(
self,
name: str | None = None,
*,
name_filter: Callable[[str], bool] | None = None,
) -> list[api_imports.WorkspaceResponse]:
"""List workspaces available to this client."""
return api_util.list_workspaces(
workspace_id="",
api_root=self.public_api_root,
name=name,
name_filter=name_filter,
client_id=self.client_id,
client_secret=self.client_secret,
bearer_token=self.bearer_token,
)
airbyte/cloud/workspaces.py:1070
CloudWorkspace.list_workspaces()typesname_filteras plainCallable | None, whileCloudClient.list_workspaces()usesCallable[[str], bool] | None. Tightening the annotation here to match (and to document the expected predicate signature) would improve usability and static checking.
def list_workspaces(
self,
name: str | None = None,
*,
name_filter: Callable | None = None,
) -> list[WorkspaceResponse]:
airbyte/cloud/workspaces.py:605
- The behavior of
from_env()now depends onresolve_cloud_credentials()(via__init__), which raisesPyAirbyteInputErrorwhen credentials are missing/partial. The docstring above still states it raisesPyAirbyteSecretNotFoundError; please update the documented exception type(s) to match the new shared-resolution behavior so callers can catch the right error.
return cls(
workspace_id=workspace_id,
api_root=api_root,
config_api_root=config_api_root,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
airbyte/cloud/_credentials.py (2)
144-150: 💤 Low valueSilent exception swallowing in
read_filemay hide real issues.Catching
OSErrorandyaml.YAMLErrorand returning an empty dict silently could mask legitimate problems (permission errors, corrupted YAML, etc.). Would it be worth at least logging a warning here so users have some visibility into credential file read failures, wdyt?try: content = credentials_file_path.read_text(encoding="utf-8").strip() parsed = yaml.safe_load(content) if content else {} except (OSError, yaml.YAMLError): + # Consider: logging.warning("Failed to read credentials file: %s", credentials_file_path) return {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/cloud/_credentials.py` around lines 144 - 150, The current exception handling in the credentials file reader swallows OSError and yaml.YAMLError and returns {} silently; modify the try/except in the read routine so that it catches exceptions as e and emits a warning including the exception message and the credentials file path (use the module logger or logging.warning) before returning {} to preserve behavior; reference the credentials_file_path.read_text call, yaml.safe_load usage, and the _as_string_mapping return so you only add logging inside that except block without changing subsequent return logic.
28-35: 💤 Low valueDuplicate environment variable constants?
These constants duplicate those imported from
airbyte.constantson lines 12-22. Are these intentionally different (e.g.,AIRBYTE_CLIENT_IDvsAIRBYTE_CLOUD_CLIENT_ID), or could we consolidate them to avoid drift, wdyt?Looking at the usage in
_env_valuecalls (e.g., line 81), both sets are checked as fallbacks, which makes sense for backward compatibility. If that's the intent, maybe a brief comment explaining this dual-env-var strategy would help future readers?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/cloud/_credentials.py` around lines 28 - 35, The file defines duplicated env var constants (CREDENTIALS_FILE_PATH, CLIENT_ID_ENV_VAR, CLIENT_SECRET_ENV_VAR, WORKSPACE_ID_ENV_VAR, ORGANIZATION_ID_ENV_VAR, PUBLIC_API_ROOT_ENV_VAR, BEARER_TOKEN_ENV_VAR, CONFIG_API_ROOT_ENV_VAR) that mirror those in airbyte.constants and are used as fallbacks via _env_value; add a brief clarifying comment above these constants explaining that this file intentionally defines legacy and new env var names (and is checked as a fallback by _env_value) for backward compatibility, or alternatively consolidate to import the canonical names from airbyte.constants and remove duplicates—update references to use the canonical symbols (or keep both with the comment) to avoid future drift.airbyte/cloud/organizations.py (1)
65-67: 💤 Low valueBare
except Exceptionis quite broad.This catches all exceptions including ones that might indicate real bugs (e.g.,
TypeErrorfrom bad API response parsing). Would it be worth narrowing this to specific expected exceptions (likeAirbyteErroror network-related exceptions), or at least logging the caught exception for debugging purposes, wdyt?try: self._organization_info = api_util.get_organization_info( organization_id=self.organization_id, api_root=self._credentials.public_api_root, config_api_root=self._credentials.config_api_root, client_id=self._credentials.client_id, client_secret=self._credentials.client_secret, bearer_token=self._credentials.bearer_token, ) - except Exception: + except (AirbyteError, ConnectionError, TimeoutError): self._organization_info_fetch_failed = True return {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/cloud/organizations.py` around lines 65 - 67, Replace the bare except in the organization info fetch block so it only handles expected errors (e.g., AirbyteError, requests.ConnectionError, requests.Timeout) instead of catching all Exception; also log the caught exception before setting self._organization_info_fetch_failed and returning {} to aid debugging. Locate the try/except around the code that sets self._organization_info_fetch_failed (the method that fetches organization info) and change the except Exception to a narrower exception tuple and call the module/class logger to record the exception details along with a clear message before returning the empty dict.airbyte/cloud/client.py (2)
238-244: ⚡ Quick winRedundant validation -
resolved_organization_idwill always be truthy here.If we enter this block (line 238 condition is true), then either
organization_idorself.organization_idis truthy. Sinceresolved_organization_id = organization_id or self.organization_id, it will always be truthy, making the check at lines 240-244 unreachable dead code. Could we simplify by removing the inner validation, wdyt?if organization_id is not None or self.organization_id is not None: resolved_organization_id = organization_id or self.organization_id - if not resolved_organization_id: - raise exc.PyAirbyteInputError( - message="Organization ID is required.", - guidance="Provide an organization ID.", - ) + assert resolved_organization_id # Guaranteed by outer condition return api_util.list_workspaces_in_organization(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/cloud/client.py` around lines 238 - 244, The conditional inside the block that checks resolved_organization_id is redundant because the outer if already guarantees organization_id or self.organization_id is truthy; remove the inner falsy check and the raise of exc.PyAirbyteInputError, and simply set resolved_organization_id = organization_id or self.organization_id (keeping the outer if condition as-is) to simplify the logic in the function that handles organization_id resolution.
49-58: 💤 Low valueMinor:
any([...])creates an unnecessary list.Using
any(...)with a generator expression is slightly more efficient thanany([...])with a list. Small nit, but since you're here anyway, wdyt?if credentials and any( - [ - client_id, - client_secret, - bearer_token, - public_api_root, - config_api_root, - organization_id, - ] + ( + client_id, + client_secret, + bearer_token, + public_api_root, + config_api_root, + organization_id, + ) ):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/cloud/client.py` around lines 49 - 58, The conditional uses any([...]) which builds an unnecessary list; change the check in client.py (the if that references credentials and any([...]) containing client_id, client_secret, bearer_token, public_api_root, config_api_root, organization_id) to use a generator/tuple expression instead (e.g., any((client_id, client_secret, bearer_token, public_api_root, config_api_root, organization_id)) or any(x for x in (...))) so no intermediate list is allocated.airbyte/cloud/workspaces.py (1)
664-679: 💤 Low value
list_workspacesonCloudWorkspaceduplicatesCloudClient.list_workspaces.This method lists workspaces available to the credentials, which isn't really workspace-specific behavior. It's also available on
CloudClient. Is this intentional for backward compatibility or convenience? If so, a brief docstring note mentioningCloudClient.list_workspacesas the canonical location might help, wdyt?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/cloud/workspaces.py` around lines 664 - 679, The CloudWorkspace.list_workspaces method duplicates behavior provided by CloudClient.list_workspaces; update the CloudWorkspace.list_workspaces docstring to state that this is provided for convenience/backward-compatibility and point callers to CloudClient.list_workspaces as the canonical API, e.g., mention CloudClient.list_workspaces and when to prefer it, and keep the existing implementation unchanged to preserve compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@airbyte/cloud/workspaces.py`:
- Around line 121-142: Remove the redundant early assignment to
self.config_api_root — it's overwritten by the value from
_AirbyteCredentials.from_auth. In the initializer that calls
_AirbyteCredentials.from_auth (the block creating credentials and then setting
self.workspace_id, self.client_id, etc.), delete the initial
self.config_api_root = config_api_root line and rely on
credentials.config_api_root when assigning self.config_api_root after
credentials are created.
---
Nitpick comments:
In `@airbyte/cloud/_credentials.py`:
- Around line 144-150: The current exception handling in the credentials file
reader swallows OSError and yaml.YAMLError and returns {} silently; modify the
try/except in the read routine so that it catches exceptions as e and emits a
warning including the exception message and the credentials file path (use the
module logger or logging.warning) before returning {} to preserve behavior;
reference the credentials_file_path.read_text call, yaml.safe_load usage, and
the _as_string_mapping return so you only add logging inside that except block
without changing subsequent return logic.
- Around line 28-35: The file defines duplicated env var constants
(CREDENTIALS_FILE_PATH, CLIENT_ID_ENV_VAR, CLIENT_SECRET_ENV_VAR,
WORKSPACE_ID_ENV_VAR, ORGANIZATION_ID_ENV_VAR, PUBLIC_API_ROOT_ENV_VAR,
BEARER_TOKEN_ENV_VAR, CONFIG_API_ROOT_ENV_VAR) that mirror those in
airbyte.constants and are used as fallbacks via _env_value; add a brief
clarifying comment above these constants explaining that this file intentionally
defines legacy and new env var names (and is checked as a fallback by
_env_value) for backward compatibility, or alternatively consolidate to import
the canonical names from airbyte.constants and remove duplicates—update
references to use the canonical symbols (or keep both with the comment) to avoid
future drift.
In `@airbyte/cloud/client.py`:
- Around line 238-244: The conditional inside the block that checks
resolved_organization_id is redundant because the outer if already guarantees
organization_id or self.organization_id is truthy; remove the inner falsy check
and the raise of exc.PyAirbyteInputError, and simply set
resolved_organization_id = organization_id or self.organization_id (keeping the
outer if condition as-is) to simplify the logic in the function that handles
organization_id resolution.
- Around line 49-58: The conditional uses any([...]) which builds an unnecessary
list; change the check in client.py (the if that references credentials and
any([...]) containing client_id, client_secret, bearer_token, public_api_root,
config_api_root, organization_id) to use a generator/tuple expression instead
(e.g., any((client_id, client_secret, bearer_token, public_api_root,
config_api_root, organization_id)) or any(x for x in (...))) so no intermediate
list is allocated.
In `@airbyte/cloud/organizations.py`:
- Around line 65-67: Replace the bare except in the organization info fetch
block so it only handles expected errors (e.g., AirbyteError,
requests.ConnectionError, requests.Timeout) instead of catching all Exception;
also log the caught exception before setting
self._organization_info_fetch_failed and returning {} to aid debugging. Locate
the try/except around the code that sets self._organization_info_fetch_failed
(the method that fetches organization info) and change the except Exception to a
narrower exception tuple and call the module/class logger to record the
exception details along with a clear message before returning the empty dict.
In `@airbyte/cloud/workspaces.py`:
- Around line 664-679: The CloudWorkspace.list_workspaces method duplicates
behavior provided by CloudClient.list_workspaces; update the
CloudWorkspace.list_workspaces docstring to state that this is provided for
convenience/backward-compatibility and point callers to
CloudClient.list_workspaces as the canonical API, e.g., mention
CloudClient.list_workspaces and when to prefer it, and keep the existing
implementation unchanged to preserve compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 070615ff-a9e9-4660-9993-b221eb997554
📒 Files selected for processing (7)
airbyte/cloud/__init__.pyairbyte/cloud/_credentials.pyairbyte/cloud/client.pyairbyte/cloud/organizations.pyairbyte/cloud/workspaces.pyairbyte/mcp/cloud.pytests/unit_tests/test_cloud_credentials.py
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte/mcp/cloud.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit_tests/test_cloud_credentials.py (2)
47-49: ⚡ Quick winConsider asserting the bearer token in the result as well?
The test verifies the bearer token was persisted to the YAML file (line 52), but doesn't check whether
resultitself exposes the bearer token. IfCloudLoginResulthas abearer_tokenfield, assertingresult.bearer_token == "test-bearer-token"would provide more complete coverage of the login flow's return value. Wdyt?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_cloud_credentials.py` around lines 47 - 49, The test currently asserts paths and API roots but doesn't verify the returned CloudLoginResult includes the bearer token; update the unit test that constructs/receives result (the CloudLoginResult instance) to also assert result.bearer_token == "test-bearer-token" (or the expected token string) so the login flow's return value is fully validated, and ensure the CloudLoginResult type exposes the bearer_token attribute before adding the assertion.
161-168: ⚡ Quick winVerify that the fake_try_get_secret signature matches the actual implementation?
The mocked function signature uses
secret_nameas positional-only,defaultas keyword, and**_for remaining kwargs. To ensure this test accurately reflects real behavior, could you confirm this matches the actualtry_get_secretsignature fromairbyte.secrets? Wdyt about running a quick check?#!/bin/bash # Description: Verify the signature of try_get_secret to ensure test mock matches reality # Search for try_get_secret definition ast-grep --pattern $'def try_get_secret($$$) $$$'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_cloud_credentials.py` around lines 161 - 168, The test's fake_try_get_secret signature may not match the real try_get_secret from airbyte.secrets; import try_get_secret and inspect.signature(try_get_secret) to confirm the exact parameters, then update fake_try_get_secret to match (same positional/keyword/varargs/kwonly layout) or simplify to accept identical *args/**kwargs and the same parameter names so the mock accurately mirrors try_get_secret's API (adjust the fake function in test_cloud_credentials.py accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit_tests/test_cloud_credentials.py`:
- Around line 47-49: The test currently asserts paths and API roots but doesn't
verify the returned CloudLoginResult includes the bearer token; update the unit
test that constructs/receives result (the CloudLoginResult instance) to also
assert result.bearer_token == "test-bearer-token" (or the expected token string)
so the login flow's return value is fully validated, and ensure the
CloudLoginResult type exposes the bearer_token attribute before adding the
assertion.
- Around line 161-168: The test's fake_try_get_secret signature may not match
the real try_get_secret from airbyte.secrets; import try_get_secret and
inspect.signature(try_get_secret) to confirm the exact parameters, then update
fake_try_get_secret to match (same positional/keyword/varargs/kwonly layout) or
simplify to accept identical *args/**kwargs and the same parameter names so the
mock accurately mirrors try_get_secret's API (adjust the fake function in
test_cloud_credentials.py accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fb26d45-7816-41c1-9cbb-d903a21022f0
📒 Files selected for processing (4)
airbyte/cloud/_credentials.pyairbyte/cloud/client.pyairbyte/cloud/workspaces.pytests/unit_tests/test_cloud_credentials.py
| self, | ||
| name: str, | ||
| source: Source, | ||
| source: Source | dict[str, Any] | None = None, |
There was a problem hiding this comment.
I don't know that we want to make this change here in this PR. And an alternate approach, if we only need source_name would be allow source as a string or a Source object, which I think is cleaner.
There was a problem hiding this comment.
Addressed in 5244853: restored CloudWorkspace.deploy_source() to accept a Source object only and removed the dict / config input path from this split PR. I also called out the broader dict/string source-input API as deferred in the PR description.
| @staticmethod | ||
| def _credentials_from_auth( | ||
| *, | ||
| organization_id: str | None = None, | ||
| client_id: str | SecretString | None = None, | ||
| client_secret: str | SecretString | None = None, | ||
| bearer_token: str | SecretString | None = None, | ||
| public_api_root: str | None = None, | ||
| config_api_root: str | None = None, | ||
| credentials_file_path: Path = CREDENTIALS_FILE_PATH, | ||
| ) -> _AirbyteCredentials: | ||
| """Create resolved Cloud credentials from explicit inputs.""" | ||
| return _AirbyteCredentials.from_auth( | ||
| organization_id=organization_id, | ||
| client_id=client_id, | ||
| client_secret=client_secret, | ||
| bearer_token=bearer_token, | ||
| public_api_root=public_api_root, | ||
| config_api_root=config_api_root, | ||
| credentials_file_path=credentials_file_path, | ||
| ) |
There was a problem hiding this comment.
This doesn't look like it belongs on this class and doesn't add any value here. Suggest deletion.
There was a problem hiding this comment.
Addressed in 5244853: deleted the passthrough _credentials_from_auth() helper and now call _AirbyteCredentials.from_auth() directly from CloudClient.from_env() and CloudClient.from_auth().
| return cls._new_from_credentials(credentials) | ||
|
|
||
| @classmethod | ||
| def _new_from_credentials(cls, credentials: _AirbyteCredentials) -> CloudClient: |
There was a problem hiding this comment.
The word 'new' is implied. You can simply rename to _from_credentials
There was a problem hiding this comment.
Addressed in 5244853: renamed the internal constructor helper from _new_from_credentials() to _from_credentials().
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@airbyte/cloud/client.py`:
- Around line 244-254: The non-organization branch that sets name =
name_contains and then calls api_util.list_workspaces does not pass through the
limit argument, so enforce the same limiting behavior by adding limit=limit to
the api_util.list_workspaces call; update the call site that currently passes
workspace_id, api_root, name, name_filter, client_id, client_secret,
bearer_token to also include limit=limit (ensure the surrounding method where
name_contains/name/name_filter are defined has access to the limit parameter).
- Around line 227-243: The organization branch currently returns
api_util.list_workspaces_in_organization directly so the name_filter (and the
name/name_contains logic) is ignored; update the organization-scoped path in the
method that references organization_id/self.organization_id to either pass the
appropriate name/name_contains argument into
api_util.list_workspaces_in_organization (e.g., use name_contains=name_filter or
name_contains=name or name_contains=name_filter or name) or call
api_util.list_workspaces_in_organization and then apply the existing name_filter
predicate to the returned list before returning, ensuring the same filtering
behavior as the non-organization branch; refer to organization_id,
api_util.list_workspaces_in_organization, name_contains, name, and name_filter
when making the change.
In `@airbyte/cloud/organizations.py`:
- Around line 58-75: The code currently sets
_organization_info_fetch_failed=True on any exception from
api_util.get_organization_info which permanently short-circuits future lookups;
instead, on exception in the method that calls api_util.get_organization_info
(the block that reads/sets self._organization_info), preserve and return the
last-known self._organization_info if it exists and avoid flipping the permanent
_organization_info_fetch_failed flag; only set the failure flag when there is no
prior cached self._organization_info (or implement a retry/timestamp-based
backoff) and ensure force_refresh still triggers a fetch attempt that can clear
the flag—update the exception handling around get_organization_info, referencing
_organization_info_fetch_failed, _organization_info, and get_organization_info
to implement this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ed12fd7-4845-48ec-99fa-137bf98e8291
📒 Files selected for processing (3)
airbyte/cloud/client.pyairbyte/cloud/organizations.pyairbyte/cloud/workspaces.py
|
Reverting to draft (on hold), as we are pivoting to first an internal CLI app launch. |
|
Closing. Stale. Migrated to ops repo CLI (internal). |
Summary
AJ asked for this split so the non-CLI Cloud/auth foundation from #1010 can merge ahead of the CLI additions.
This PR is intentionally non-stacked against
mainand includes only shared PyAirbyte Cloud/auth code:airbyte.cloud.CloudClientas the shared authenticated Cloud/self-managed API facade inairbyte/cloud/client.py.airbyte.cloud.CloudOrganizationinairbyte/cloud/organizations.py._AirbyteCredentialsclass in privateairbyte.cloud._credentials, used internally byCloudClient,CloudWorkspace, andCloudOrganization._AirbyteCredentialstype; internal credential injection uses private factory methods._AirbyteCredentialswhile keeping login/logout helpers private toairbyte.cloud._credentials.airbyte.cloud._credentialshelpers for credential-file handling, explicit/env/secret-manager/credentials-file resolution, client-credentials login, logout, bearer-token auth, and optionalorganization_idsupport.CloudWorkspaceto resolve credentials through shared Cloud credential logic while preserving keyword-only construction.CloudWorkspace.deploy_source()scoped to deploying aSource; broader dict/string source deployment API changes are deferred.Included files:
airbyte/_util/api_imports.pyairbyte/cloud/__init__.pyairbyte/cloud/_credentials.pyairbyte/cloud/client.pyairbyte/cloud/organizations.pyairbyte/cloud/workspaces.pyairbyte/constants.pyairbyte/mcp/cloud.pytests/unit_tests/test_cloud_credentials.pyExplicitly excluded from this split:
airbyte/cli/**Cyclopts CLI entrypoints and command modules.airbyte-cloud/airbyte-localconsole-script changes inpyproject.toml.uv.lockdependency changes from PR 1010.CloudConnection,CloudConnector, andSyncResult.CloudWorkspace.deploy_source()API changes for dict/string source inputs.Review & Testing Checklist for Human
CloudClient/CloudWorkspace/CloudOrganizationconstruction shape matches what MCP and future CLI consumers should share._AirbyteCredentialsapproach is the preferred visibility boundary before PR 1010 rebases on this.airbyte-cloudandairbyte-localCLIs #1010 onmainand remove any duplicate Cloud/auth foundation changes from that branch.Notes
Validation run locally after latest review updates:
uv --directory /home/ubuntu/repos/PyAirbyte run ruff format --check airbyte/cloud/client.py airbyte/cloud/workspaces.py— passed.uv --directory /home/ubuntu/repos/PyAirbyte run ruff check airbyte/cloud/client.py airbyte/cloud/workspaces.py airbyte/mcp/cloud.py tests/unit_tests/test_cloud_credentials.py tests/unit_tests/test_mcp_cloud.py— passed.uv --directory /home/ubuntu/repos/PyAirbyte run pyrefly check— completed with 0 errors and 2 warnings about existing redundant casts inairbyte/mcp/cloud.py.uv --directory /home/ubuntu/repos/PyAirbyte run pytest tests/unit_tests/test_cloud_credentials.py tests/unit_tests/test_mcp_cloud.py tests/unit_tests/test_cloud_api_util.py tests/unit_tests/test_cloud_api_roots.py tests/integration_tests/cloud/test_cloud_api_util.py tests/integration_tests/cloud/test_cloud_workspaces.py— passed: 47 passed, 1 skipped. The skipped test is an existing flaky Cloud workspace test linked to https://github.com/airbytehq/airbyte-internal-issues/issues/15502.CI status after the latest push: pending.
Preview docs deployment: https://vercel.com/airbyte-growth/pyairbyte-api-docs/4uG5sch9xeGcHH9U4VYyw9Vzcnmk
Link to Devin session: https://app.devin.ai/sessions/b97ebb27aa72481d982ebb1b9131b687
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit
Release Notes
New Features
CloudClientfor authenticated Airbyte Cloud API access with improved credentials managementTests