feat: add install CLI command for pre-installing connectors#737
feat: add install CLI command for pre-installing connectors#737Aaron ("AJ") Steers (aaronsteers) wants to merge 3 commits into
Conversation
- Add new 'install' command that accepts generic connector arguments - Support all installation methods: pip, docker, local executable, manifest - Include --use-python option for Python interpreter selection - Useful for pre-installing connectors during image build processes - Uses get_connector_executor() directly for efficiency and type-agnostic approach - Follows existing CLI patterns with proper error handling 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/1754025312-add-install-cli-command' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1754025312-add-install-cli-command'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new install CLI command to PyAirbyte that allows pre-installing connectors without running them, which is particularly useful for optimizing container image builds by front-loading installation costs.
- Adds
pyab installcommand with comprehensive connector installation options - Supports all installation methods including pip URLs, Docker images, local executables, and YAML manifests
- Uses
get_connector_executor()directly for type-agnostic connector handling
| docker_image_param: bool | str | None = None | ||
| if docker_image == "true": | ||
| docker_image_param = True | ||
| elif docker_image: | ||
| docker_image_param = docker_image | ||
|
|
||
| source_manifest_param: bool | str | None = None | ||
| if source_manifest == "true": | ||
| source_manifest_param = True | ||
| elif source_manifest: | ||
| source_manifest_param = source_manifest | ||
|
|
||
| use_python_param = _parse_use_python(use_python) | ||
|
|
||
| try: | ||
| executor = get_connector_executor( | ||
| name=connector, | ||
| version=version, | ||
| pip_url=pip_url, | ||
| local_executable=local_executable, | ||
| docker_image=docker_image_param, | ||
| use_host_network=use_host_network, | ||
| source_manifest=source_manifest_param, |
There was a problem hiding this comment.
[nitpick] The variable name docker_image_param is verbose and the pattern of converting string parameters to typed parameters could be extracted into a helper function to reduce code duplication and improve maintainability.
| docker_image_param: bool | str | None = None | |
| if docker_image == "true": | |
| docker_image_param = True | |
| elif docker_image: | |
| docker_image_param = docker_image | |
| source_manifest_param: bool | str | None = None | |
| if source_manifest == "true": | |
| source_manifest_param = True | |
| elif source_manifest: | |
| source_manifest_param = source_manifest | |
| use_python_param = _parse_use_python(use_python) | |
| try: | |
| executor = get_connector_executor( | |
| name=connector, | |
| version=version, | |
| pip_url=pip_url, | |
| local_executable=local_executable, | |
| docker_image=docker_image_param, | |
| use_host_network=use_host_network, | |
| source_manifest=source_manifest_param, | |
| # Use helper function below to parse docker_image and source_manifest | |
| use_python_param = _parse_use_python(use_python) | |
| def parse_bool_or_str(val: str | None) -> bool | str | None: | |
| if val == "true": | |
| return True | |
| elif val: | |
| return val | |
| return None | |
| docker_image = parse_bool_or_str(docker_image) | |
| source_manifest = parse_bool_or_str(source_manifest) | |
| try: | |
| executor = get_connector_executor( | |
| name=connector, | |
| version=version, | |
| pip_url=pip_url, | |
| local_executable=local_executable, | |
| docker_image=docker_image, | |
| use_host_network=use_host_network, | |
| source_manifest=source_manifest, |
| docker_image_param: bool | str | None = None | ||
| if docker_image == "true": | ||
| docker_image_param = True | ||
| elif docker_image: | ||
| docker_image_param = docker_image | ||
|
|
||
| source_manifest_param: bool | str | None = None | ||
| if source_manifest == "true": | ||
| source_manifest_param = True | ||
| elif source_manifest: | ||
| source_manifest_param = source_manifest |
There was a problem hiding this comment.
[nitpick] Similar to docker_image_param, this parameter conversion logic is duplicated. Consider extracting the pattern of converting 'true' string to boolean into a reusable helper function.
| docker_image_param: bool | str | None = None | |
| if docker_image == "true": | |
| docker_image_param = True | |
| elif docker_image: | |
| docker_image_param = docker_image | |
| source_manifest_param: bool | str | None = None | |
| if source_manifest == "true": | |
| source_manifest_param = True | |
| elif source_manifest: | |
| source_manifest_param = source_manifest | |
| docker_image_param: bool | str | None = _parse_bool_or_str(docker_image) | |
| source_manifest_param: bool | str | None = _parse_bool_or_str(source_manifest) |
| executor.install() | ||
| print(f"Connector '{connector}' installed successfully!", file=sys.stderr) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Catching the broad Exception class may hide specific errors that could be handled differently. Consider catching more specific exceptions or at least preserving the original exception type in logs for better debugging.
| except Exception as e: | |
| except Exception as e: | |
| print("An unexpected error occurred during connector installation:", file=sys.stderr) | |
| traceback.print_exc(file=sys.stderr) |
|
Warning Rate limit exceededdevin-ai-integration[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Executor
User->>CLI: airbyte install [options]
CLI->>CLI: Parse and normalize arguments
CLI->>Executor: get_connector_executor(install_if_missing=True)
Executor-->>CLI: Return executor instance
CLI->>Executor: install()
Executor-->>CLI: Installation result
CLI->>Executor: If validate, run spec command
Executor-->>CLI: Spec output or error
CLI->>User: Print success, warning, or error message
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested reviewers
Would you like me to suggest some tests or documentation improvements for this new CLI command, wdyt? ✨ Finishing Touches
🧪 Generate unit tests
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte/cli.py (1)
698-708: Consider improving boolean string normalization consistency - wdyt?The current normalization only handles "true" but not "false" for both
docker_imageandsource_manifest. Should we also handle "false" explicitly to convert it toFalse? This would make the behavior more predictable and consistent.Also, since this pattern is duplicated, would you consider extracting it to a helper function like
_parse_boolean_or_string(value: str | None) -> bool | str | None?+def _parse_boolean_or_string(value: str | None) -> bool | str | None: + """Parse a string that could be 'true', 'false', or an actual value.""" + if value == "true": + return True + elif value == "false": + return False + elif value: + return value + return None + def install( connector: str, version: str | None = None, pip_url: str | None = None, docker_image: str | None = None, local_executable: str | None = None, *, use_host_network: bool = False, source_manifest: str | None = None, use_python: str | None = None, ) -> None: """CLI command to install a connector.""" - docker_image_param: bool | str | None = None - if docker_image == "true": - docker_image_param = True - elif docker_image: - docker_image_param = docker_image - - source_manifest_param: bool | str | None = None - if source_manifest == "true": - source_manifest_param = True - elif source_manifest: - source_manifest_param = source_manifest + docker_image_param = _parse_boolean_or_string(docker_image) + source_manifest_param = _parse_boolean_or_string(source_manifest)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/cli.py(3 hunks)
⏰ 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 (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (4)
airbyte/cli.py (4)
72-72: LGTM on the import addition!The import of
get_connector_executoris correctly placed and necessary for the new install command functionality.
636-685: Comprehensive command definition with good option coverage!The install command options cover all the necessary parameters for different connector installation methods. The help text is clear and follows the established patterns in the codebase.
745-745: Perfect command registration!The install command is properly registered with the CLI group, following the established pattern.
713-734: No action needed—get_connector_executorhandles all passed parameters correctlyI’ve reviewed the implementation in
airbyte/_executors/util.pyand confirmed that:
- The signature matches your call site exactly.
- It enforces mutual exclusivity between
local_executable,docker_image,pip_url/use_python, andsource_manifest.- It applies sane defaults (local executable → manifest → Python → Docker) when no method is specified.
- It validates combinations like
version+pip_urlor taggeddocker_image+version, raisingPyAirbyteInputErroras needed.- It respects
use_host_networkin the Docker branch and honorsinstall_if_missing=Truefor the virtual-env path.All looks good to me—let me know if you’d like a deeper dive! wdyt?
- Add validation option that defaults to True (--validate) - Run spec command after installation to verify connector functionality - Report installation success separately from validation success - Show validation failures as warnings to allow manual debugging - Use executor.execute(['spec']) for validation instead of creating connector object - Fix linting issues with noqa comment for argument count Co-Authored-By: AJ Steers <aj@airbyte.io>
- Replace NotImplementedError with actual Docker image management - Use 'docker pull' for install() to download images - Use 'docker rmi' for uninstall() to remove local images - Add proper error handling and logging - Handle cases where Docker is not installed or images don't exist - Gracefully handle missing images during uninstall Co-Authored-By: AJ Steers <aj@airbyte.io>
feat: add install CLI command for pre-installing connectors
Summary
This PR adds a new
installCLI command to PyAirbyte that allows pre-installing connectors, particularly useful for front-loading installation costs during image build processes. The command accepts generic connector arguments that work for both sources and destinations.Key Features:
pyab installcommand with comprehensive connector installation options--use-pythonoption for Python interpreter selectionget_connector_executor()directly for efficiency and type-agnostic approachExample Usage:
Review & Testing Checklist for Human
Recommended Test Plan:
pyab install --connector=source-hardcoded-recordspyab install --connector=source-hardcoded-records --use-python=truepyab install --connector=source-hardcoded-records --version=999.999.999Diagram
%%{ init : { "theme" : "default" }}%% graph TD CLI["airbyte/cli.py"]:::major-edit Executor["airbyte/_executors/util.py<br/>get_connector_executor()"]:::context VenvExec["airbyte/_executors/python.py<br/>VenvExecutor"]:::context DockerExec["airbyte/_executors/docker.py<br/>DockerExecutor"]:::context CLI -->|"calls directly"| Executor Executor -->|"creates appropriate"| VenvExec Executor -->|"creates appropriate"| DockerExec VenvExec -->|"executor.install()"| InstallVenv["Virtual Environment<br/>Installation"]:::context DockerExec -->|"executor.install()"| InstallDocker["Docker Image<br/>Installation"]:::context subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
get_connector_executor()directly rather thanget_source()/get_destination()for efficiency and type-agnostic supportPyAirbyteInputErrorconsistent with other CLI commandsSession Info:
Summary by CodeRabbit