-
Notifications
You must be signed in to change notification settings - Fork 14
Fix ssh option processing, change to-disk to use ssh #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @cgwalters, 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 addresses a critical issue where the Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a critical issue with how command arguments are passed to SSH by introducing proper shell escaping. The refactoring to create a shared CommonSshOptions is also a great improvement for maintainability and consistency. My review includes a couple of suggestions to further enhance these changes: one to improve the flexibility of the new libvirt SSH options, and another to strengthen the tests for the new shell escaping logic.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a critical fix for handling multi-argument commands over SSH by correctly shell-escaping them. The refactoring to centralize common SSH options into a shared struct is a great improvement for code reuse and maintainability. The changes in to-disk to use a detached VM with an SSH-based installation process also appear more robust. I've identified a couple of high-severity issues regarding potential panics on user input and a shell injection vulnerability, along with a medium-severity suggestion to strengthen the new tests. Overall, these are excellent changes that significantly improve the correctness and structure of the SSH handling.
b59a478 to
2d42742
Compare
Signed-off-by: Colin Walters <walters@verbum.org>
I got bit badly by the problem that `ssh` always runs a remote shell and basically just does the completely wrong thing if you pass it multiple things in argv. While we're here fix things so libvirt and ephemeral share more code. Signed-off-by: Colin Walters <walters@verbum.org>
The exec flow is buffered and also doesn't handle things like tty widths etc. We could replicate all of that, but it's just way easier to fork ssh. This feels conceptually less clean in that my preference is actually for systems to be more autonomous, but this way right now is the only way we could sanely get a progress bar for example. Signed-off-by: Colin Walters <walters@verbum.org>
2d42742 to
113ffc6
Compare
The bootc test suite adds this because of testcloud/tmt, and without providing a metadata source we'll just time out. While we're here, namespace our environment variables. Signed-off-by: Colin Walters <walters@verbum.org>
jmarrero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I got bit badly by the problem that
sshalways runsa remote shell and basically just does the completely
wrong thing if you pass it multiple things
in argv.
While we're here fix things so libvirt and ephemeral
share more code.
Signed-off-by: Colin Walters walters@verbum.org