Small cli fixes #1763
Small cli fixes #1763
Conversation
Signed-off-by: Radek Ježek <[email protected]>
dosubot
bot
added
the
size:L
Summary of ChangesHello @jezekra1, 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 introduces several small but impactful fixes and enhancements across the Agent Stack CLI and an example agent. The primary focus is on improving the 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. 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
|
![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 several small fixes and improvements to the CLI, particularly around the agent build and run processes. The changes correctly handle form data in the form agent, remove problematic stdin reading, and update the build process to support an internal image registry.
My review includes a few suggestions to improve robustness and maintainability:
- Reinstating a check for the platform's running state in the build command to provide clearer error messages.
- Removing duplicated code in the Lima driver.
- Making exception handling more specific in the Lima driver.
Overall, these are good improvements to the CLI's functionality.
| sys.exit(1) | ||
| await driver.import_image(tag) | ||
|
|
||
| await driver.import_image_to_internal_registry(tag) |
There was a problem hiding this comment.
The check to ensure the Agent Stack platform is running has been removed. This could lead to cryptic error messages if the platform is not running when driver.import_image_to_internal_registry(tag) is called. It's better to explicitly check the platform status and provide a clear error message to the user, as was done previously. Please add a check like the following before this line:
if (await driver.status()) != "running":
console.error("Agent Stack platform is not running. Please start it with 'agentstack platform start'.")
raise typer.Exit(1)| except Exception: | ||
| console.warning("Internal registry service not found. Push might fail.") |
There was a problem hiding this comment.
Catching a broad Exception is generally not recommended as it can hide unexpected errors. It's better to catch a more specific exception related to command execution failure. If a broad exception is necessary, logging the exception details would be helpful for debugging.
| except Exception: | |
| console.warning("Internal registry service not found. Push might fail.") | |
| except Exception as e: | |
| console.warning(f"Internal registry service not found. Push might fail: {e}") |
| async def push_to_local_registry(self, tag: str): | ||
| image_dir = anyio.Path("/tmp/agentstack") | ||
| await image_dir.mkdir(exist_ok=True, parents=True) | ||
| image_file = str(uuid.uuid4()) | ||
| image_path = image_dir / image_file | ||
|
|
||
| try: | ||
| await run_command( | ||
| ["docker", "image", "save", "-o", str(image_path), tag], f"Exporting image {tag} from Docker" | ||
| ) | ||
| await self.run_in_vm( | ||
| ["/bin/sh", "-c", f"k3s ctr images import /tmp/agentstack/{image_file}"], | ||
| f"Importing image {tag} into Agent Stack platform", | ||
| ) | ||
| finally: | ||
| await image_path.unlink(missing_ok=True) |
There was a problem hiding this comment.
The method push_to_local_registry appears to be an exact duplicate of the import_image method defined earlier in this file (lines 183-198). Since it's also unused, it should be removed to avoid code duplication and improve maintainability.
Signed-off-by: Radek Ježek <[email protected]>
Signed-off-by: Radek Ježek <[email protected]>
Summary
Linked Issues
Documentation
If this PR adds new feature or changes existing. Make sure documentation is adjusted accordingly. If the docs is not needed, please explain why.