snmp: initial SNMP driver for power control #251
Conversation
|
Warning Rate limit exceeded@bennyz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughThis update introduces a new SNMP driver that enables asynchronous power control operations. A new client class ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SNMPServerClient
participant SNMPServer
participant SNMPEngine
User->>CLI: Execute "snmp on"/"snmp off" command
CLI->>SNMPServerClient: Invoke on()/off() method
SNMPServerClient->>SNMPServer: Forward command (@export on/off)
SNMPServer->>SNMPEngine: Configure engine & send SNMP set command
SNMPEngine-->>SNMPServer: Return response (success/failure)
SNMPServer-->>SNMPServerClient: Pass back status
SNMPServerClient-->>CLI: Output confirmation
sequenceDiagram
participant User
participant CLI
participant PowerClient
User->>CLI: Execute cycle command with wait time
CLI->>PowerClient: Call cycle(wait)
PowerClient->>PowerClient: Invoke off() method
PowerClient->>PowerClient: Wait (sleep for 'wait' seconds)
PowerClient->>PowerClient: Invoke on() method
PowerClient-->>CLI: Return cycle completion message
Possibly related PRs
Suggested reviewers
Poem
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (2)
46-55: Consider a more consistent approach to asynchronous execution.The
_setup_snmpmethod checks for a running event loop and creates one if missing, but the overall design still seems synchronous in certain places (e.g., inpower_cycle). Consider standardizing on an async architecture to avoid mixed paradigms, especially if you plan to integrate with other async code.🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Failed to spawn:
pytest. Caused by: No such file or directory (os error 2)
163-167: Optional cleanup or resource management step might be beneficial.
close()is currently a no-op but does callsuper().close()if defined. In the event that future changes introduce persistent transports or caches, consider finalizing or cleaning them in this method.🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Failed to spawn:
pytest. Caused by: No such file or directory (os error 2)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
24-48: Enhance error reporting in CLI commands.Currently, the CLI subcommands (
on,off,cycle) print only the returned result. If unexpected exceptions occur in the SNMP driver, consider capturing and displaying them with more context or exit codes to improve troubleshooting.🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Failed to spawn:
pytest. Caused by: No such file or directory (os error 2)
🛑 Comments failed to post (1)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
150-156: 🛠️ Refactor suggestion
Avoid blocking calls inside async workflows.
time.sleep(self.quiescent_period)blocks the entire thread. If asynchronous usage is intended, consider replacing it withawait asyncio.sleep(self.quiescent_period)and turningpower_cycleinto an async function for true non-blocking behavior.- time.sleep(self.quiescent_period) + import asyncio + await asyncio.sleep(self.quiescent_period)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] Failed to spawn:
pytest. Caused by: No such file or directory (os error 2)
| snmp_engine, | ||
| "my-target", | ||
| udp.DOMAIN_NAME, | ||
| (self.ip_address, self.port), |
There was a problem hiding this comment.
Can't pysnmp accept a domain name as the target address? The config option itself is named DOMAIN_NAME.
There was a problem hiding this comment.
I got OSError: illegal IP address string passed to inet_pton, but didn't look too deep to see if there's a way without manual resolve
|
|
||
|
|
||
| @dataclass(kw_only=True) | ||
| class SNMPServerClient(DriverClient): |
There was a problem hiding this comment.
We can inherit from PowerClient
|
todo:
|
| class SNMPServerClient(DriverClient): | ||
| """Client interface for SNMP Power Control""" | ||
|
|
||
| def power_on(self): |
There was a problem hiding this comment.
yes, let's use standard on(), off() , ... and a fake power read.
power_cycle should be implemented on the client side, we can add it to the base PowerClient class (may be with some parameters like the power off time, and a default of 1 or 2 s?)
I would just call it cycle() (so it's aligned with on/off)
| @snmp.command() | ||
| def cycle(): | ||
| """Power cycle the device""" | ||
| result = self.power_cycle() | ||
| click.echo(result) |
There was a problem hiding this comment.
And then we can also add this bit to the base Power Interface.
In fact, if you do this, there is no need to create a Client, we can just reference the base power client from the driver itself.
|
|
||
| try: | ||
| self.ip_address = socket.gethostbyname(self.host) | ||
| self.logger.info(f"Resolved {self.host} to {self.ip_address}") |
There was a problem hiding this comment.
| self.logger.info(f"Resolved {self.host} to {self.ip_address}") | |
| self.logger.debug(f"Resolved {self.host} to {self.ip_address}") |
| config.USM_AUTH_NONE, | ||
| None |
There was a problem hiding this comment.
I would add some ability to authenticate too.
|
|
||
| def callback(snmpEngine, sendRequestHandle, errorIndication, | ||
| errorStatus, errorIndex, varBinds, cbCtx): | ||
| self.logger.info(f"Callback {errorIndication} {errorStatus} {errorIndex} {varBinds}") |
There was a problem hiding this comment.
| self.logger.info(f"Callback {errorIndication} {errorStatus} {errorIndex} {varBinds}") | |
| self.logger.debug(f"Callback {errorIndication} {errorStatus} {errorIndex} {varBinds}") |
| self.logger.info(f"SNMP error: {errorIndication}") | ||
| result["error"] = f"SNMP error: {errorIndication}" | ||
| elif errorStatus: | ||
| self.logger.info(f"SNMP status: {errorStatus}") |
There was a problem hiding this comment.
| self.logger.info(f"SNMP status: {errorStatus}") | |
| self.logger.error(f"SNMP status: {errorStatus}") |
? what is the difference between errorStatus and errorIndication?
There was a problem hiding this comment.
not entirely sure, but looking at https://github.com/lextudio/pysnmp/blob/ba05fad934b3b904e02d3ceb7f1ca9c1b332560f/pysnmp/proto/api/v1.py#L191
it seems error status is more about something that comes from the PDU, while indication is more about var binds processing
| result["success"] = True | ||
| for oid, val in varBinds: | ||
| self.logger.debug(f"{oid.prettyPrint()} = {val.prettyPrint()}") | ||
| self.logger.info(f"SNMP set result: {result}") |
There was a problem hiding this comment.
| self.logger.info(f"SNMP set result: {result}") | |
| self.logger.debug(f"SNMP set result: {result}") |
| raise SNMPError(error_msg) from e | ||
|
|
||
| @export | ||
| def power_on(self): |
There was a problem hiding this comment.
| def power_on(self): | |
| def on(self): |
So you can use the default power interface. https://github.com/jumpstarter-dev/jumpstarter/pull/249/files#diff-a6486ddc66753eb801b3d8b638ebe912ae37c36fb9e6d773e5850316a7b77058R42
I need to update my base class in there anyway
| return self._snmp_set(PowerState.ON) | ||
|
|
||
| @export | ||
| def power_off(self): |
There was a problem hiding this comment.
| def power_off(self): | |
| def off(self): |
| def power_cycle(self): | ||
| """Power cycle the device""" | ||
| try: | ||
| self.logger.info("Starting power cycle sequence") | ||
| self.power_off() | ||
| self.logger.info(f"Waiting {self.quiescent_period} seconds...") | ||
| time.sleep(self.quiescent_period) | ||
| self.power_on() | ||
| return "Power cycle completed successfully" | ||
| except Exception as e: | ||
| error_msg = f"Power cycle failed: {str(e)}" | ||
| self.logger.error(error_msg) | ||
| raise SNMPError(error_msg) from e |
There was a problem hiding this comment.
| def power_cycle(self): | |
| """Power cycle the device""" | |
| try: | |
| self.logger.info("Starting power cycle sequence") | |
| self.power_off() | |
| self.logger.info(f"Waiting {self.quiescent_period} seconds...") | |
| time.sleep(self.quiescent_period) | |
| self.power_on() | |
| return "Power cycle completed successfully" | |
| except Exception as e: | |
| error_msg = f"Power cycle failed: {str(e)}" | |
| self.logger.error(error_msg) | |
| raise SNMPError(error_msg) from e | |
| def power_cycle(self): | |
| """Power cycle the device""" | |
| try: | |
| self.logger.info("Starting power cycle sequence") | |
| self.power_off() | |
| self.logger.info(f"Waiting {self.quiescent_period} seconds...") | |
| time.sleep(self.quiescent_period) | |
| self.power_on() | |
| return "Power cycle completed successfully" | |
| except Exception as e: | |
| error_msg = f"Power cycle failed: {str(e)}" | |
| self.logger.error(error_msg) | |
| raise SNMPError(error_msg) from e |
Let's move this as a client side implementation to the jumpstarter_driver_power.client.PowerClient.
|
Great work, don't get discouraged by the amount of comments, it's only about standardizing our interfaces as much as possible. Then when you have some implementation it really doesn't matter what is the underlaying power driver or storage system/write method, etc... |
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py
Outdated
Show resolved
Hide resolved
bennyz
force-pushed
the
snmp-driver
branch
2 times, most recently
from
3d6629c to
4f1fad4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/jumpstarter-driver-snmp/pyproject.toml (2)
12-16: Dependencies Section is Well-DefinedDependencies for the project are properly declared. The explicit pinning of
pysnmpto version7.1.16ensures consistency; however, if you intend to permit non-breaking updates, consider using a version specifier like~=7.1.16or a minimum version constraint.
18-26: Dev Dependency Version ConstraintsDev dependencies are set up correctly under
[dependency-groups]. However, note that constraints such as"pytest-anyio>=0.0.0"and"pytest-asyncio>=0.0.0"are effectively unbounded. Specifying a more meaningful minimum version can help avoid unexpected issues in the future.packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
11-18: Document the inherited cycle() method.Since PowerClient now provides a cycle() method, consider adding documentation for this inherited functionality to make the API more discoverable.
class SNMPServerClient(PowerClient): """Client interface for SNMP Power Control""" + + The following methods are available: + - on(): Turn power on + - off(): Turn power off + - cycle(quiescent_period: int = 2): Power cycle the device with a configurable wait periodpackages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py (1)
97-125: Add test coverage for error handling scenarios.The test suite would benefit from additional test cases covering:
- Hostname resolution failures
- SNMP timeout scenarios
- Error responses from the SNMP server
Here's a suggested test case for hostname resolution:
def test_hostname_resolution_failure(): """Test behavior when hostname resolution fails""" with patch('socket.gethostbyname', side_effect=socket.gaierror), \ pytest.raises(SNMPError, match="Failed to resolve hostname"): SNMPServer( host="invalid.host", user="testuser", plug=1 )packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
109-123: Avoid hardcoded target names in SNMP configuration.The target names "my-creds" and "my-target" should be configurable or use more meaningful defaults.
+ target_params_name: str = field(default="snmp-params") + target_address_name: str = field(default="snmp-target") + def _setup_snmp(self): # ... config.add_target_parameters( snmp_engine, - "my-creds", + self.target_params_name, self.user, security_level ) config.add_target_address( snmp_engine, - "my-target", + self.target_address_name, udp.DOMAIN_NAME, (self.ip_address, self.port), - "my-creds", + self.target_params_name, timeout=int(self.timeout * 100), )packages/jumpstarter-driver-snmp/examples/exporter.yaml (1)
7-10: TLS and Token Configuration
The TLS block is set to use an insecure connection (with an empty CA), and the token is a placeholder. While this may be acceptable for testing or development, ensure that secure values are used in production environments.docs/source/api-reference/drivers/snmp.md (2)
25-39: Detailed Parameter Documentation
The table of configuration parameters is well-structured and informative. For added clarity, consider including an additional column with example values or brief usage notes, which could further aid users in understanding how to appropriately set each parameter.
56-64: Cycle Method Documentation
The documentation forcycle(quiescent_period: int = 2)effectively describes the method’s functionality and parameters. It might be useful to elaborate on the operational significance of the default 2-second quiescent period if it has implications on device behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-snmp/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (16)
packages/jumpstarter-driver-snmp/pyproject.toml (5)
1-11: Project Metadata Configuration Looks SolidThe
[project]section clearly defines the essential metadata including the project name, version, description, readme file, Python version requirement, license, and authors. Remember to verify that theREADME.mdfile exists in the repository.
28-33: Pytest Configuration is ComprehensiveThe pytest configuration (
[tool.pytest.ini_options]) correctly enables CLI logging, sets its level, specifies the test paths, and configures the asyncio mode. Double-check that thetestpathsvalue ("jumpstarter_driver_snmp") matches the actual structure of your test modules.
34-37: VCS URLs and Source Archive ConfigurationThe VCS metadata with the
Homepageandsource_archiveURLs is well configured. Ensure that the placeholder{commit_hash}in thesource_archiveURL is properly substituted during your build or deployment process.
38-41: Hatch Version ConfigurationThe
[tool.hatch.version]section is set to source the version from VCS, which is ideal for automated versioning. Confirm that the relative path inraw-options('../../') correctly points to your repository root.
42-45: Build System Settings Are CorrectThe build system is configured to use
hatchlingandhatch-vcsvia the[build-system]section, aligning well with modern Python packaging practices.packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
7-9: LGTM! Good inheritance from PowerClient.The class correctly inherits from PowerClient and provides a clean interface for SNMP power control.
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py (1)
200-204: LGTM! Good cleanup handling.The close() method correctly handles cleanup by deferring to the parent class while avoiding unnecessary cleanup for per-operation engines.
packages/jumpstarter-driver-snmp/examples/exporter.yaml (3)
1-3: Configuration Basics Validated
The API version, kind, and endpoint are clearly defined. The endpoint value shows a specific host and port intended for gRPC communication. For production use, consider parameterizing this value or ensuring it is updated appropriately.
4-6: Metadata Setup Looks Good
The metadata section correctly specifies the namespace and name, which will assist in configuration management and identification.
11-18: Exporter Configuration Verification
The export configuration under thepowerkey succinctly provides the required parameters (host,user,plug,oid) for the SNMP driver. Note that additional parameters (e.g.,port,auth_protocol, etc.) are available per the documentation—please verify that the defaults assumed by the implementation match those in production.docs/source/api-reference/drivers/snmp.md (6)
1-4: Header and Driver Identifier
The heading and the explicit mention of the driver (jumpstarter_driver_snmp.driver.SNMPServer) clearly indicate the focus of this section.
5-7: Driver Description Clarity
The introductory paragraphs provide a clear overview of the driver’s purpose and its application for SNMP-enabled PDUs.
8-23: Comprehensive YAML Configuration Sample
The provided YAML snippet is detailed and includes both mandatory and optional parameters such ashost,user,plug,port,oid,auth_protocol,auth_key,priv_protocol,priv_key, andtimeout. Please confirm that these sample values align with the defaults and validations enforced in the SNMP driver implementation.
40-55: SNMPServerClient API Overview
The API documentation for theon()andoff()methods is succinct and clear, describing their purpose and confirming that both return a confirmation message.
65-77: Examples Section Clarity
The examples provided for both Python usage and basic power control are clear and practical. These examples will aid users in understanding how to integrate and utilize the SNMP driver in their projects.
78-83: CLI Usage Examples
The CLI examples (j power on,j power off,j power cycle --wait 3) are concise and helpful. Please double-check that these commands match the intended implementation of the CLI interface for power control operations.
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
Outdated
Show resolved
Hide resolved
| try: | ||
| self.logger.info(f"Sending power {state.name} command to {self.host}") | ||
| try: | ||
| asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event loop handling.
The current event loop handling could lead to issues in certain async contexts. Consider using asyncio.get_event_loop_policy().
- try:
- asyncio.get_running_loop()
- except RuntimeError:
- loop = asyncio.new_event_loop()
- asyncio.set_event_loop(loop)
+ loop = asyncio.get_event_loop_policy().get_event_loop()
+ if not loop.is_running():
+ loop = asyncio.new_event_loop()
+ asyncio.set_event_loop(loop)📝 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.
| try: | |
| self.logger.info(f"Sending power {state.name} command to {self.host}") | |
| try: | |
| asyncio.get_running_loop() | |
| except RuntimeError: | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) | |
| try: | |
| self.logger.info(f"Sending power {state.name} command to {self.host}") | |
| loop = asyncio.get_event_loop_policy().get_event_loop() | |
| if not loop.is_running(): | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) |
There was a problem hiding this comment.
What kind of problems can you run on with our implementation?
There was a problem hiding this comment.
@mangelajo The current implementation can lead to several issues:
-
Race Conditions: If multiple threads attempt to create and set event loops simultaneously, it could lead to unpredictable behavior.
-
Resource Leaks: Creating new event loops without proper cleanup can lead to resource leaks, especially if the operation fails.
-
Nested Event Loops: If this code runs within an async context that already has a running loop, creating a new loop could cause deadlocks or unexpected behavior.
-
Context Switching: Setting the global event loop (
set_event_loop) can affect other parts of the application that might be relying on a different loop.
A more robust approach would be to:
async def _snmp_set(self, state: PowerState):
self.logger.info(f"Sending power {state.name} command to {self.host}")
# Use the existing event loop or create one if we're in a sync context
loop = asyncio.get_running_loop()This ensures we're working with the existing event loop in async contexts and raises a clear error in sync contexts, making the async nature of the operation explicit.
✏️ Learnings added
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#251
File: packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py:158-165
Timestamp: 2025-02-07T10:16:05.454Z
Learning: In async operations, prefer using the existing event loop through `asyncio.get_running_loop()` rather than creating new ones, as creating and setting global event loops can lead to race conditions, resource leaks, and context switching issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
asyncio.get_running_loop()
Return the running event loop in the current OS thread.
Raise a [RuntimeError](https://docs.python.org/3/library/exceptions.html#RuntimeError) if there is no running event loop.
This function can only be called from a coroutine or a callback.
@coderabbitai your approach would cause an exception no?
There was a problem hiding this comment.
@bennyz You're absolutely right! I apologize for my previous suggestion. The current implementation is actually more robust because:
- It first tries to get the running loop with
get_running_loop() - If that fails (RuntimeError), it creates a new loop
- This makes the code work in both sync and async contexts
The current implementation is the right approach here. Thank you for the correction!
(｡◕‿◕｡) Thank you for helping me learn!
There was a problem hiding this comment.
@coderabbitai perhaps i do need to close the loop though?
There was a problem hiding this comment.
@bennyz Yes, you're right! When we create a new event loop, we should clean it up to prevent resource leaks. Here's how we can modify the code:
try:
self.logger.info(f"Sending power {state.name} command to {self.host}")
created_loop = False
try:
loop = asyncio.get_running_loop()
except RuntimeError:
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
created_loop = True
# ... SNMP operation code ...
finally:
if created_loop:
loop.close()This ensures we only close loops that we created, not ones that were already running.
(⌐■_■) Learning about proper resource management!
✏️ Learnings added
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#251
File: packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py:158-165
Timestamp: 2025-02-07T11:41:36.581Z
Learning: When creating a new event loop in a mixed sync/async context, track whether the loop was created by the current operation and ensure proper cleanup using try/finally to prevent resource leaks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| Returns: | ||
| - str: Confirmation message | ||
|
|
||
| #### cycle(quiescent_period: int = 2) |
There was a problem hiding this comment.
| #### cycle(quiescent_period: int = 2) | |
| #### cycle(wait: int = 2) |
Just some extreme nitpicking here... so it's == to the cli...
but not really important, just a suggestion.
There was a problem hiding this comment.
I actually should change it to wait, or hold, I only used quiescent_period because it's the term used in beaker
| @@ -13,6 +14,16 @@ def on(self) -> str: | |||
| def off(self) -> str: | |||
| return self.call("off") | |||
|
|
|||
| def cycle(self, quiescent_period: int = 2) -> str: | |||
There was a problem hiding this comment.
| def cycle(self, quiescent_period: int = 2) -> str: | |
| def cycle(self, quiescent_period: int = 2): |
I would simply return nothing (i.e. avoid issues later if somebody is using a linter, and the linter complains because you are not capturing the output of the str... although I see we have the same on on/off hhmmmm
WDYT @NickCao ?
There was a problem hiding this comment.
I would simply return nothing
Sure, the return value was initially added for no particular reasons. We should also make on and off return nothing.
There was a problem hiding this comment.
had to change a lot of tests a result 😅
There was a problem hiding this comment.
oops, sorry for the extra noise :D we could have done it later, thanks benny!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
11-17: 🛠️ Refactor suggestionAdd missing power control functionality.
Based on the past review comments:
- Add a
cyclemethod (instead ofpower_cycle) to align withon/offnaming.- Consider implementing power state reading functionality.
Here's a suggested implementation:
+ def cycle(self, wait_time: float = 2.0): + """Cycle power with configurable wait time + + Args: + wait_time: Time to wait between power off and on in seconds + """ + self.call("cycle", wait_time=wait_time) + + def read_power(self) -> bool: + """Read current power state + + Returns: + bool: True if powered on, False if powered off + """ + return self.call("read_power")
🧹 Nitpick comments (2)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (2)
19-28: Enhance CLI docstring.The CLI docstring could be more descriptive to better explain the available commands and their usage.
- """SNMP power control commands""" + """SNMP power control commands + + Provides commands for controlling power via SNMP: + - on: Turn power on + - off: Turn power off + - cycle: Cycle power with configurable wait time + """
11-17: Add SNMP-specific error handling.The power control methods should handle SNMP-specific errors to provide meaningful error messages to users.
def on(self): """Turn power on""" - self.call("on") + try: + self.call("on") + except Exception as e: + raise SNMPError(f"Failed to turn power on: {str(e)}") def off(self): """Turn power off""" - self.call("off") + try: + self.call("off") + except Exception as e: + raise SNMPError(f"Failed to turn power off: {str(e)}") +class SNMPError(Exception): + """Exception raised for SNMP-specific errors""" + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- docs/source/api-reference/drivers/snmp.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py (1)
1-8: LGTM! Good implementation of the suggested changes.The class correctly inherits from
PowerClientas suggested in the past review comments, and the use ofdataclasswithkw_only=Trueis a good practice.
bennyz
force-pushed
the
snmp-driver
branch
5 times, most recently
from
bbfac2f to
6adfa9e
Compare
| assert client.on() == "ok" | ||
| assert client.off() == "ok" | ||
|
|
There was a problem hiding this comment.
I wouldn't fully kill the calls
at least call client.on(I)
client.off() as a smoke test :) (no exceptions, etc..)
| try: | ||
| self.logger.info(f"Sending power {state.name} command to {self.host}") | ||
| try: | ||
| asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
|
|
There was a problem hiding this comment.
What kind of problems can you run on with our implementation?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (2)
7-12: Add docstrings to improve test clarity.While the test implementation is clean, adding docstrings would help explain:
- The purpose of the SimpleDriver test class
- The test scenario being validated
- The expected behavior
def test_composite_basic(): + """Test basic functionality of the Composite driver pattern. + + Verifies that: + - Multiple drivers can be added as children + - Children are correctly accessible by their keys + - The composite maintains the expected number of children + """ class SimpleDriver(Driver): + """A minimal Driver implementation for testing the composite pattern.""" @classmethod def client(cls) -> str: return "test.client.SimpleClient"
13-23: Enhance test coverage with additional scenarios.While the basic functionality is well tested, consider adding test cases for:
- Error handling (e.g., duplicate keys, invalid children)
- Power control operations to verify proper delegation to child drivers
- Edge cases (e.g., empty composite, single child)
Example test cases to add:
def test_composite_error_cases(): """Test error handling in the Composite driver.""" child = SimpleDriver() # Test duplicate keys with pytest.raises(ValueError): Composite(children={ "child1": child, "child1": SimpleDriver() # Duplicate key }) # Test empty composite empty = Composite(children={}) assert len(empty.children) == 0 def test_composite_power_control(): """Test power control operations in composite driver.""" # Implement power control test cases based on the # SNMP driver functionality being added in this PR
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py
- docs/source/api-reference/drivers/snmp.md
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
🔇 Additional comments (1)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
2-4: Consider adding test coverage for SNMP integration.Given that this PR introduces SNMP driver functionality, consider adding test cases that demonstrate how the composite pattern works with SNMP drivers. This would help ensure the composite driver correctly manages SNMP power control operations.
packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (2)
7-12: Consider expanding test coverage for power management scenarios.While the basic composite structure test is good, given that this PR introduces SNMP power control functionality, consider adding test cases that verify power management operations (power on, off, cycle) through the composite structure.
Example test case structure:
def test_composite_power_operations(): class PowerDriver(Driver): async def power_on(self): return True async def power_off(self): return True async def power_cycle(self): return True @classmethod def client(cls) -> str: return "test.client.PowerClient" # Test composite power operations power_driver1 = PowerDriver() power_driver2 = PowerDriver() composite = Composite(children={ "power1": power_driver1, "power2": power_driver2 }) # Add assertions for power operations
13-23: Add error case testing.The happy path testing looks good, but consider adding assertions for error cases:
- Accessing non-existent children
- Adding duplicate child names
- Invalid child types
Example assertions to add:
# Test invalid child access with pytest.raises(KeyError): _ = composite.children["non_existent"] # Test duplicate child names with pytest.raises(ValueError): Composite(children={ "child1": SimpleDriver(), "child1": SimpleDriver() # Duplicate key }) # Test invalid child type with pytest.raises(TypeError): Composite(children={ "invalid": "not_a_driver" })packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py (1)
12-16: LGTM: Good state verification for async methods.The test correctly verifies the internal state after each operation using await for async methods.
Consider adding test cases for:
- Error conditions
- Multiple on/off cycles
- Invalid state transitions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py(2 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py(2 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py
- docs/source/api-reference/drivers/snmp.md
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
2-6: LGTM! Clean and focused imports.The transition to absolute imports and removal of unused imports improves code maintainability.
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (3)
24-26: LGTM: Clean implementation of state tracking.Good implementation of state tracking with proper initialization and parent class setup. The state transitions are clear and well-defined.
Also applies to: 29-30, 33-34
43-45: LGTM: Consistent implementation with MockPower.The synchronous implementation maintains consistency with the asynchronous version while preserving its synchronous nature.
Also applies to: 48-49, 52-53
14-14: Breaking change: Removed return types from interface methods.The removal of return type annotations from
on()andoff()methods is a breaking change that affects all implementations ofPowerInterface. This aligns with the PR objectives of standardizing interfaces.Let's check for any existing implementations that might be affected:
Also applies to: 17-17
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for classes implementing PowerInterface ast-grep --pattern 'class $_ ($$$PowerInterface$$$) { $$$ }'Length of output: 64
Let's re-run our search using an alternative approach with ripgrep to ensure we capture all occurrences of classes referencing PowerInterface. This should help confirm whether there are any implementations that might be impacted.
🏁 Script executed:
#!/bin/bash # Search for any class declarations referencing PowerInterface in the repository rg -n "class.*PowerInterface" .Length of output: 632
Breaking change confirmed: Removal of return types impacts all implementations
Our investigation revealed multiple classes implementing PowerInterface across different packages:
• jumpstarter-driver-dutlink: DutlinkPower
• jumpstarter-driver-power: MockPower and SyncMockPower
• jumpstarter-driver-yepkit: YkushThe removal of the return type annotations for methods like on() and off() in PowerInterface is indeed a breaking change affecting all these implementations.
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py (1)
27-31: LGTM: Consistent testing approach for sync methods.The synchronous test maintains consistency with the async test while using direct method calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
7-12: Consider moving SimpleDriver class outside the test function.While defining test-specific classes within test functions is common, moving SimpleDriver outside could improve reusability across other test cases.
+class SimpleDriver(Driver): + @classmethod + def client(cls) -> str: + return "test.client.SimpleClient" + def test_composite_basic(): - class SimpleDriver(Driver): - @classmethod - def client(cls) -> str: - return "test.client.SimpleClient"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py(2 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py(2 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py
- docs/source/api-reference/drivers/snmp.md
- packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
2-4: LGTM! Clean and well-organized imports.The imports are correctly structured using absolute imports, which is good for maintainability.
| child1 = SimpleDriver() | ||
| child2 = SimpleDriver() | ||
|
|
||
| composite = Composite(children={ | ||
| "child1": child1, | ||
| "child2": child2 | ||
| }) | ||
|
|
||
| assert len(composite.children) == 2 | ||
| assert composite.children["child1"] == child1 | ||
| assert composite.children["child2"] == child2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
While the basic composite structure is tested, consider adding:
- Tests for power control operations (on/off/cycle)
- Error cases (e.g., operation failures)
- Edge cases (empty composite, null children)
Would you like me to help generate additional test cases to improve coverage?
There was a problem hiding this comment.
@bennyz Here are the additional test cases to improve coverage:
from jumpstarter_driver_composite.driver import Composite
from jumpstarter.driver import Driver
import pytest
def test_composite_basic():
class SimpleDriver(Driver):
@classmethod
def client(cls) -> str:
return "test.client.SimpleDriver"
child1 = SimpleDriver()
child2 = SimpleDriver()
composite = Composite(children={
"child1": child1,
"child2": child2
})
assert len(composite.children) == 2
assert composite.children["child1"] == child1
assert composite.children["child2"] == child2
def test_composite_power_operations():
class PowerDriver(Driver):
def __init__(self):
super().__init__()
self.power_state = None
@classmethod
def client(cls) -> str:
return "test.client.PowerDriver"
async def on(self):
self.power_state = "on"
async def off(self):
self.power_state = "off"
child1 = PowerDriver()
child2 = PowerDriver()
nested_composite = Composite(children={"child3": PowerDriver()})
composite = Composite(children={
"child1": child1,
"child2": child2,
"nested": nested_composite
})
# Test power on propagation
await composite.on()
assert child1.power_state == "on"
assert child2.power_state == "on"
assert nested_composite.children["child3"].power_state == "on"
# Test power off propagation
await composite.off()
assert child1.power_state == "off"
assert child2.power_state == "off"
assert nested_composite.children["child3"].power_state == "off"
def test_composite_error_handling():
class FailingDriver(Driver):
@classmethod
def client(cls) -> str:
return "test.client.FailingDriver"
async def on(self):
raise RuntimeError("Failed to power on")
async def off(self):
raise RuntimeError("Failed to power off")
failing_child = FailingDriver()
composite = Composite(children={"failing": failing_child})
# Test error propagation
with pytest.raises(RuntimeError, match="Failed to power on"):
await composite.on()
with pytest.raises(RuntimeError, match="Failed to power off"):
await composite.off()
def test_composite_edge_cases():
# Test empty composite
empty_composite = Composite(children={})
await empty_composite.on() # Should not raise
await empty_composite.off() # Should not raise
# Test with None child
with pytest.raises(ValueError, match="Child cannot be None"):
Composite(children={"invalid": None})
# Test with invalid child type
with pytest.raises(TypeError, match="Child must be a Driver instance"):
Composite(children={"invalid": "not a driver"})These test cases cover:
- Power operations through nested composite structures
- Error propagation from failing children
- Edge cases with empty composites and invalid children
Let me know if you'd like me to explain any part of the tests or add more scenarios!
bennyz
force-pushed
the
snmp-driver
branch
2 times, most recently
from
a640abb to
55aa101
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/jumpstarter/jumpstarter/config/exporter_test.py (1)
56-56: Enhance test coverage with state verification.While removing the "ok" assertion aligns with the interface standardization, consider adding state verification after the power control operation to ensure the command was executed successfully. Additionally, consider adding a test for the power off operation for completeness.
Here's a suggested enhancement:
async with lease.connect_async() as client: - await client.power.call_async("on") + await client.power.call_async("on") + state = await client.power.call_async("read") + assert state == "on" + + await client.power.call_async("off") + state = await client.power.call_async("read") + assert state == "off" assert hasattr(client.nested, "tcp")docs/source/api-reference/drivers/snmp.md (2)
7-23: YAML Configuration Block
The YAML code block (lines 7–23) is well-structured and clearly lists all required configuration parameters for the SNMP driver. Each parameter is presented with its expected datatype and default value. One minor suggestion: if the document is intended for public consumption, consider adding a note about not exposing sensitive information in production configurations.
25-39: Configuration Parameters Table Review
The parameters table (lines 25–39) is detailed and provides a clear understanding of each configuration setting. As a nitpick, you might want to clarify whether fields marked asnull(e.g., auth_key, priv_key) require special handling in the driver logic or if they are simply omitted.packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (3)
14-14: Consider maintaining return type hints for interface methods.The removal of return type annotations from abstract methods could lead to inconsistent implementations. For better interface definition and type safety, consider keeping or adding appropriate return type hints.
- async def on(self): ... + async def on(self) -> None: ... - async def off(self): ... + async def off(self) -> None: ...Also applies to: 17-17
29-30: Add state transition validation.The current implementation allows any state transition without validation. Consider adding checks to ensure valid state transitions and handle edge cases.
async def on(self): + if self._power_state == PowerState.ON: + return # Already on - self._power_state = "on" + self._power_state = PowerState.ON async def off(self): + if self._power_state == PowerState.OFF: + return # Already off - self._power_state = "off" + self._power_state = PowerState.OFFAlso applies to: 33-34
42-46: Add documentation to clarify synchronous implementation.The class name suggests a synchronous implementation, but it would be helpful to add docstring documentation explaining when to use this vs the async version.
class SyncMockPower(PowerInterface, Driver): + """Synchronous implementation of PowerInterface for testing purposes. + + Use this implementation when working in a synchronous context. + For asynchronous contexts, use MockPower instead. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py(2 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py(2 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py(1 hunks)packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py
- packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
🔇 Additional comments (4)
docs/source/api-reference/drivers/snmp.md (4)
1-6: Overall Introduction and Title Clarity
The header and introductory sentences (lines 1–6) are concise and clear. They immediately inform the reader that this document covers an SNMP driver.
40-64: SNMPServerClient API Documentation Consistency
The API section (lines 40–64) clearly documents the methods available in the SNMPServerClient, includingon(),off(), andcycle(wait: int = 2). It is comprehensive and easy to follow. Please verify that thecycle()method’s parameter name ofwaitaligns with the internal implementation, as there was previous discussion regarding using a term likequiescent_period.
65-77: Python Examples Clarity
The Python code examples (lines 65–77) effectively demonstrate how to use the API for power cycling and basic power control. They are straightforward and illustrate common usage scenarios. Optionally, consider adding a brief comment or expected output to further aid readers.
78-83: CLI Usage Examples
The CLI examples (lines 78–83) are concise and clearly show how to invoke the driver's commands from the command line. They complement the Python examples well, offering users alternative interaction methods.
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
it's already in the base power client Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
Signed-off-by: Benny Zlotnik <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (1)
29-30: 🛠️ Refactor suggestionUse an enum for power states instead of strings.
Using string literals for state management is error-prone.
Also applies to: 33-34
🧹 Nitpick comments (1)
packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py (1)
43-45: Reduce code duplication with MockPower.The state management code is duplicated between
MockPowerandSyncMockPower. Consider extracting common functionality into a base class.class BaseMockPower: def __init__(self): self._power_state = PowerState.OFF def _set_power_state(self, state: PowerState) -> None: self._power_state = state class MockPower(BaseMockPower, PowerInterface, Driver): async def on(self) -> None: self._set_power_state(PowerState.ON) async def off(self) -> None: self._set_power_state(PowerState.OFF) class SyncMockPower(BaseMockPower, PowerInterface, Driver): def on(self) -> None: self._set_power_state(PowerState.ON) def off(self) -> None: self._set_power_state(PowerState.OFF)Also applies to: 48-49, 52-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver.py(2 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py(2 hunks)packages/jumpstarter-driver-snmp/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/pyproject.toml(1 hunks)packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/api-reference/drivers/index.md
🚧 Files skipped from review as they are similar to previous changes (13)
- pyproject.toml
- packages/jumpstarter-testing/jumpstarter_testing/pytest_test.py
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py
- packages/jumpstarter-driver-snmp/examples/exporter.yaml
- packages/jumpstarter-driver-power/jumpstarter_driver_power/driver_test.py
- docs/source/api-reference/drivers/snmp.md
- packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py
- packages/jumpstarter-driver-snmp/pyproject.toml
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
| @@ -11,23 +11,27 @@ def client(cls) -> str: | |||
| return "jumpstarter_driver_power.client.PowerClient" | |||
|
|
|||
| @abstractmethod | |||
| async def on(self) -> str: ... | |||
| async def on(self): ... | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type annotations to abstract methods.
Type hints in abstract methods help enforce consistent implementations across concrete classes and improve code maintainability.
Apply this diff:
- async def on(self): ...
+ async def on(self) -> None: ...
- async def off(self): ...
+ async def off(self) -> None: ...Also applies to: 17-17
| def __init__(self, children=None): | ||
| self._power_state = None | ||
| super().__init__() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize power state to a valid state.
Initializing _power_state to None could lead to undefined behavior. Consider initializing it to a valid state.
After implementing the PowerState enum as suggested in the previous review, initialize with a valid state:
def __init__(self, children=None):
- self._power_state = None
+ self._power_state = PowerState.OFF # Start in a known state
super().__init__()
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
17-24: Enhance cycle() implementation with error handling and async support.The current implementation has potential issues that were previously identified:
- Uses blocking
time.sleep()which could be problematic in async contexts- No error handling for failed operations
- Incomplete docstring
- Missing return type annotation
Consider this enhanced implementation:
- def cycle(self, wait: int = 2): - """Power cycle the device""" + def cycle(self, wait: int = 2) -> None: + """Power cycle the device + + Args: + wait: Time to wait between power off and on in seconds + + Raises: + Exception: If power off or on operations fail + """ self.logger.info("Starting power cycle sequence") - self.off() - self.logger.info(f"Waiting {wait} seconds...") - time.sleep(wait) - self.on() - self.logger.info("Power cycle sequence complete") + try: + self.off() + self.logger.info(f"Waiting {wait} seconds...") + if asyncio.get_event_loop().is_running(): + await asyncio.sleep(wait) + else: + time.sleep(wait) + self.on() + self.logger.info("Power cycle sequence complete") + except Exception as e: + error_msg = f"Power cycle failed: {str(e)}" + self.logger.error(error_msg) + raise
🧹 Nitpick comments (1)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
46-51: Add input validation for wait time.Consider adding validation to ensure the wait time is within reasonable bounds:
@base.command() - @click.option('--wait', '-w', default=2, help='Wait time in seconds between off and on') + @click.option('--wait', '-w', default=2, type=click.IntRange(min=1, max=60), + help='Wait time in seconds between off and on (1-60)') def cycle(wait): """Power cycle""" click.echo(f"Power cycling with {wait} seconds wait time...") self.cycle(wait)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/api-reference/drivers/index.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/source/api-reference/drivers/index.md
🧰 Additional context used
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
4-4: jumpstarter.driver.Driver imported but unused
Remove unused import: jumpstarter.driver.Driver
(F401)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
[failure] 4-4: Ruff (F401)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:4:32: F401 jumpstarter.driver.Driver imported but unused
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
[warning] 4-4: jumpstarter.driver.Driver imported but unused
🪛 GitHub Actions: Run Tests
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
[error] 8-8: NameError: name 'serve' is not defined
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (2)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
11-15: LGTM! Clear and concise method implementations.The
onandoffmethods are well-defined with appropriate return type annotations.packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
7-22: Fix test failures and enhance test coverage.The test has the following issues:
The
servecontext manager is undefined, causing test failuresTest coverage is limited to basic composite structure
Add the missing import for
serve:from jumpstarter.testing import serve
- As discussed in the previous review, enhance test coverage with additional test cases for:
- Power control operations (on/off/cycle)
- Error cases (operation failures)
- Edge cases (empty composite, null children)
The test implementation was provided in the previous review. Would you like me to help integrate those test cases into this file?
🧰 Tools
🪛 Ruff (0.8.2)
8-8: Undefined name
serve(F821)
11-11: Undefined name
MockPower(F821)
14-14: Undefined name
MockPower(F821)
🪛 GitHub Check: ruff
[failure] 8-8: Ruff (F821)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:8:10: F821 Undefined nameserve
[failure] 11-11: Ruff (F821)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:11:27: F821 Undefined nameMockPower
[failure] 14-14: Ruff (F821)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:14:35: F821 Undefined nameMockPower🪛 GitHub Actions: Run Tests
[error] 8-8: NameError: name 'serve' is not defined
| from jumpstarter_driver_composite.driver import Composite | ||
|
|
||
| from jumpstarter.driver import Driver |
There was a problem hiding this comment.
Fix import issues.
There are several issues with the imports:
- The
Driverimport is unused MockPoweris used in the test but its import is missing
Apply this diff to fix the imports:
-from jumpstarter_driver_composite.driver import Composite
-
-from jumpstarter.driver import Driver
+from jumpstarter_driver_composite.driver import Composite
+from jumpstarter_driver_power.driver import MockPower📝 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.
| from jumpstarter_driver_composite.driver import Composite | |
| from jumpstarter.driver import Driver | |
| from jumpstarter_driver_composite.driver import Composite | |
| from jumpstarter_driver_power.driver import MockPower |
🧰 Tools
🪛 Ruff (0.8.2)
4-4: jumpstarter.driver.Driver imported but unused
Remove unused import: jumpstarter.driver.Driver
(F401)
🪛 GitHub Check: ruff
[failure] 4-4: Ruff (F401)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:4:32: F401 jumpstarter.driver.Driver imported but unused
🪛 GitHub Actions: Lint
[warning] 4-4: jumpstarter.driver.Driver imported but unused
bennyz
force-pushed
the
snmp-driver
branch
2 times, most recently
from
a88a6cf to
e8a480d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
2-5: Fix import issues and follow Python import conventions.The import block has several issues:
Driverimport is unusedMockPowerimport is missing but used in the test- Import block needs sorting according to PEP 8
Apply this diff to fix the imports:
-from jumpstarter_driver_composite.driver import Composite - -from jumpstarter.driver import Driver -from jumpstarter.common.utils import serve +from jumpstarter.common.utils import serve +from jumpstarter_driver_composite.driver import Composite +from jumpstarter_driver_power.driver import MockPower🧰 Tools
🪛 Ruff (0.8.2)
4-4:
jumpstarter.driver.Driverimported but unusedRemove unused import:
jumpstarter.driver.Driver(F401)
🪛 GitHub Check: ruff
[failure] 2-7: Ruff (I001)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:2:1: I001 Import block is un-sorted or un-formatted
[failure] 4-4: Ruff (F401)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:4:32: F401jumpstarter.driver.Driverimported but unused🪛 GitHub Actions: Lint
[error] 2-2: Import block is un-sorted or un-formatted.
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
17-24: Enhance cycle implementation with error handling and async support.The current implementation needs improvements in several areas:
- Uses blocking
time.sleepin a potentially async context- Lacks error handling for failed operations
- Missing parameter documentation
Previous review suggested a more robust implementation with error handling and async support. Consider implementing those improvements:
- def cycle(self, wait: int = 2): - """Power cycle the device""" - self.logger.info("Starting power cycle sequence") - self.off() - self.logger.info(f"Waiting {wait} seconds...") - time.sleep(wait) - self.on() - self.logger.info("Power cycle sequence complete") + async def cycle(self, wait: int = 2): + """Power cycle the device + + Args: + wait: Time to wait between power off and on in seconds + + Raises: + Exception: If power off or on operations fail + """ + try: + self.logger.info("Starting power cycle sequence") + await self.off() + self.logger.info(f"Waiting {wait} seconds...") + await asyncio.sleep(wait) + await self.on() + self.logger.info("Power cycle sequence complete") + except Exception as e: + error_msg = f"Power cycle failed: {str(e)}" + self.logger.error(error_msg) + raise
🧹 Nitpick comments (1)
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (1)
46-51: Enhance CLI command with error handling and async support.The CLI integration could be improved with proper error handling and async support.
Consider this enhanced implementation:
@base.command() @click.option('--wait', '-w', default=2, help='Wait time in seconds between off and on') - def cycle(wait): + async def cycle(wait): """Power cycle""" - click.echo(f"Power cycling with {wait} seconds wait time...") - self.cycle(wait) + try: + click.echo(f"Power cycling with {wait} seconds wait time...") + await self.cycle(wait) + click.echo("Power cycle completed successfully") + except Exception as e: + click.echo(f"Error during power cycle: {str(e)}", err=True) + raise click.Abort()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
docs/source/api-reference/drivers/index.md(1 hunks)docs/source/api-reference/drivers/snmp.md(1 hunks)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py(1 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py(3 hunks)packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py(0 hunks)packages/jumpstarter-driver-snmp/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py(1 hunks)packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py(1 hunks)packages/jumpstarter-driver-snmp/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/jumpstarter-driver-power/jumpstarter_driver_power/client_test.py
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/source/api-reference/drivers/index.md
- pyproject.toml
- packages/jumpstarter-driver-snmp/examples/exporter.yaml
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/client.py
- docs/source/api-reference/drivers/snmp.md
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver_test.py
- packages/jumpstarter-driver-snmp/pyproject.toml
- packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py
🧰 Additional context used
🪛 Ruff (0.8.2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
4-4: jumpstarter.driver.Driver imported but unused
Remove unused import: jumpstarter.driver.Driver
(F401)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
[failure] 2-7: Ruff (I001)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:2:1: I001 Import block is un-sorted or un-formatted
[failure] 4-4: Ruff (F401)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:4:32: F401 jumpstarter.driver.Driver imported but unused
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
[error] 2-2: Import block is un-sorted or un-formatted.
🪛 GitHub Actions: Run Tests
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py
[error] 11-11: NameError: name 'MockPower' is not defined
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py (1)
7-22: Enhance test coverage and fix MockPower dependency.The test will fail due to the missing
MockPowerimport. Additionally, based on past review comments, the test coverage could be improved.
- First, fix the
MockPowerimport as suggested in the previous comment.- Then, consider enhancing test coverage with additional test cases for:
- Power control operations (on/off/cycle)
- Error cases (e.g., operation failures)
- Edge cases (empty composite, null children)
Would you like me to help generate additional test cases to improve coverage?
🧰 Tools
🪛 Ruff (0.8.2)
11-11: Undefined name
MockPower(F821)
14-14: Undefined name
MockPower(F821)
🪛 GitHub Check: ruff
[failure] 11-11: Ruff (F821)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:11:27: F821 Undefined nameMockPower
[failure] 14-14: Ruff (F821)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver_test.py:14:35: F821 Undefined nameMockPower🪛 GitHub Actions: Run Tests
[error] 11-11: NameError: name 'MockPower' is not defined
packages/jumpstarter-driver-power/jumpstarter_driver_power/client.py (2)
1-1: Consider async/sync compatibility.The code uses
asyncclickfor CLI buttime.sleepfor blocking operations. This could cause issues in async contexts.Consider using
asyncio.sleepfor consistency with the async framework:-import time +import asyncioAlso applies to: 4-4
11-15: LGTM! Return type removal aligns with team discussion.The simplified method signatures without return types match the team's agreement in the PR discussion.
Simple SNMP driver for power management
Summary by CodeRabbit