feat: add "runas_host_user" param to get_source() #614
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new boolean parameter Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Function
participant DockerExecutor
Caller->>Function: Call with runas_host_user flag set (True/False)
alt runas_host_user True
Function->>DockerExecutor: Execute Docker command with "--user UID:GID"
else runas_host_user False
Function->>DockerExecutor: Execute standard Docker command
end
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (2)
airbyte/_executors/util.py (2)
241-243: Consider adding platform compatibility check?The
os.getuid()andos.getgid()functions are not available on Windows. Should we add a platform check and provide a helpful error message for Windows users, wdyt?if runas_host_user is True: + if os.name == 'nt': # Windows + raise exc.PyAirbyteInputError( + message="The 'runas_host_user' parameter is not supported on Windows.", + guidance="This feature is only available on POSIX systems (Linux, macOS)." + ) docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"])
127-127: Consider documenting platform limitations?The
runas_host_userparameter is only supported on POSIX systems. Should we add this information to the docstring, wdyt?use_host_network: bool = False, - runas_host_user: bool = False, + runas_host_user: bool = False, # Only supported on POSIX systems (Linux, macOS) source_manifest: bool | dict | Path | str | None = None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte/_executors/util.py(3 hunks)airbyte/destinations/util.py(3 hunks)airbyte/sources/util.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
airbyte/destinations/util.py (1)
31-31: LGTM!The parameter is well-documented and correctly propagated to the executor.
Also applies to: 60-62, 77-77
airbyte/sources/util.py (1)
58-58: LGTM!The parameter is well-documented and correctly propagated to the executor.
Also applies to: 99-101, 123-123
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
airbyte/_executors/util.py (4)
2-9: Import block needs reordering per pipeline warning.The CI pipeline reported that the import block is un-sorted. Would you consider reordering the imports alphabetically and grouping them properly (standard library, third-party, local imports)? This will resolve the warning and improve code organization, wdyt?
from __future__ import annotations import logging import os import tempfile from pathlib import Path from typing import TYPE_CHECKING, Literal, cast import requests import yaml from rich import print # noqa: A004 # Allow shadowing the built-in🧰 Tools
🪛 GitHub Actions: Run Linters
[warning] 2-2: Ruff: Import block is un-sorted or un-formatted. Organize imports.
129-129: New parameter looks good, but should be documented.The
runas_host_userparameter matches the PR objectives and follows the existing parameter pattern. However, I noticed the docstring only references another function for parameter documentation. Would you consider updating the docstring to include this new parameter directly?
121-121: Function complexity exceeds threshold per pipeline error.The
get_connector_executorfunction now exceeds the complexity threshold (25 > 24) according to the pipeline error. This is likely due to the newrunas_host_userimplementation.Would you consider refactoring some of the logic into smaller helper functions to reduce the complexity? Perhaps extracting the Docker command construction into a separate function would help here.
🧰 Tools
🪛 Ruff (0.8.2)
121-121:
get_connector_executoris too complex (25 > 24)(C901)
🪛 GitHub Actions: Run Linters
[error] 121-121: Ruff:
get_connector_executoris too complex (25 > 24).
243-248: Consider handling Windows systems more explicitly.The current implementation tries to use
os.getuid()andos.getgid(), which aren't available on Windows. While the exception handling works, it might be clearer to detect the OS type explicitly.What do you think about this approach?
if runas_host_user is True: + import platform + if platform.system() != "Windows": try: docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"]) - except: + except AttributeError: logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.") + else: + logger.info("The 'runas_host_user' parameter is not supported on Windows.")🧰 Tools
🪛 Ruff (0.8.2)
246-246: Do not use bare
except(E722)
🪛 GitHub Actions: Run Linters
[error] 246-246: Ruff: Do not use bare
except.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/_executors/util.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
airbyte/_executors/util.py
246-246: Do not use bare except
(E722)
🪛 GitHub Actions: Run Linters
airbyte/_executors/util.py
[warning] 2-2: Ruff: Import block is un-sorted or un-formatted. Organize imports.
[error] 121-121: Ruff: get_connector_executor is too complex (25 > 24).
[error] 246-246: Ruff: Do not use bare except.
| if runas_host_user is True: | ||
| try: | ||
| docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"]) | ||
| except: | ||
| logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.") | ||
|
|
There was a problem hiding this comment.
Avoid bare except and consider reporting non-POSIX limitation more visibly.
The implementation of the runas_host_user parameter makes sense, but there are a couple of issues:
- The bare
exceptclause is flagged by linters and should be replaced with specific exception types. - The behavior silently falls back without the
--userflag on non-POSIX systems.
What do you think about this implementation?
if runas_host_user is True:
try:
docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"])
- except:
+ except AttributeError:
logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.")
+ logger.warning("Continuing without host user permissions. File operations may fail.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if runas_host_user is True: | |
| try: | |
| docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"]) | |
| except: | |
| logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.") | |
| if runas_host_user is True: | |
| try: | |
| docker_cmd.extend(["--user", f"{os.getuid()}:{os.getgid()}"]) | |
| except AttributeError: | |
| logger.info("The 'runas_host_user' parameter is only supported on POSIX systems.") | |
| logger.warning("Continuing without host user permissions. File operations may fail.") |
🧰 Tools
🪛 Ruff (0.8.2)
246-246: Do not use bare except
(E722)
🪛 GitHub Actions: Run Linters
[error] 246-246: Ruff: Do not use bare except.
|
resolved by #637 |
Docker in Docker context.
I need to execute a containerized source with same UID as the containerized host executing script in order to simplify file permissions management.
"config" and "catalog" files generated thanks to "tempfile" library have 600 POSIX perms and the "runas_host_user" approach helped me to allow source container to read this ones.
It works like the
use_host_networkparam, simply setrunas_host_usertoTruewhile callingget_source()method.Summary by CodeRabbit
New Features
Deprecations