Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Vnc console #36

Merged
merged 3 commits into from
Oct 10, 2018
Merged

Vnc console #36

merged 3 commits into from
Oct 10, 2018

Conversation

mareklibra
Copy link

@mareklibra mareklibra commented Oct 8, 2018

In-browser access to VNC console of a VM is added.

Works fine but UXD needs to be reviewed, e.g. resizing of the console or its positioning in the UI in general.

At the time of posting this PR, it's accessible via Consoles tab under VirtualMachine page, no matter it's related to VirtualMachineInstance.

I did some cleaning of references as part of this PR too.


@mareklibra mareklibra changed the title WIP: Vnc console Vnc console Oct 9, 2018
@mareklibra
Copy link
Author

PR deployed on my upshift:

Please note, this is environment is used for development and can be broken any time in favor of other tasks. Please contact me in such case.

@mareklibra
Copy link
Author

@lizsurette, can you please have a look at this PR?

@mareklibra
Copy link
Author

vncconsole

@rawagner
Copy link

rawagner commented Oct 9, 2018

LGTM. Do we want to push yarn.lock too ?

@mareklibra
Copy link
Author

@rawagner , good point. The yarn.lock is removed now.
I will separate kubevirt and openshift npm dependencies in a follow-up.

@lizsurette
Copy link
Collaborator

lizsurette commented Oct 9, 2018

@mareklibra This is looking very nice! I think it works really well in the Consoles tab. A few questions for you...

1 - This is SUPER nit picky, but I don't think we need the thin line at the top of the console output area.

2 - Would it be possible to add a label on the left to let the user know which console they are viewing? I think this would be especially nice once we have a dropdown to access multiple different consoles (if available). I'll attach a screenshot of what I'm talking about :)

3 - How about allowing the user to disconnect? Also, see screenshot.

4 - Are there other options we want to expose from the Send Key menu?

screen shot 2018-10-09 at 10 15 28 am

Text "Console: VNC" is rendered on top of the VncCOnsole component.

Once design of console componenents is stabilized in pf-react, esp.
around the ConsolleSwitcher component, this change will be moved to pf-react.
@mareklibra
Copy link
Author

@lizsurette , thanks for the review.

1 - This is SUPER nit picky, but I don't think we need the thin line at the top of the console output area.

Done. The "line" is the border rendered by Toolbar.Results within VncConsole component in pf-react.
I placed it's override to web-ui but it will be moved to the pf-react once design around console components (esp. the ConsoleSelector) is stabilized.

2 - Would it be possible to add a label on the left to let the user know which console they are viewing? I think this would be especially nice once we have a dropdown to access multiple different consoles (if available). I'll attach a screenshot of what I'm talking about :)

Done. The screenshot suggests placing it "close" to the console component, means at the same row as the toolbar actions). Looks nice this way but the position is already reserved for "status" information (see screenshot bellow).
This can be easily resolved within the pf-react's VncConsole by rendering either status info or the console type. I would prefer doing that once the general console design in pf-react is stabilized.

For now, the console-type-text is rendered one row above, leaving space for potential status information.
@lizsurette , do you have any other idea? (please note, any potential change to pf-react will not make it to tomorrow's build).

3 - How about allowing the user to disconnect? Also, see screenshot.

I can add it for consistency, but technically it makes probably no sense.

This is console for the VNC protocol, not the Serial one. The connection is "hijacked" by a later one, when connected in parallel. The console is disconected when the view (i.e. Consoles tab) is changed.

In case of Serial console, this button would make sense - as another user can't "hijack" the connection easily.

Anyway, such a change should go to pf-react. So let's include either resolution to the design there, please.

4 - Are there other options we want to expose from the Send Key menu?

Not at this moment. I'm not aware of any kubevirt/openshift specifics in that matter, so I would add any useful shortcuts directly to pf-react's component directly. Unfortunately, the already identified ones will not make it to the build for tomorrow. Can we resolve that independently of this PR, please?

@mareklibra
Copy link
Author

01_connecting

02_connected

@rawagner rawagner merged commit 6342cfc into kubevirt:master Oct 10, 2018
@lizsurette
Copy link
Collaborator

@mareklibra I think this looks great for a first pass. Glad it was merged :) I will follow up with a small issue if I think of a good way to combine the status and console selection!

@docvirt
Copy link

docvirt commented Jun 4, 2019

Dear @mareklibra , Ask you a question. How to use novnc to connect vm created by kubevirt?
kubevirt/kubevirt#2339

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

Successfully merging this pull request may close these issues.

5 participants