Conversation
Summary of ChangesHello @mesudip, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request overhauls the SSH client functionality by introducing a dedicated server management class, enabling more flexible and robust interactions with remote SSH hosts. It also significantly expands the test suite with Docker-based integration tests and new examples, ensuring the reliability and demonstrating the capabilities of the updated SSH and RPC communication mechanisms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2 +/- ##
==========================================
- Coverage 85.65% 83.38% -2.28%
==========================================
Files 28 28
Lines 1220 1294 +74
Branches 126 137 +11
==========================================
+ Hits 1045 1079 +34
- Misses 133 170 +37
- Partials 42 45 +3 ☔ View full report in Codecov by Sentry. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the SSH functionality by creating a new SshServer class to manage connections, allowing for multiplexing commands over a single SSH connection. The changes also include extensive new tests and examples, which greatly improve the project's test coverage and usability. However, I've identified a few critical issues, including a syntax error and incorrect API usage in the new examples, as well as some areas for improvement in error handling and event naming. Addressing these points will help ensure the new features are robust and easy to use.
| if isinstance(self._ssh_channel, asyncssh.SSHClientChannel): | ||
| cl_chan: asyncssh.SSHClientChannel = self._ssh_channel | ||
| print("Force aborting SSH channel...") | ||
| cl_chan.abort() | ||
| elif isinstance(self._ssh_channel, asyncssh.SSHClientProcess): | ||
| print("Force killing SSH process...") | ||
| cl_proc: asyncssh.SSHClientProcess = self._ssh_channel | ||
| cl_proc.kill() |
There was a problem hiding this comment.
This block of code has an extra level of indentation, which will cause a SyntaxError. It should be aligned with the except asyncio.TimeoutError: block.
| if isinstance(self._ssh_channel, asyncssh.SSHClientChannel): | |
| cl_chan: asyncssh.SSHClientChannel = self._ssh_channel | |
| print("Force aborting SSH channel...") | |
| cl_chan.abort() | |
| elif isinstance(self._ssh_channel, asyncssh.SSHClientProcess): | |
| print("Force killing SSH process...") | |
| cl_proc: asyncssh.SSHClientProcess = self._ssh_channel | |
| cl_proc.kill() | |
| if isinstance(self._ssh_channel, asyncssh.SSHClientChannel): | |
| cl_chan: asyncssh.SSHClientChannel = self._ssh_channel | |
| print("Force aborting SSH channel...") | |
| cl_chan.abort() | |
| elif isinstance(self._ssh_channel, asyncssh.SSHClientProcess): | |
| print("Force killing SSH process...") | |
| cl_proc: asyncssh.SSHClientProcess = self._ssh_channel | |
| cl_proc.kill() |
| server = SshServer( | ||
| host=args.host, | ||
| port=args.port, | ||
| username=args.user, | ||
| password=args.password, | ||
| ) | ||
| await server.connect() |
There was a problem hiding this comment.
The SshServer is being instantiated incorrectly. The SshServer.connect() is a class method that should be awaited to get a connected server instance. The current code attempts to instantiate SshServer directly without a connection object, which will fail, and then calls a non-existent instance method connect().
| server = SshServer( | |
| host=args.host, | |
| port=args.port, | |
| username=args.user, | |
| password=args.password, | |
| ) | |
| await server.connect() | |
| server = await SshServer.connect( | |
| host=args.host, | |
| port=args.port, | |
| username=args.user, | |
| password=args.password, | |
| ) |
| server = SshServer( | ||
| host=args.host, | ||
| port=args.port, | ||
| username=args.user, | ||
| password=args.password, | ||
| ) | ||
|
|
||
| try: | ||
| await server.connect() | ||
| except Exception as e: | ||
| print(f"Failed to connect: {e}") | ||
| return |
There was a problem hiding this comment.
The SshServer is being instantiated and used incorrectly. SshServer.connect() is an async class method that should be awaited to create and connect the server instance. The current code will raise an error.
| server = SshServer( | |
| host=args.host, | |
| port=args.port, | |
| username=args.user, | |
| password=args.password, | |
| ) | |
| try: | |
| await server.connect() | |
| except Exception as e: | |
| print(f"Failed to connect: {e}") | |
| return | |
| try: | |
| server = await SshServer.connect( | |
| host=args.host, | |
| port=args.port, | |
| username=args.user, | |
| password=args.password, | |
| ) | |
| except Exception as e: | |
| print(f"Failed to connect: {e}") | |
| return |
| return host | ||
| return "localhost" | ||
|
|
||
| @pytest.fixture(scope="session") |
There was a problem hiding this comment.
There seems to be a duplication of SSH test fixtures between this file (root conftest.py) and tests/conftest.py. The fixtures in tests/conftest.py seem to follow a better pattern (e.g., separating config from the connected server instance) and are likely the intended ones. To avoid confusion and maintain a single source of truth for test setup, it would be best to consolidate these fixtures into tests/conftest.py and remove the duplicated ones from this file.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Catching a broad Exception and silently passing can hide potential issues during the drain operation. It would be beneficial to log or print the exception to aid in debugging, even if the program flow can continue.
| except Exception: | |
| pass | |
| except Exception as e: | |
| print(f"AioPipe: Failed to drain writer on EOF: {e}") |
| print("Sent EOF to SSH channel, waiting for it to close...") | ||
|
|
||
| print("Terminating SSH channel...") | ||
| self._ssh_channel.terminate() | ||
| try: | ||
| # Wait "for" graceful closure for a short time | ||
| print("waiting") | ||
| await asyncio.wait_for(self._ssh_channel.wait_closed(), timeout=4.0) | ||
| except asyncio.TimeoutError: | ||
| # If timed out, try to force terminate | ||
|
|
||
| if isinstance(self._ssh_channel, asyncssh.SSHClientChannel): | ||
| cl_chan: asyncssh.SSHClientChannel = self._ssh_channel | ||
| print("Force aborting SSH channel...") | ||
| cl_chan.abort() | ||
| elif isinstance(self._ssh_channel, asyncssh.SSHClientProcess): | ||
| print("Force killing SSH process...") | ||
| cl_proc: asyncssh.SSHClientProcess = self._ssh_channel | ||
| cl_proc.kill() |
There was a problem hiding this comment.
The terminate method contains several print statements for debugging. For production-quality code, these should be replaced with a proper logging mechanism (e.g., using the logging module). This allows for better control over log levels and output destinations.
| if self._ssh_channel: | ||
| await self._ssh_channel.wait_closed() |
There was a problem hiding this comment.
The call to self._ssh_channel.wait_closed() is redundant as it's already called on the preceding line. This block can be removed.
| keys = client_keys | ||
| if ssh_key_content: | ||
| key = asyncssh.import_private_key(ssh_key_content, passphrase=ssh_key_passphrase) | ||
| if keys: | ||
| keys.append(key) | ||
| else: | ||
| keys = [key] |
There was a problem hiding this comment.
The client_keys list is modified in-place if it's provided. This can lead to unexpected side effects for the caller. It's safer to create a copy of the list before appending to it.
keys = list(client_keys) if client_keys else []
if ssh_key_content:
key = asyncssh.import_private_key(ssh_key_content, passphrase=ssh_key_passphrase)
keys.append(key)2 participants
No description provided.