Kubevirt VmTemplateList #1673

yaacov · 2019-06-05T06:55:34Z

Introduces List Page for Virtual Machines.

Override template yaml:

yaacov · 2019-06-05T07:11:31Z

@mareklibra @vojtechszocs @rawagner original PR #1637 is closed ... continue here.

yaacov · 2019-06-11T11:38:06Z

@mareklibra @vojtechszocs @rawagner using page route seems to work :-) please review.

yaacov · 2019-06-11T12:18:24Z

Removed WIP and removed dev dependency for kubvirt plugin.

mareklibra · 2019-06-12T09:48:51Z

frontend/packages/kubevirt-plugin/src/components/vm-template.tsx

+export const VmTemplatesPageTitle = 'Virtual Machine Templates';
+
+const createItems = {
+  wizard: 'Create with Wizard',


Let's comment-out this item until wizard is implemented.
We can keep commented-out code here if there is imminent plan for a follow-up fixing that, but a PR should result in a production-ready change.

Removing 👍

mareklibra · 2019-06-12T09:49:54Z

frontend/packages/kubevirt-plugin/src/components/vm-template.tsx

+
+import {
+  getTemplateOperatingSystems,
+  DASHES,


Based on review of a separate PR, we should align on using of just single dash -.

Replaced 🙉

mareklibra · 2019-06-12T10:49:49Z

frontend/packages/kubevirt-plugin/src/plugin.ts

@@ -65,6 +79,26 @@ const plugin: Plugin<ConsumedExtensions> = [
      template: yamlTemplates.getIn([models.VirtualMachineModel, 'default']),


Shouldn't be yaml template customized as well?

Yes, I forgot it 😎 , added.

Note, when defining a template we override the TemplateModel yaml template, so if we want to support a different yaml for templates we will have to write a new page renderer for ~new.

Note, when defining a template we override the TemplateModel yaml template

YAML templates are keyed by model & template name (using default if not specified).

In this case, we should do something like:

{ type: 'YAMLTemplate', properties: { model: TemplateModel, template: 'VM template YAML goes here', templateName: 'vm-template', }, },

Nice, thanks !

mareklibra · 2019-06-12T10:52:18Z

frontend/packages/kubevirt-plugin/src/plugin.ts

+  {
+    type: 'Page/Route',
+    properties: {
+      path: `/k8s/ns/:ns/${models.VmTemplateModel.plural}`,


Is this needed? We have a resource list page and specialized model. I think generic router rule should work. Please correct me.

This is what I understood from #1658 (comment) , I may have misunderstood the discussion ...

Background
AFAIU VmTemplateModel is of kind Template so defining it will redefining the existing model Template, so ModelDefinition skips it. Once I added the template we actually get a console warning:

attempt to redefine model template.openshift.io~v1~Template

Yes, KubeVirt reuses the existing Template (template.openshift.io~v1~Template).

There's no need to redefine that model. What we need is a resource list page which applies a default filter to show VM templates only, e.g. via Table.staticFilters prop.

We have a resource list page and specialized model.

We don't have a specialized model for VM template, we reuse the existing template model.

Mapping between models vs. resource list/details pages is 1-1 so we need to use route-based page here.

yaacov · 2019-06-26T11:54:39Z

@suomiy thanks 👍 addressed the comments except the one about moving the yaml templates, It will impact orher resources and IMHO should be done separate from this PR.

p.s.
squashed commits because it became more then 20 commits ... :-)

atiratree · 2019-06-26T11:59:51Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template.tsx

+};
+VmTempleateTableHeader.displayName = 'VmTemplateTableHeader';
+
+const VmTemplateTableRow = ({ template, index, key, style }: VmTemplateTableRowProps) => {


please also here

atiratree · 2019-06-26T12:00:38Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template.tsx

+);
+
+type VmTemplateTableRowProps = {
+  template: K8sResourceKind;


TemplateKind should be more concrete

atiratree · 2019-06-26T12:01:18Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template.tsx

+  key: string;
+  style: any;
+};
+type VirtualMachineTemplatesProps = {};


having empty types is not much useful

yaacov · 2019-06-26T14:01:56Z

@suomiy thanks for the comments 👍 , broke some things in the way, now it's working again :-)

vojtechszocs · 2019-06-26T15:24:40Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template.tsx

+  Kebab.columnClass,
+];
+
+const VmTempleateTableHeader = () => {


Typo: VmTemplateTableHeader.

Thanks 😎

vojtechszocs · 2019-06-26T15:31:07Z

frontend/packages/kubevirt-plugin/src/plugin.ts

+  {
+    // NOTE(yaacov): vmtemplates is a template resource with a selector.
+    // 'NavItem/ResourceNS' is used, and not 'NavItem/Href', because it injects
+    // the namespace needed to get the correct link to a resource ( template with selector ) in our case.


In theory, NavItem/Href should only be used for URLs outside of Console 😄

So using NavItem/ResourceNS pointing to VM template resource is self-explanatory, I think.

This comment is here because of #1754 (comment)

May be an over kill 😄 I can remove it if you go not think someone may refactor it into an href in the future 🌷

vojtechszocs · 2019-06-26T15:33:01Z

frontend/packages/kubevirt-plugin/src/plugin.ts

+    type: 'Page/Route',
+    properties: {
+      exact: true,
+      path: [`/k8s/ns/:ns/vmtemplates`, '/k8s/all-namespaces/vmtemplates'],


Nit: no need for ` in static strings.

vojtechszocs · 2019-06-26T15:34:44Z

frontend/packages/kubevirt-plugin/src/yaml-templates.ts

+    metadata:
+      creationTimestamp: null
+      labels:
+        kubevirt-vm: 'vm-\${NAME}'


I'm probably missing something but where does NAME (and other uppercase-labeled variables) come from?

It's the template pramters, https://docs.okd.io/latest/dev_guide/templates.html#writing-parameters

default values defined here:
https://github.com/openshift/console/pull/1673/files#diff-a05f509d407deb7b65bb87cd43944154R106

@yaacov Thank you for clarification.

vojtechszocs · 2019-06-26T15:39:18Z

squashed commits because it became more then 20 commits ... :-)

When working with multiple un-merged PRs stacked onto each other, it's more practical & easier to review (in my opinion) when one PR is one commit. But that's just a workaround to the general problem of GitHub not supporting proper PR linking/association.

yaacov · 2019-06-27T06:25:13Z

@vojtechszocs Thanks ^^

yaacov · 2019-06-27T09:40:40Z

@vojtechszocs @suomiy please review

yaacov · 2019-06-27T10:52:58Z

@suomiy thanks ! this PR also need lgtm label 🚜

atiratree · 2019-06-27T11:06:10Z

/lgtm

openshift-ci-robot · 2019-06-27T11:06:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: suomiy, yaacov

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/kubevirt-plugin/OWNERS~~ [suomiy]

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

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2019

openshift-ci-robot requested review from jhadvig and zherman0 June 5, 2019 06:55

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2019

spadgett added this to the v4.2 milestone Jun 5, 2019

yaacov force-pushed the kubvirt-plugin-vmtemplate branch 2 times, most recently from 49902eb to b7d1a51 Compare June 11, 2019 11:23

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 7166cb8 to 3c6aa9b Compare June 11, 2019 11:36

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 3c6aa9b to f1beb77 Compare June 11, 2019 11:47

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2019

yaacov changed the title ~~[WIP] Kubvirt plugin vmtemplate~~ Kubvirt plugin vmtemplate Jun 11, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2019

mareklibra suggested changes Jun 12, 2019

View reviewed changes

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2019

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from f0c1735 to 08f00e0 Compare June 12, 2019 11:20

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 12, 2019

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 08f00e0 to 4b08e86 Compare June 12, 2019 11:21

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 26, 2019

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from daf5f4e to 1024120 Compare June 26, 2019 11:30

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 26, 2019

yaacov force-pushed the kubvirt-plugin-vmtemplate branch 3 times, most recently from cb986e3 to 1672a08 Compare June 26, 2019 11:51

atiratree reviewed Jun 26, 2019

View reviewed changes

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from e3f06b9 to f2f6812 Compare June 26, 2019 13:57

Use page route instead of resource for vmtemplates

bd336fe

yaacov force-pushed the kubvirt-plugin-vmtemplate branch from f2f6812 to bd336fe Compare June 26, 2019 13:59

vojtechszocs suggested changes Jun 26, 2019

View reviewed changes

yaacov added 2 commits June 27, 2019 09:13

use single quat for regular strings

85fdfce

typo

c4f4244

atiratree approved these changes Jun 27, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2019

openshift-ci-robot assigned atiratree Jun 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 27, 2019

openshift-merge-robot merged commit 78628d9 into openshift:master Jun 27, 2019

yaacov mentioned this pull request Jun 27, 2019

Move yaml tempalte to models dir #1842

Merged

		@@ -65,6 +79,26 @@ const plugin: Plugin<ConsumedExtensions> = [
		template: yamlTemplates.getIn([models.VirtualMachineModel, 'default']),

Kubevirt VmTemplateList #1673

Kubevirt VmTemplateList #1673

Conversation

yaacov commented Jun 5, 2019 • edited Loading

yaacov commented Jun 5, 2019

yaacov commented Jun 11, 2019

yaacov commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

vojtechszocs Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

yaacov commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented Jun 26, 2019 • edited Loading

yaacov commented Jun 27, 2019

yaacov commented Jun 27, 2019

yaacov commented Jun 27, 2019

atiratree commented Jun 27, 2019

openshift-ci-robot commented Jun 27, 2019

yaacov commented Jun 5, 2019 •

edited

Loading

vojtechszocs Jun 20, 2019 •

edited

Loading

vojtechszocs Jun 18, 2019 •

edited

Loading

vojtechszocs Jun 18, 2019 •

edited

Loading

vojtechszocs commented Jun 26, 2019 •

edited

Loading