Skip to content
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

feat: allow to ssh to podman virtual machine - api.d.ts changes #9382

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gastoner
Copy link
Contributor

@gastoner gastoner commented Oct 15, 2024

What does this PR do?

Provide the ability to ssh to podman machine - api.d.ts changes
1/4 PR's
-Needs to be merged first

Screenshot / video of UI

Screen.Recording.2024-10-15.145316.mp4

What issues does this PR fix or reference?

Part of #889

How to test this PR?

Create a podman machine, open terminal

  • Tests are covering the bug fix or the new feature

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should add some sample/typedoc

also I don't understand starConnection and stopConnection

setWindow should be maybe resize

packages/extension-api/src/extension-api.d.ts Outdated Show resolved Hide resolved
@gastoner
Copy link
Contributor Author

gastoner commented Oct 15, 2024

you should add some sample/typedoc

also I don't understand starConnection and stopConnection

setWindow should be maybe resize

The start/stop is used in stop ssh connection at first if there is any/ or to start the connection when are all callbacks set

The resize is used in container terminal and the ssh2 lib uses setWindow, so I wanted to stick with ssh2 lib theme

@gastoner gastoner force-pushed the provide_ability_to_easily_get_a_shell_in_a_machine_a branch from 34a60d5 to 0843f0f Compare October 15, 2024 14:10
@gastoner gastoner requested a review from benoitf October 15, 2024 14:11
@gastoner gastoner force-pushed the provide_ability_to_easily_get_a_shell_in_a_machine_a branch from 0843f0f to 8bea74e Compare October 15, 2024 14:35
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the API might be hard to understand if you still use ssh2 method as setWindow means nothing here, so please use resize as we're resizing

Then I'm not sure the on callback should be there if you follow that approach of having also "start and stop"

because the on callback and write are useless if you're not connected as they're tied to a connection/session

it means, from API POV, the events and method to interact with a session should only be part of the result of the start()

And from what I see, it's more to connect/disconnect than start and stop

or open and close

so maybe it's connect() method that gives you an object and on this object you have the on('data', 'error', 'end') events and the write method and the disconnect and the resize

also please remove any mention of 'ssh connection' as it's up to the implementation to see how it connects. API is not relying on ssh

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that write is with string parameter, maybe it could also be Uint8Array

@gastoner gastoner force-pushed the provide_ability_to_easily_get_a_shell_in_a_machine_a branch from 8bea74e to f4f58de Compare October 17, 2024 07:46
@gastoner gastoner requested a review from benoitf October 17, 2024 07:46
@gastoner gastoner force-pushed the provide_ability_to_easily_get_a_shell_in_a_machine_a branch from f4f58de to 7da4b99 Compare October 17, 2024 07:46
@gastoner gastoner marked this pull request as ready for review October 17, 2024 07:47
@gastoner gastoner requested a review from a team as a code owner October 17, 2024 07:47
@gastoner gastoner requested review from jeffmaury, cdrage and feloy and removed request for a team October 17, 2024 07:47
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might add samples with @example

packages/extension-api/src/extension-api.d.ts Outdated Show resolved Hide resolved
@gastoner gastoner force-pushed the provide_ability_to_easily_get_a_shell_in_a_machine_a branch from 7da4b99 to 916cde0 Compare October 17, 2024 08:44
@axel7083
Copy link
Contributor

How can I test as you are doing in the video, which branches should I combined with this PR ?

@gastoner
Copy link
Contributor Author

gastoner commented Oct 17, 2024

@axel7083 all changes are in https://github.com/gastoner/podman-desktop/tree/provide_ability_to_easily_get_a_shell_in_a_machine_test branch

Otherwise there is a provide_ability_to_easily_get_a_shell_in_a_machine + "a"/"b"/"c" branches (4 total)

Signed-off-by: Evzen Gasta <[email protected]>
@gastoner gastoner force-pushed the provide_ability_to_easily_get_a_shell_in_a_machine_a branch from 916cde0 to 359d02f Compare October 17, 2024 08:58
@gastoner gastoner requested a review from benoitf October 17, 2024 09:03
Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants