Conversation
WalkthroughThis pull request introduces a new DBus networking capability to the jumpstarter-driver-network package. A new class Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DN as DbusNetwork
participant Env as Environment
App->>DN: Instantiate DbusNetwork(kind)
DN->>Env: Retrieve bus address (based on kind)
Env-->>DN: Provide DBus address
DN->>DN: Parse address (extract scheme & args)
alt scheme == "unix"
DN->>DN: Invoke connect_unix()
else scheme == "tcp"
DN->>DN: Invoke connect_tcp()
end
DN-->>App: Returns client info & extra labels
sequenceDiagram
participant Client as DbusNetworkClient
participant DA as DbusAdapter
participant Env as Environment
Client->>DA: __aenter__()
DA->>DA: Call parent __aenter__() to obtain TCP address
DA->>Env: Read & store current DBUS_* variable
DA->>Env: Set new DBUS_* variable with TCP address
DA-->>Client: Yield connection
Client->>DA: __aexit__()
DA->>DA: Call parent __aexit__()
DA->>Env: Restore or remove DBUS_* variable
DA-->>Client: Exit context
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
This closes jumpstarter-dev/jumpstarter#152 and jumpstarter-dev/jumpstarter#153 as native client cli/libraries for bluez/iwd/wpa_supplicant can now be used as usual remotely via the forwarded dbus socket. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
66-69: Add docstring forDbusNetworkclass.
It would be beneficial to add a docstring to briefly explain how the bus type is determined (system vs session) and how environment variables are leveraged. This helps future maintainers understand the intended usage.packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
113-116: Ensure tests run in a controlled environment.
Skipping the test if the DBus system bus isn't present is appropriate. Consider adding a fallback or mocking approach if you want coverage in CI environments that lack system D-Bus.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
4-5: Use caution when mixinggetenvand direct environment variable references.
While it's perfectly fine to import and usegetenvandgetuidhere, consider providing more context in docstrings or in your class docstring to clarify how you expect environment variables to be set for bus addresses.
[approve]packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (2)
11-11: Imports look good.
All essential classes are imported. No issues.
118-128: Confirm output validation.
Currently, the test usessubprocess.runwithcheck=Truebut doesn't assert anything about the output. Confirm if you need to verify the output content to ensure correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
113-144: Consider enhancing test coverage.While the basic connectivity tests are good, consider adding:
- Docstrings explaining the purpose of each test
- Verification of the
busctloutput to ensure expected services are available- Error scenario tests (e.g., invalid bus kind, connection failures)
- Tests for actual D-Bus service functionality (e.g., calling a method on a known service)
Example enhancement for system bus test:
@pytest.mark.skipif( os.getenv("DBUS_SYSTEM_BUS_ADDRESS") is None and not os.path.exists("/run/dbus/system_bus_socket"), reason="dbus system bus not available", ) @pytest.mark.skipif(which("busctl") is None, reason="busctl not available") def test_dbus_network_system(monkeypatch): + """ + Test D-Bus system bus connectivity and basic functionality. + + Verifies: + 1. Connection to system bus can be established + 2. Basic D-Bus operations work through the forwarded connection + """ with serve(DbusNetwork(kind="system")) as client: with PortforwardAdapter(client=client) as addr: - subprocess.run( + result = subprocess.run( ["busctl", "list", "--system", "--no-pager"], env=os.environ | {"DBUS_SYSTEM_BUS_ADDRESS": f"tcp:host={addr[0]},port={addr[1]}"}, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) + # Verify expected system services are present + output = result.stdout.decode() + assert "org.freedesktop.DBus" in output, "D-Bus service not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (3)
11-11: LGTM!The import of
DbusNetworkis correctly placed alongside other network-related imports.
113-128: LGTM! Well-structured test for D-Bus system bus.The test correctly verifies basic connectivity to the D-Bus system bus with appropriate skip conditions and environment configuration.
130-144: LGTM! Well-structured test for D-Bus session bus.The test correctly verifies basic connectivity to the D-Bus session bus with appropriate skip conditions and environment configuration.
It would not close those issues, but make it much easier to develop. To close those issues we may have to offer extensive documentation and wrappers on top to let do the specific actions on the client side. |
|
Please add documentation with examples of usage, exporter configuration, etc... |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
22-33: Consider adding validation for local_host and local_port.While the implementation is correct, it would be beneficial to add input validation for
local_hostandlocal_portto prevent potential runtime errors.@dataclass(kw_only=True) class TcpPortforwardAdapter(PortforwardAdapter): local_host: str = "127.0.0.1" local_port: int = 0 + + def __post_init__(self): + if not isinstance(self.local_port, int) or self.local_port < 0: + raise ValueError(f"Invalid port number: {self.local_port}") + if not self.local_host: + raise ValueError("local_host cannot be empty")packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
124-139: Consider adding more test cases for D-Bus system bus.While the basic functionality is tested, consider adding test cases for:
- Error cases (invalid bus address)
- Connection failures
- Different message types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/__init__.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(3 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (9)
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/__init__.py (1)
4-6: LGTM! Clean refactoring of port forwarding adapters.The changes properly expose the new specialized TCP and Unix socket forwarding adapters while maintaining backward compatibility through the
__all__declaration.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
6-6: LGTM! Appropriate use of TCP-specific adapter.The change to inherit from
TcpPortforwardAdapteraligns with the class's use of TCP sockets (AF_INET) for communication.Also applies to: 10-10
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py (1)
7-7: LGTM! Appropriate use of TCP-specific adapter.The change to inherit from
TcpPortforwardAdapteraligns with the class's use of TCP-specific connection parameters (host and port).Also applies to: 11-11
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py (1)
5-5: LGTM! Appropriate use of TCP-specific adapter.The change to inherit from
TcpPortforwardAdapteraligns with:
- The class's use of TCP-specific connection parameters (host and port)
- WebSocket protocol operating over TCP
Also applies to: 10-10
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (2)
8-20: LGTM! Well-structured base adapter implementation.The
PortforwardAdapterprovides a clean base implementation with proper connection lifecycle management and stream forwarding.
35-40: LGTM! Clean Unix adapter implementation.The
UnixPortforwardAdapterprovides a straightforward implementation for Unix socket forwarding.packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
67-70: LGTM! Well-defined class with proper type constraints.The use of
Literaltype forkindensures type safety at compile time.
71-118: Consider breaking down the complex connection logic.The connection method is quite complex with multiple responsibilities. Consider extracting the bus address parsing and connection logic into separate methods.
+ def _parse_bus_address(self) -> tuple[str, dict[str, str]]: + match self.kind: + case "system": + bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") + case "session": + bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") + case _: + raise ValueError(f"invalid bus type: {self.kind}") + + scheme, sep, rem = bus.partition(":") + if not sep: + raise ValueError(f"invalid bus addr: {bus}") + + args = {} + for part in rem.split(","): + key, sep, value = part.partition("=") + if not sep: + raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") + args[key] = value + + return scheme, args + async def _connect_unix(self, path: str) -> MetadataStream: + self.logger.debug("Connecting UDS path=%s", path) + stream = await connect_unix(path=path) + return MetadataStream(stream=stream, metadata={"kind": self.kind}) + async def _connect_tcp(self, host: str, port: int) -> MetadataStream: + self.logger.debug("Connecting TCP host=%s port=%d", host, port) + stream = await connect_tcp(remote_host=host, remote_port=port) + return MetadataStream(stream=stream, metadata={"kind": self.kind}) @exportstream @asynccontextmanager async def connect(self): # noqa: C901 - match self.kind: - case "system": - bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") - case "session": - bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") - case _: - raise ValueError(f"invalid bus type: {self.kind}") - - scheme, sep, rem = bus.partition(":") - if not sep: - raise ValueError(f"invalid bus addr: {bus}") - - args = {} - for part in rem.split(","): - key, sep, value = part.partition("=") - if not sep: - raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") - args[key] = value + scheme, args = self._parse_bus_address() match scheme: case "unix": if "path" not in args: raise ValueError(f"invalid bus addr: {bus}, missing path argument") - - self.logger.debug("Connecting UDS path=%s", args["path"]) - async with await connect_unix(path=args["path"]) as stream: - async with MetadataStream(stream=stream, metadata={"kind": self.kind}) as mstream: + async with await self._connect_unix(args["path"]) as mstream: yield mstream case "tcp": if "host" not in args: raise ValueError(f"invalid bus addr: {bus}, missing host argument") if "port" not in args: raise ValueError(f"invalid bus addr: {bus}, missing port argument") try: port = int(args["port"]) except ValueError as e: raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e - self.logger.debug("Connecting TCP host=%s port=%d", args["host"], port) - async with await connect_tcp(remote_host=args["host"], remote_port=port) as stream: - async with MetadataStream(stream=stream, metadata={"kind": self.kind}) as mstream: + async with await self._connect_tcp(args["host"], port) as mstream: yield mstream case _: raise ValueError(f"invalid bus scheme: {scheme}")packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
141-155: Consider adding more test cases for D-Bus session bus.Similar to the system bus tests, consider adding more test cases for comprehensive coverage.
Additionally, verify that the test environment is properly cleaned up:
|
Blocked by our inability to pass labels from drivers to clients: we need a mechanism to pass the type of the dbus bus (session or system) to the client side, so it knows which environment variable to set. |
What do you mean? |
So the driver can proxy one of two different kinds of dbus: session bus or system bus, and they are represented by two different environment variables |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
docs/source/api-reference/drivers/dbus.yaml (1)
1-4: 🛠️ Refactor suggestionAdd configuration options for xdg-dbus-proxy features.
The current configuration is minimal and doesn't expose the filtering and security features available in xdg-dbus-proxy. Consider adding configuration options for:
- Allowed service names
- Broadcast filtering
- Ownership filtering
- Talk filtering
This would align better with the PR objective of incorporating xdg-dbus-proxy functionality.
Also, consider adding validation for the
kindparameter:type: "jumpstarter_driver_network.driver.DbusNetwork" config: - kind: "system" # which bus to connect to, system or session + kind: + type: string + enum: ["system", "session"] + description: "which bus to connect to, system or session"
♻️ Duplicate comments (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
82-121: 🛠️ Refactor suggestionRefactor complex initialization logic.
The
__post_init__method is quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused methods.def __post_init__(self): if hasattr(super(), "__post_init__"): super().__post_init__() + bus = self._get_bus_address() + self._parse_bus_address(bus) + self._validate_scheme_args(bus) +def _get_bus_address(self) -> str: match self.kind: case "system": - bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") + return getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") case "session": - bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") + return getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") case _: raise ValueError(f"invalid bus type: {self.kind}") +def _parse_bus_address(self, bus: str) -> None: self.scheme, sep, rem = bus.partition(":") if not sep: raise ValueError(f"invalid bus addr: {bus}") for part in rem.split(","): key, sep, value = part.partition("=") if not sep: raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") self.args[key] = value +def _validate_scheme_args(self, bus: str) -> None: match self.scheme: case "unix": if "path" not in self.args: raise ValueError(f"invalid bus addr: {bus}, missing path argument") case "tcp": if "host" not in self.args: raise ValueError(f"invalid bus addr: {bus}, missing host argument") if "port" not in self.args: raise ValueError(f"invalid bus addr: {bus}, missing port argument") try: port = int(self.args["port"]) except ValueError as e: raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e self.args["port"] = port case _: raise ValueError(f"invalid bus scheme: {self.scheme}")
🧹 Nitpick comments (6)
packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py (1)
18-22: Add docstring to explain method existence check.Consider adding a docstring to explain the purpose and behavior of this method.
def _check_method_exists(self, method): + """ + Verify that the specified method exists in the available methods list. + + Args: + method: The name of the method to check + + Raises: + AttributeError: If the method is not found in the available methods + """ if self._methods is None: self._methods = self.call("get_methods") if method not in self._methods: raise AttributeError(f"method {method} not found in {self._methods}")packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py (2)
12-15: Remove unnecessary pass statements.The pass statements in the match cases are redundant and can be removed.
match self.client.kind: case "system": self.varname = "DBUS_SYSTEM_BUS_ADDRESS" - pass case "session": self.varname = "DBUS_SESSION_BUS_ADDRESS" - passAlso applies to: 15-17
20-21: Consider using a more robust environment variable handling approach.The current implementation might be vulnerable to race conditions in a multi-threaded environment. Consider using a thread-local storage or a context-local variable for storing the environment state.
docs/source/api-reference/drivers/dbus.md (2)
25-38: Consider using native DBus libraries instead of subprocess.The example uses
subprocess.runto callbusctl. Consider using native DBus libraries likedbus-pythonorpydbusfor better type safety and error handling:import dbus bus = dbus.SystemBus() systemd = bus.get_object('org.freedesktop.systemd1', '/org/freedesktop/systemd1') machine_id = systemd.GetMachineId(dbus_interface='org.freedesktop.DBus.Peer') print(machine_id)
40-48: Document the bus type limitation and align test setup with configuration example.Two issues to address:
- The test setup uses "session" bus while the configuration example uses "system" bus. This inconsistency might confuse users.
- Based on the PR objectives, there's a technical limitation about passing bus type labels from drivers to clients. This should be documented to help users understand potential issues.
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
37-191: Consider adding docstrings for privacy-related fields.While the implementation is correct, consider adding docstrings to explain the privacy protocol options and their implications.
@dataclass(kw_only=True) class SNMPServer(Driver): """SNMP Power Control Driver""" host: str = field() user: str = field() port: int = field(default=161) plug: int = field() oid: str = field(default="1.3.6.1.4.1.13742.6.4.1.2.1.2.1") auth_protocol: AuthProtocol = field(default=AuthProtocol.NONE) auth_key: str | None = field(default=None) - priv_protocol: PrivProtocol = field(default=PrivProtocol.NONE) - priv_key: str | None = field(default=None) + priv_protocol: PrivProtocol = field(default=PrivProtocol.NONE, + metadata={"description": "Privacy protocol for SNMP v3 (NONE, DES, or AES)"}) + priv_key: str | None = field(default=None, + metadata={"description": "Privacy key for SNMP v3 when using DES or AES"}) timeout: float = field(default=5.0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docs/source/api-reference/drivers/dbus.md(1 hunks)docs/source/api-reference/drivers/dbus.yaml(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(0 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/__init__.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/client.py(1 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py(2 hunks)packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py(7 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py(2 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)
💤 Files with no reviewable changes (1)
- packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
✅ Files skipped from review due to trivial changes (4)
- docs/source/api-reference/drivers/index.md
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-driver-shell/jumpstarter_driver_shell/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/init.py
👮 Files not reviewed due to content moderation or server errors (4)
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py
- packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py
- packages/jumpstarter/jumpstarter/driver/base.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (8)
packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py (1)
12-22: Well-structured implementation of DBus network client!The implementation follows Python's context management best practices and properly delegates connection handling to the adapter.
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
124-134: Well-structured connect method implementation!The connect method is clean and follows a good pattern for handling different connection types.
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (2)
20-77: LGTM! Well-structured enums and error handling.The implementation of privacy protocols is clean and follows best practices:
- Clear enum definitions for protocols
- Proper error class hierarchy
- Well-organized protocol mappings
20-34: LGTM! Well-structured enums and error class.The new enums and error class are well-defined:
- Clear privacy protocol options
- Proper inheritance from str/IntEnum
- Custom error class with descriptive docstring
packages/jumpstarter/jumpstarter/driver/base.py (4)
84-85: LGTM! Clean implementation of extra labels.The
extra_labelsmethod provides a clean way to extend label functionality in derived classes.
173-180: LGTM! Well-integrated label merging.The report method cleanly integrates the extra labels with existing labels using dictionary merging.
84-86: LGTM! Well-designed extension point.The
extra_labelsmethod provides a clean extension point for subclasses to add custom labels.
167-180: LGTM! Clean integration of extra labels.The report method cleanly integrates the extra labels while maintaining the existing functionality.
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
|
Sorry, I did an unsuccessful rebase on the web ui |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
107-175: Add docstring with configuration examples.Consider adding a docstring similar to other network drivers in this file, showing YAML configuration examples for both system and session bus types with unix and tcp schemes.
@dataclass(kw_only=True) class DbusNetwork(NetworkInterface, Driver): + ''' + DbusNetwork is a driver for connecting to DBus sockets + + >>> config = """ + ... type: jumpstarter_driver_network.driver.DbusNetwork + ... config: + ... kind: system # or session + ... """ + >>> with run(config) as dbus: + ... pass + '''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
docs/source/api-reference/drivers/index.md(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/source/api-reference/drivers/index.md
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (4)
107-114: LGTM! Well-structured class definition.The class follows consistent patterns with other network drivers and has proper type constraints for DBus bus types.
116-122: Good implementation of label passing mechanism.The
extra_labelsmethod provides a way to convey the DBus bus type to clients, addressing the concern mentioned in the PR about passing labels from drivers to clients.
123-162: Consider breaking down the initialization logic.The
__post_init__method is complex (as indicated by noqa: C901) and handles multiple responsibilities. Consider extracting the bus address parsing and scheme-specific validation into separate methods.def __post_init__(self): if hasattr(super(), "__post_init__"): super().__post_init__() - match self.kind: - case "system": - bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") - case "session": - bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") - case _: - raise ValueError(f"invalid bus type: {self.kind}") + bus = self._get_bus_address() + self._parse_bus_address(bus) + self._validate_scheme_args(bus) +def _get_bus_address(self) -> str: + match self.kind: + case "system": + return getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") + case "session": + return getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") + case _: + raise ValueError(f"invalid bus type: {self.kind}") +def _parse_bus_address(self, bus: str) -> None: + self.scheme, sep, rem = bus.partition(":") + if not sep: + raise ValueError(f"invalid bus addr: {bus}") + + for part in rem.split(","): + key, sep, value = part.partition("=") + if not sep: + raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") + self.args[key] = value +def _validate_scheme_args(self, bus: str) -> None: + match self.scheme: + case "unix": + if "path" not in self.args: + raise ValueError(f"invalid bus addr: {bus}, missing path argument") + case "tcp": + if "host" not in self.args or "port" not in self.args: + raise ValueError(f"invalid bus addr: {bus}, missing required arguments") + try: + self.args["port"] = int(self.args["port"]) + except ValueError as e: + raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e + case _: + raise ValueError(f"invalid bus scheme: {self.scheme}")
163-175: LGTM! Clean connection handling implementation.The connection logic is well-implemented, following consistent patterns with other drivers and providing appropriate logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (3)
106-124: Enhance test coverage for system bus.While the test verifies basic connectivity, consider adding more assertions to validate:
- The actual DBus functionality (e.g., calling a well-known service)
- Error cases (e.g., invalid addresses)
- The content of the busctl output
def test_dbus_network_system(monkeypatch): with serve(DbusNetwork(kind="system")) as client: assert client.kind == "system" oldvar = os.getenv("DBUS_SYSTEM_BUS_ADDRESS") with client: assert oldvar != os.getenv("DBUS_SYSTEM_BUS_ADDRESS") - subprocess.run( + result = subprocess.run( ["busctl", "list", "--system", "--no-pager"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) + # Verify expected services are present + output = result.stdout.decode() + assert "org.freedesktop.DBus" in output, "DBus service not found" + + # Test error case + with pytest.raises(ValueError): + DbusNetwork(kind="invalid")
126-143: Enhance test coverage for session bus.Similar to the system bus test, this test could benefit from additional assertions and error cases.
def test_dbus_network_session(monkeypatch): with serve(DbusNetwork(kind="session")) as client: assert client.kind == "session" oldvar = os.getenv("DBUS_SESSION_BUS_ADDRESS") with client: assert oldvar != os.getenv("DBUS_SESSION_BUS_ADDRESS") - subprocess.run( + result = subprocess.run( ["busctl", "list", "--user", "--no-pager"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) + # Verify expected services are present + output = result.stdout.decode() + assert "org.freedesktop.DBus" in output, "DBus service not found" + + # Verify environment variable format + new_addr = os.getenv("DBUS_SESSION_BUS_ADDRESS") + assert new_addr.startswith(("unix:", "tcp:")), f"Invalid address format: {new_addr}"
106-143: Consider adding more comprehensive test cases.While the current tests verify basic functionality, consider adding tests for:
- Error cases (invalid bus types, addresses)
- Connection failures
- Data transmission over the DBus connection
Verify busctl command output.
The tests currently don't verify the output of the
busctl listcommands. Consider checking for expected services in the output.# Add assertions for expected output: result = subprocess.run( ["busctl", "list", "--system", "--no-pager"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, ) assert b"org.freedesktop.DBus" in result.stdout, "DBus service not found in system bus"packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (3)
107-114: Add class documentation.Consider adding a docstring to describe the class purpose, parameters, and usage examples, similar to other network classes in this file.
@dataclass(kw_only=True) class DbusNetwork(NetworkInterface, Driver): + ''' + DbusNetwork is a driver for connecting to DBus sockets + + >>> config = """ + ... type: jumpstarter_driver_network.driver.DbusNetwork + ... config: + ... kind: system # or session + ... """ + >>> with run(config) as dbus: + ... pass + ''' kind: Literal["system", "session"]
123-162: Refactor complex initialization logic.The
__post_init__method is quite complex with multiple responsibilities. Consider breaking it down into smaller, focused methods:
- Bus address resolution
- Argument parsing
- Scheme-specific validation
def __post_init__(self): if hasattr(super(), "__post_init__"): super().__post_init__() + + bus = self._resolve_bus_address() + self._parse_bus_address(bus) + self._validate_scheme_args() +def _resolve_bus_address(self) -> str: match self.kind: case "system": - bus = getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") + return getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") case "session": - bus = getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") + return getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") case _: raise ValueError(f"invalid bus type: {self.kind}") +def _parse_bus_address(self, bus: str) -> None: self.scheme, sep, rem = bus.partition(":") if not sep: raise ValueError(f"invalid bus addr: {bus}") for part in rem.split(","): key, sep, value = part.partition("=") if not sep: raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") self.args[key] = value +def _validate_scheme_args(self) -> None: match self.scheme: case "unix": if "path" not in self.args: raise ValueError(f"invalid bus addr: {bus}, missing path argument") case "tcp": if "host" not in self.args: raise ValueError(f"invalid bus addr: {bus}, missing host argument") if "port" not in self.args: raise ValueError(f"invalid bus addr: {bus}, missing port argument") try: port = int(self.args["port"]) except ValueError as e: raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e self.args["port"] = port case _: raise ValueError(f"invalid bus scheme: {self.scheme}")
123-162: Consider breaking down the initialization logic.The
__post_init__method is quite complex (C901 noqa comment indicates this). Consider extracting the bus address parsing logic into separate methods.- def __post_init__(self): # noqa: C901 + def __post_init__(self): + if hasattr(super(), "__post_init__"): + super().__post_init__() + self._parse_bus_address(self._get_bus_address()) + + def _get_bus_address(self) -> str: + match self.kind: + case "system": + return getenv("DBUS_SYSTEM_BUS_ADDRESS", "unix:path=/run/dbus/system_bus_socket") + case "session": + return getenv("DBUS_SESSION_BUS_ADDRESS", f"unix:path=/run/user/{getuid()}/bus") + case _: + raise ValueError(f"invalid bus type: {self.kind}") + + def _parse_bus_address(self, bus: str) -> None: + self.scheme, sep, rem = bus.partition(":") + if not sep: + raise ValueError(f"invalid bus addr: {bus}") + + self._parse_arguments(bus, rem) + self._validate_scheme_arguments(bus) + + def _parse_arguments(self, bus: str, rem: str) -> None: + for part in rem.split(","): + key, sep, value = part.partition("=") + if not sep: + raise ValueError(f"invalid bus addr: {bus}, missing separator in arguments") + self.args[key] = value + + def _validate_scheme_arguments(self, bus: str) -> None: + match self.scheme: + case "unix": + if "path" not in self.args: + raise ValueError(f"invalid bus addr: {bus}, missing path argument") + case "tcp": + if "host" not in self.args: + raise ValueError(f"invalid bus addr: {bus}, missing host argument") + if "port" not in self.args: + raise ValueError(f"invalid bus addr: {bus}, missing port argument") + try: + self.args["port"] = int(self.args["port"]) + except ValueError as e: + raise ValueError(f"invalid bus addr: {bus}, invalid port argument") from e + case _: + raise ValueError(f"invalid bus scheme: {self.scheme}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
docs/source/api-reference/drivers/dbus.md(1 hunks)docs/source/api-reference/drivers/dbus.yaml(1 hunks)docs/source/api-reference/drivers/index.md(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/__init__.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py(2 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py(3 hunks)packages/jumpstarter/jumpstarter/driver/base.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/init.py
- docs/source/api-reference/drivers/index.md
- packages/jumpstarter/jumpstarter/driver/base.py
- packages/jumpstarter-driver-network/jumpstarter_driver_network/client.py
- docs/source/api-reference/drivers/dbus.yaml
- docs/source/api-reference/drivers/dbus.md
- packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (3)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver_test.py (1)
1-11: LGTM! Clean import organization.The imports are well-organized, with standard library imports first, followed by third-party and local imports.
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (2)
107-114: LGTM! Well-structured class definition.The class is well-defined with appropriate type hints and default values. The use of
ClassVarfor the label constant is a good practice.
116-121: LGTM! Clean implementation of client and label methods.The methods are concise and follow single responsibility principle.
| @exportstream | ||
| @asynccontextmanager | ||
| async def connect(self): | ||
| match self.scheme: | ||
| case "unix": | ||
| self.logger.debug("Connecting UDS path=%s", self.args["path"]) | ||
| async with await connect_unix(path=self.args["path"]) as stream: | ||
| yield stream | ||
| case "tcp": | ||
| self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"]) | ||
| async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream: | ||
| yield stream | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for connection failures.
The connection logic should handle potential network errors and provide meaningful error messages.
@exportstream
@asynccontextmanager
async def connect(self):
+ try:
match self.scheme:
case "unix":
self.logger.debug("Connecting UDS path=%s", self.args["path"])
async with await connect_unix(path=self.args["path"]) as stream:
yield stream
case "tcp":
self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"])
async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream:
yield stream
+ except ConnectionError as e:
+ self.logger.error("Failed to connect to %s DBus: %s", self.kind, e)
+ raise ValueError(f"Failed to connect to {self.kind} DBus: {e}") from e📝 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.
| @exportstream | |
| @asynccontextmanager | |
| async def connect(self): | |
| match self.scheme: | |
| case "unix": | |
| self.logger.debug("Connecting UDS path=%s", self.args["path"]) | |
| async with await connect_unix(path=self.args["path"]) as stream: | |
| yield stream | |
| case "tcp": | |
| self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"]) | |
| async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream: | |
| yield stream | |
| @exportstream | |
| @asynccontextmanager | |
| async def connect(self): | |
| try: | |
| match self.scheme: | |
| case "unix": | |
| self.logger.debug("Connecting UDS path=%s", self.args["path"]) | |
| async with await connect_unix(path=self.args["path"]) as stream: | |
| yield stream | |
| case "tcp": | |
| self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"]) | |
| async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream: | |
| yield stream | |
| except ConnectionError as e: | |
| self.logger.error("Failed to connect to %s DBus: %s", self.kind, e) | |
| raise ValueError(f"Failed to connect to {self.kind} DBus: {e}") from e |
🛠️ Refactor suggestion
Consider adding error handling for connection failures.
The connect method should handle potential connection failures and provide meaningful error messages.
@exportstream
@asynccontextmanager
async def connect(self):
+ try:
match self.scheme:
case "unix":
self.logger.debug("Connecting UDS path=%s", self.args["path"])
async with await connect_unix(path=self.args["path"]) as stream:
yield stream
case "tcp":
self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"])
async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream:
yield stream
+ except OSError as e:
+ raise ConnectionError(f"Failed to connect to {self.kind} bus: {e}") from e📝 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.
| @exportstream | |
| @asynccontextmanager | |
| async def connect(self): | |
| match self.scheme: | |
| case "unix": | |
| self.logger.debug("Connecting UDS path=%s", self.args["path"]) | |
| async with await connect_unix(path=self.args["path"]) as stream: | |
| yield stream | |
| case "tcp": | |
| self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"]) | |
| async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream: | |
| yield stream | |
| @exportstream | |
| @asynccontextmanager | |
| async def connect(self): | |
| try: | |
| match self.scheme: | |
| case "unix": | |
| self.logger.debug("Connecting UDS path=%s", self.args["path"]) | |
| async with await connect_unix(path=self.args["path"]) as stream: | |
| yield stream | |
| case "tcp": | |
| self.logger.debug("Connecting TCP host=%s port=%d", self.args["host"], self.args["port"]) | |
| async with await connect_tcp(remote_host=self.args["host"], remote_port=self.args["port"]) as stream: | |
| yield stream | |
| except OSError as e: | |
| raise ConnectionError(f"Failed to connect to {self.kind} bus: {e}") from e |
2 participants
Right now the driver proxies the dbus socket directly without filtering. We should incorporate https://github.com/flatpak/xdg-dbus-proxy for that functionality.
Summary by CodeRabbit
New Features
DbusNetworkClientfor enhanced context management and connection labeling.DbusAdapterfor structured management of D-Bus connections over TCP.Documentation
Tests
Refactor