Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Add DesktopViewer to VmConsoles #127

Merged
merged 9 commits into from
Jan 7, 2019
Merged

Conversation

mareklibra
Copy link
Contributor

@mareklibra mareklibra commented Nov 30, 2018

With this patch the DesktopViewer is added among other console types.

TODO:

  • have pf-react #989 merged, upgrade dependency version
  • maybe a follow-up: pass console details as an object instead of getter function
  • render only if either VM's VNC or RDP service is exposed (port, route)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 462

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 78.936%

Totals Coverage Status
Change from base Build 363: 0.01%
Covered Lines: 974
Relevant Lines: 1162

💛 - Coveralls

@coveralls
Copy link

coveralls commented Nov 30, 2018

Pull Request Test Coverage Report for Build 588

  • 21 of 23 (91.3%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 85.812%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/VmConsoles/VmConsoles.js 6 8 75.0%
Totals Coverage Status
Change from base Build 448: -0.2%
Covered Lines: 1342
Relevant Lines: 1489

💛 - Coveralls

The component renders user info for missing Service object for the RDP.
VM:
spec:
  template:
    metadata:
      labels:

are now automatically populated by:
  vm.cnv.io/name: ${VM_NAME}
@mareklibra mareklibra changed the title WIP: Add DesktopViewer to VmConsoles Add DesktopViewer to VmConsoles Jan 7, 2019
@mareklibra mareklibra requested a review from rawagner January 7, 2019 11:38
@@ -2,3 +2,7 @@
padding-bottom: 12px !important;
margin-top: -14px !important;
}

.vm-consoles-no-rdp .expand-collapse-pf-link-container {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use BEM with kubevirt prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

For better experience accessing Windows console, it is recommended to use the RDP. To do so, create a service:
<ul>
<li>
exposing the <b>3389/tcp</b> port of the virtual machine
Copy link
Contributor

Choose a reason for hiding this comment

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

You already have RDP constant in kubevirt/web-ui#129 . I propose to move it to web-ui-components and reuse it in this text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

exposing the <b>3389/tcp</b> port of the virtual machine
</li>
<li>
using selector: <b>vm.cnv.io/name: {vm.metadata.name}</b>
Copy link
Contributor

Choose a reason for hiding this comment

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

use constant TEMPLATE_VM_NAME_LABEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

</li>
</ul>
Make sure, the VM object has <b>spec.template.metadata.labels</b> set to{' '}
<b>vm.cnv.io/name: {vm.metadata.name}</b>
Copy link
Contributor

Choose a reason for hiding this comment

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

use constant TEMPLATE_VM_NAME_LABEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rawagner rawagner merged commit 71d2d4f into kubevirt:master Jan 7, 2019
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.

4 participants