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 <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/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. |
📝 WalkthroughWalkthroughInstallType gained Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Registry as airbyte/registry.py
Note right of Registry `#d1f0d1`: 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 (registry cache keys)
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 MANIFEST_ONLY/YAML connectors
end
else
Registry-->>Client: return connectors filtered by explicit InstallType membership
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Would you like me to scan the repo for remaining 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: 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 <aj@airbyte.io>
- Format generator expressions to match ruff's canonical style - Put expression target on separate line from 'for' clause Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/registry.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/registry.py (2)
airbyte/_util/meta.py (1)
is_docker_installed(193-194)airbyte/exceptions.py (1)
AirbyteConnectorNotRegisteredError(288-297)
🪛 GitHub Actions: Generate Docs
airbyte/registry.py
[error] 13-13: ImportError: cannot import name 'StrEnum' from 'typing'. This prevents docs generation (poetry run poe docs-generate) from loading the Airbyte module.
🪛 GitHub Actions: Run Linters
airbyte/registry.py
[error] 4-4: Import block is unsorted or un-formatted (ruff I001).
[error] 252-252: Line too long (102 > 100).
[error] 1-1: Ruff check failed. Command 'poetry run ruff check .' exited with code 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). (1)
- GitHub Check: Pytest (All, Python 3.11, Windows)
🔇 Additional comments (5)
airbyte/registry.py (5)
48-66: Nice addition of docstrings and the new enum values!The semantic distinction between
ANY(environment-independent, all connectors) andINSTALLABLE(environment-sensitive, depends on Docker availability) is well documented. This clarifies the intent nicely. Once the import issue is resolved, this looks good to go.
254-268: Environment-aware filtering logic looks solid.The branching between Docker-available (all connectors) and Docker-unavailable (Python/Manifest-only) correctly implements the
INSTALLABLEsemantics. The generator expression is a nice touch for memory efficiency.
289-290: Consolidation ofDOCKERandANYhandling looks correct.Both
DOCKERandANYreturn all connectors unconditionally, so grouping them makes sense. TheINSTALLABLEcase is now properly handled earlier in the function (lines 254-268) with the environment-aware logic.
465-471: UsingInstallType.ANYfor existence validation is the right call.For checking if a connector exists in the registry (regardless of installability),
ANYis the correct semantic choice. This aligns with the author's suggestions in the past review comments.
525-531: Consistent usage ofInstallType.ANYfor validation here too.This mirrors the pattern in
get_connector_api_docs_urls. Good consistency across both functions.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Devin, can you fix the ci issues. Thanks. |
Aaron ("AJ") Steers (aaronsteers)
changed the title
InstallType.ANY and InstallType.INSTALLABLE enum values
StrEnum is only available in Python 3.11+, but PyAirbyte supports Python 3.10. Changed InstallType from StrEnum to (str, Enum) which provides the same functionality and is compatible with Python 3.10. This fixes the ImportError: cannot import name 'StrEnum' from 'enum' that was causing CI failures on Python 3.10. Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airbyte/registry.py (1)
250-299: String"installable"currently hits the error path instead of using INSTALLABLE semantics
get_available_connectorsadvertisesinstall_type: InstallType | str | None, and you normalize string values toInstallTypelater. However,"installable"as a string won’t ever take the INSTALLABLE branch:
- For
install_type="installable", the firstif install_type is None or install_type == InstallType.INSTALLABLE:is false (Enum vs str).- After normalization to
InstallType.INSTALLABLE, there is no subsequent branch forINSTALLABLE, so the call falls through and raisesPyAirbyteInputError.If callers pass
"installable"(which seems natural given the type hint and other string values), they’ll unexpectedly get an “Invalid install type” error instead of the env‑aware behavior.Would it be clearer and safer to normalize
install_typeto anInstallTypeup front (treatingNoneasINSTALLABLE) and then branch on the enum only, e.g.:-def get_available_connectors( - install_type: InstallType | str | None = InstallType.INSTALLABLE, -) -> list[str]: +def get_available_connectors( + install_type: InstallType | str | None = InstallType.INSTALLABLE, +) -> list[str]: @@ - if install_type is None or install_type == InstallType.INSTALLABLE: - # Filter for installable connectors (default behavior). - if is_docker_installed(): - logger.info("Docker is detected. Returning all connectors.") - return sorted(_get_registry_cache().keys()) - - logger.info("Docker was not detected. Returning only Python and Manifest-only connectors.") - return sorted( - [ - connector_name - for connector_name, conn_info in _get_registry_cache().items() - if conn_info.language in {Language.PYTHON, Language.MANIFEST_ONLY} - ] - ) - - if not isinstance(install_type, InstallType): - install_type = InstallType(install_type) + if install_type is None: + install_type = InstallType.INSTALLABLE + elif not isinstance(install_type, InstallType): + install_type = InstallType(install_type) + + if install_type == InstallType.INSTALLABLE: + # Filter for installable connectors (default behavior). + if is_docker_installed(): + logger.info("Docker is detected. Returning all connectors.") + return sorted(_get_registry_cache().keys()) + + logger.info("Docker was not detected. Returning only Python and Manifest-only connectors.") + return sorted( + [ + connector_name + for connector_name, conn_info in _get_registry_cache().items() + if conn_info.language in {Language.PYTHON, Language.MANIFEST_ONLY} + ] + ) @@ - if install_type in {InstallType.DOCKER, InstallType.ANY}: + if install_type in {InstallType.DOCKER, InstallType.ANY}: return sorted(_get_registry_cache().keys())This would make
"installable",InstallType.INSTALLABLE, andNoneall behave consistently while preserving existing behavior for other values. Wdyt?
🧹 Nitpick comments (1)
airbyte/mcp/connector_registry.py (1)
87-89: Align comment with newInstallType.ANYsemanticsUsing
get_available_connectors(install_type=InstallType.ANY)here makes sense and keepslist_connectorsenv‑independent. The inline comment still talks about “all support Docker”, which is technically true but might confuse future readers now thatANYis the intentional base. Would you consider updating the comment to mentionInstallType.ANY(or “all connectors in the registry”) to reflect the new semantics, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/mcp/connector_registry.py(1 hunks)airbyte/registry.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
airbyte/mcp/connector_registry.py (1)
airbyte/registry.py (2)
get_available_connectors(250-313)InstallType(48-66)
airbyte/registry.py (2)
airbyte/_util/meta.py (1)
is_docker_installed(193-194)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, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (4)
airbyte/registry.py (4)
51-66: NewInstallTypemembers and docstrings look consistent with behaviorThe addition of
INSTALLABLEandANYplus the expanded docstrings for the existing members line up with howget_available_connectorsuses them (env‑sensitive vs env‑independent, and DOCKER as backward‑compatible “all connectors”). I don’t see issues with the enum surface here.
169-176:_get_registry_cachesignature/docstring change is reasonableMaking
force_refreshkeyword‑only and documenting that the cache maps connector name →ConnectorMetadataclarifies usage without changing behavior. This looks good to me.
473-479: UsingInstallType.ANYfor docs membership checks matches “all connectors” semanticsSwitching the guard to
connector_name not in get_available_connectors(InstallType.ANY)and usingANYagain for theavailable_connectorscontext makes this check explicitly env‑independent, which seems right for API docs lookups. This looks solid to me.
533-539: Version history guard onInstallType.ANYalso looks correctThe version‑history path now uses
get_available_connectors(InstallType.ANY)both for the membership check and for the error context, which aligns with the “all connectors in the registry” semantics you describe forANY. I don’t see issues here.
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1762981002-add-installtype-any-installable
branch
1 participant
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
✏️ 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.