Skip to content

[WIP] Kubvirt plugin vmtemplate #1637

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.

Sign up for GitHub

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

Jump to bottom
Closed
yaacov wants to merge 0 commits into from
Closed

[WIP] Kubvirt plugin vmtemplate #1637

yaacov wants to merge 0 commits into from

Conversation

@yaacov
Copy link
Member

@yaacov yaacov commented May 30, 2019 •
edited
Loading

Integrate template list page

  • Add a menu item for vm templates.
  • Import vm template list page.
  • Add tests

Depends on:

Screenshot:
Peek 2019-06-02 14-38

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 30, 2019
@yaacov
Copy link
Member Author

yaacov commented May 30, 2019

cc// @mareklibra

@yaacov yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 5bb73e9 to 16e9d11 Compare May 30, 2019 14:12
@spadgett spadgett added this to the v4.2 milestone May 30, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2019
@yaacov yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 16e9d11 to 9f11bc5 Compare June 2, 2019 08:18
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 2, 2019
@yaacov yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 9f11bc5 to 04b3db4 Compare June 2, 2019 08:22
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 2, 2019
@yaacov yaacov force-pushed the kubvirt-plugin-vmtemplate branch 5 times, most recently from 8a07eae to 8f2c0d8 Compare June 2, 2019 11:40
yaacov
yaacov commented Jun 2, 2019
View reviewed changes
frontend/packages/kubevirt-plugin/src/models.ts Outdated
Copy link
Member Author

@yaacov yaacov Jun 2, 2019

Choose a reason for hiding this comment

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

This should be:
plural: 'vmtemplates' then the menu item link is /vmtemplates but the page it'self stay /tempates and we get a 404 page :-(

@mareklibra what am I missing ?

Copy link
Contributor

@mareklibra mareklibra Jun 3, 2019

Choose a reason for hiding this comment

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

We do not have specific CRD for a VM template, regular OpenShift templates are reused, just with template.kubevirt.io/type label is set (afaik either base or vm values recently).

Proper plural here is vmtemplates.
Per comment in #1592, I will remove TemplateModel from here in favour of core console-app models. But the VmTemplateModel should remain here, we differentiate via id/plural/selector.

Copy link
Contributor

@mareklibra mareklibra Jun 3, 2019

Choose a reason for hiding this comment

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

Regarding the 404, the VmTemplateModel is filtered out in k8s-models.ts on an attempt to redefine Template model.

@vojtechszocs , do you have any idea how to ease this check to enable specialization of a model?

If we stay strict on disabling redefinition, we might introduce explicit extension for Router so we will not land on the VmTemplatesList page via generic /k8s/ns/:ns/:plural but a specific one will be provided.

mareklibra
mareklibra reviewed Jun 3, 2019
View reviewed changes
frontend/packages/kubevirt-plugin/src/plugin.ts Outdated
Copy link
Contributor

@mareklibra mareklibra Jun 3, 2019

Choose a reason for hiding this comment

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

We do not have a CRD for VmTemplates, I think this should be removed.

frontend/packages/kubevirt-plugin/src/models.ts Outdated
Copy link
Contributor

@mareklibra mareklibra Jun 3, 2019

Choose a reason for hiding this comment

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

We do not have specific CRD for a VM template, regular OpenShift templates are reused, just with template.kubevirt.io/type label is set (afaik either base or vm values recently).

Proper plural here is vmtemplates.
Per comment in #1592, I will remove TemplateModel from here in favour of core console-app models. But the VmTemplateModel should remain here, we differentiate via id/plural/selector.

frontend/packages/kubevirt-plugin/src/models.ts Outdated
Copy link
Contributor

@mareklibra mareklibra Jun 3, 2019

Choose a reason for hiding this comment

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

Regarding the 404, the VmTemplateModel is filtered out in k8s-models.ts on an attempt to redefine Template model.

@vojtechszocs , do you have any idea how to ease this check to enable specialization of a model?

If we stay strict on disabling redefinition, we might introduce explicit extension for Router so we will not land on the VmTemplatesList page via generic /k8s/ns/:ns/:plural but a specific one will be provided.

@yaacov yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 7b1714c to 6234d24 Compare June 5, 2019 06:46
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 5, 2019
@yaacov yaacov closed this Jun 5, 2019
@yaacov yaacov force-pushed the kubvirt-plugin-vmtemplate branch from 6234d24 to cdd250d Compare June 5, 2019 06:48
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 5, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yaacov

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2019
@yaacov yaacov mentioned this pull request Jun 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jhadvig jhadvig Awaiting requested review from jhadvig

@rhamilto rhamilto Awaiting requested review from rhamilto

1 more reviewer

@mareklibra mareklibra mareklibra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

v4.2

Development

Successfully merging this pull request may close these issues.

4 participants

@yaacov @openshift-ci-robot @mareklibra @spadgett