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

Bug 1837677: open the VNC console in a new window #5548

Closed
wants to merge 1 commit into from

Conversation

glekner
Copy link
Contributor

@glekner glekner commented May 24, 2020

This enables opening the vnc console in a new window.

If the main window context is changed (e.g. move from console to details tab) then the new window will close aswell.

Screens:
Screen Shot 2020-05-24 at 18 12 57


Screen Shot 2020-05-24 at 18 13 20

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label May 24, 2020
@openshift-ci-robot
Copy link
Contributor

@glekner: This pull request references Bugzilla bug 1837677, which is invalid:

  • expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1837677: open the VNC console in a new window

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 24, 2020
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label May 24, 2020
@openshift-ci-robot openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label May 24, 2020
@@ -0,0 +1,56 @@
import * as React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rawagner can you review :)

@yaacov
Copy link
Member

yaacov commented May 24, 2020

@glekner Wow Great !

cc:// @jelkosz

@yaacov
Copy link
Member

yaacov commented May 25, 2020

@glekner great 🍰 I liked better the option of the new window with no buttons at all ( just the terminal ) can you make a screenshot of that option too ?

@yfrimanm
Copy link

yfrimanm commented May 25, 2020

This is great @glekner !

Please take a look at this design suggestion I have for rearranging the buttons
https://imgur.com/QDr2nMT
WDYT?

Thanks.

@glekner
Copy link
Contributor Author

glekner commented May 25, 2020

  • Applied @yfrimanm suggestion
  • If Window creation fails, an alert is shown @yaacov

Screen Shot 2020-05-25 at 12 33 44

() => {
externalWindow.current = window.open(
'',
'',
Copy link
Member

Choose a reason for hiding this comment

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

can you add the vmi name + "Console" as the window name ?

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: glekner, yaacov
To complete the pull request process, please assign jhadvig
You can assign the PR to them by writing /assign @jhadvig in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@atiratree
Copy link
Member

atiratree commented May 25, 2020

Looks pretty good, but it has few limitations which could be fixed.

If the main window context is changed (e.g. move from console to details tab) then the new window will close aswell.

IMO this is very counter-intuitive and we would get complaints about this soon.

cons

  • you need to have a host window and not use it in the future. Not until you close the terminal
  • you can't reference the terminal URL in case you need a quick look at the VM (more clicks have to be done)

We should have referenceable URL and proper standalone window for this to be user friendly.

Luckily something similar has already been done and is used in the console (che workspaces terminal):

they also have an option to open the terminal in standalone window at my.domain/terminal

this is how the init step looks (see the PR for other examples):

bb

can we get inspired by this and do something similar? And have our own url with the vm name like: /k8s/ns/default/virtualmachines/my-example-vm/standalone-console ?

@yaacov
Copy link
Member

yaacov commented May 26, 2020

Notes:

can we get inspired by this and do something similar? And have our own url with the vm name like: /k8s/ns/default/virtualmachines/my-example-vm/standalone-console ?

This is kubevirt backend endpoint for vnc [1]
/apis/subresources.kubevirt.io/v1alpha3/namespaces/NAMESPACE/virtualmachineinstances/VM/vnc
[1] https://kubevirt.io/2019/Access-Virtual-Machines-graphic-console-using-noVNC.html

@atiratree
Copy link
Member

This is kubevirt backend endpoint for vnc [1]
/apis/subresources.kubevirt.io/v1alpha3/namespaces/NAMESPACE/virtualmachineinstances/VM/vnc

yes we are using that internally - although our console is also for serial and rdp.

Do you want to encode the selected console into the url? Like /default/virtualmachines/my-vm/consoles?console=vnc

also, not sure we should be changing vm -> vmi endpoint, because the consoles page should be also accessible when the vmi is non existent

@glekner
Copy link
Contributor Author

glekner commented May 26, 2020

Do you want to encode the selected console into the url? Like /default/virtualmachines/my-vm/consoles?console=vnc

sounds like a good idea. do we need to handle permissions to those namespaces or they will already be dealt with?

@atiratree
Copy link
Member

sounds like a good idea. do we need to handle permissions to those namespaces or they will already be dealt with?

+1, we could have the same approach like the details page with StatusBox and show the appropriate error or Restricted Access message. Although we should offer redirect back to the console in that case.

@glekner
Copy link
Contributor Author

glekner commented May 27, 2020

Closing in favour of #5593

@glekner glekner closed this May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants