fix: resolve issue in connector dir discovery #603
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the connector directory discovery by ensuring that if the starting directory itself is the Airbyte repository root, it’s immediately returned instead of only checking parent and sibling directories.
- Added a direct
_is_airbyte_repo_rootcheck oncurrent_dirwithin the traversal loop. - Short-circuits and returns
current_dirwhen it matches the repo root.
📝 WalkthroughWalkthroughThe update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant connector_paths.py
Caller->>connector_paths.py: resolve_airbyte_repo_root(start_dir)
connector_paths.py->>connector_paths.py: _is_airbyte_repo_root(current_dir)
alt If current_dir is repo root
connector_paths.py-->>Caller: return current_dir
else
connector_paths.py->>connector_paths.py: _find_in_adjacent_dirs(current_dir)
connector_paths.py-->>Caller: return found_dir or None
end
✨ Finishing Touches
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://cdn.statically.io/img/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_cdk/utils/connector_paths.py (1)
70-74: Early-return is spot-on; tiny refactor could simplify the loop, wdyt?Great catch adding the
_is_airbyte_repo_root(current_dir)short-circuit – this resolves the “already at repo root” scenario.
For readability (and a microscopic performance gain), you could perform this check once before entering thewhileloop:current_dir = from_dir.resolve().absolute() -while current_dir != current_dir.parent: - if _is_airbyte_repo_root(current_dir): - return current_dir +if _is_airbyte_repo_root(current_dir): + return current_dir + +while current_dir != current_dir.parent:This avoids re-evaluating the same directory twice on the first iteration and makes the intent crystal-clear.
No functional change, purely stylistic – up to you if you think it reads better, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/utils/connector_paths.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
📝 WalkthroughWalkthroughThe update modifies the internal logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant resolve_airbyte_repo_root
participant _is_airbyte_repo_root
participant _find_in_adjacent_dirs
Caller->>resolve_airbyte_repo_root: Call function
loop For each directory up to filesystem root
resolve_airbyte_repo_root->>_is_airbyte_repo_root: Check if current_dir is repo root
alt If current_dir is root
_is_airbyte_repo_root-->>resolve_airbyte_repo_root: True
resolve_airbyte_repo_root-->>Caller: Return current_dir
else Not root
_is_airbyte_repo_root-->>resolve_airbyte_repo_root: False
resolve_airbyte_repo_root->>_find_in_adjacent_dirs: Search adjacent dirs for repo root
_find_in_adjacent_dirs-->>resolve_airbyte_repo_root: Return result or None
end
end
resolve_airbyte_repo_root-->>Caller: Return None if not found
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/utils/connector_paths.py (1)
34-41: Tiny readability tweak in_is_airbyte_repo_root?Would dropping the intermediate list and using straightforward boolean operators make this a bit leaner? Something like:
-return all( - [ - (path.name == "airbyte" or path.name == "airbyte-enterprise"), - (path / "airbyte-integrations").is_dir(), - ] -) +return ( + path.name in {"airbyte", "airbyte-enterprise"} + and (path / "airbyte-integrations").is_dir() +)Same semantics, one less allocation, and (arguably) easier to read — wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/utils/connector_paths.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (1)
airbyte_cdk/utils/connector_paths.py (1)
72-74: Nice short-circuit!Checking
current_dirfirst avoids an unneeded_find_in_adjacent_dirscall when we’re already at the repo root — good catch.
Summary by CodeRabbit