feat: Add InstallType.ANY and InstallType.INSTALLABLE enum values #864
Conversation
- Add InstallType.ANY and InstallType.INSTALLABLE to the InstallType enum - Update get_available_connectors() to handle new enum values - Replace InstallType.DOCKER with InstallType.INSTALLABLE in get_connector_api_docs_urls() and get_connector_version_history() - Consolidate DOCKER, ANY, and INSTALLABLE handling to reduce return statements (fixes PLR0911 lint error) - No behavior changes, just making code more maintainable and future-proof Co-Authored-By: AJ Steers <[email protected]>
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/1762981002-add-installtype-any-installable' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1762981002-add-installtype-any-installable'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughExpanded the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Registry as airbyte/registry.py
Note over Registry: install_type ∈ {DOCKER, ANY, INSTALLABLE, PYTHON, YAML, JAVA}
Client->>Registry: get_available_connectors(install_type)
alt install_type == DOCKER or install_type == ANY
Registry-->>Client: return all connectors (includes docker-enabled)
else alt install_type == INSTALLABLE
Registry->>Registry: detect Docker availability
alt Docker present
Registry-->>Client: return all connectors
else Docker absent
Registry-->>Client: return only PYTHON and YAML-only connectors
end
else
Registry-->>Client: return connectors filtered by install_type
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 0
🧹 Nitpick comments (1)
airbyte/registry.py (1)
55-56: Consider adding docstrings to clarify the semantic intent of these new enum values?While the names
ANYandINSTALLABLEare fairly self-descriptive, adding brief docstrings would help clarify when to use each versusDOCKER(which also returns all connectors currently). Something like:class InstallType(str, Enum): """The type of installation for a connector.""" YAML = "yaml" PYTHON = "python" DOCKER = "docker" JAVA = "java" - ANY = "any" - INSTALLABLE = "installable" + ANY = "any" # Returns all connectors regardless of install type + INSTALLABLE = "installable" # Returns all installable connectors (semantic clarification of DOCKER)This would help future developers understand the subtle semantic differences, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/registry.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/registry.py (1)
airbyte/exceptions.py (1)
AirbyteConnectorNotRegisteredError(288-297)
⏰ 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, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (3)
airbyte/registry.py (3)
274-275: Nice consolidation that improves maintainability!Grouping
DOCKER,ANY, andINSTALLABLEinto a single check makes the code cleaner and addresses the PLR0911 lint issue. The logic correctly reflects that all three enum values should return the complete set of connectors.
450-457: Good semantic clarification usingINSTALLABLEinstead ofDOCKER!The change better expresses the intent of this validation: checking whether the connector is available/installable in the registry, rather than specifically checking for Docker availability. Since this function retrieves documentation URLs from multiple sources (registry metadata and manifest files), the more general "installable" concept is appropriate here.
510-517: Consistent semantic improvement here as well!Like the change in
get_connector_api_docs_urls(), usingINSTALLABLEinstead ofDOCKERbetter expresses that this function is checking for connector availability in the registry rather than Docker-specific functionality. The consistency across both functions strengthens the clarity of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
airbyte/registry.py (1)
55-56: Consider adding docstrings to clarify the semantic distinction between ANY and INSTALLABLE?While the enum values are defined, their specific purposes aren't documented. Based on the PR description,
ANYreturns all connectors regardless of install type, whileINSTALLABLEclarifies semantics for installable connectors. Would it help to add inline comments or docstrings to make this distinction clear for future developers? WDYT?Example:
DOCKER = "docker" JAVA = "java" - ANY = "any" - INSTALLABLE = "installable" + ANY = "any" # Returns all connectors regardless of install type + INSTALLABLE = "installable" # Returns all installable connectors (semantic clarification of DOCKER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/registry.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/registry.py (1)
airbyte/exceptions.py (1)
AirbyteConnectorNotRegisteredError(288-297)
⏰ 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, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (2)
airbyte/registry.py (2)
450-457: Good semantic improvement usingInstallType.ANYhere.The change from
InstallType.DOCKERtoInstallType.ANYclarifies that this function is checking for connector existence across all install types, not specifically docker-installable connectors. This aligns well with the function's purpose of retrieving documentation URLs regardless of how the connector is installed.
510-517: Consistent semantic improvement here as well.Just like in
get_connector_api_docs_urls(), usingInstallType.ANYinstead ofInstallType.DOCKERbetter reflects the function's intent to retrieve version history for any connector regardless of install type. The consistency across both functions is good!
- Add explicit handler for InstallType.INSTALLABLE that mirrors the None behavior - INSTALLABLE is environment-sensitive: returns all connectors if Docker is installed, otherwise only Python and YAML connectors - ANY is environment-independent: always returns all connectors - Add comprehensive docstrings to InstallType enum to clarify semantics - Refactor get_available_connectors() to reduce return statements (fixes PLR0911) - Consolidate None and INSTALLABLE handlers to use single return statements Co-Authored-By: AJ Steers <[email protected]>
- Format generator expressions to match ruff's canonical style - Put expression target on separate line from 'for' clause Co-Authored-By: AJ Steers <[email protected]>
feat: Add InstallType.ANY and InstallType.INSTALLABLE enum values
Summary
This PR adds two new enum values to
InstallTypeto make the codebase more maintainable and future-proof:InstallType.ANY- Returns all connectors regardless of install typeInstallType.INSTALLABLE- Returns all installable connectors (equivalent to whatDOCKERcurrently does, but with clearer semantics)The PR also replaces uses of
InstallType.DOCKERwithInstallType.INSTALLABLEinget_connector_api_docs_urls()andget_connector_version_history()where we were using it to mean "all connectors" rather than specifically Docker connectors.Key changes:
InstallType.ANYandInstallType.INSTALLABLEto the enumget_available_connectors()to handle the new enum values by consolidating DOCKER, ANY, and INSTALLABLE into a single check (also fixes PLR0911 lint error about too many return statements)InstallType.DOCKERwithInstallType.INSTALLABLEin two functions where we wanted "all connectors" semanticsNo behavior changes - all three enum values (DOCKER, ANY, INSTALLABLE) return the same set of connectors (635 total). This is purely a refactoring to improve code clarity and maintainability.
Review & Testing Checklist for Human
ANY,INSTALLABLE, andDOCKERis clear and matches the intended use cases. Currently all three return identical results - is this the desired behavior?InstallType.DOCKERstill works correctly and that the replacement withINSTALLABLEin the two functions is appropriateget_available_connectors(InstallType.ANY)andget_available_connectors(InstallType.INSTALLABLE)to verify they return the expected connector listsTest Plan
Notes
poe check)Summary by CodeRabbit
New Features
Bug Fixes / Improvements