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.

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

[WIP]Kubevirt VmTemplateDetails #1754

Closed
wants to merge 0 commits into from
Closed

[WIP]Kubevirt VmTemplateDetails #1754

wants to merge 0 commits into from

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Jun 19, 2019 •
edited
Loading

Introduces Details Page for Virtual Machine Templates.

Depends on:

Ref:
#1682

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 19, 2019
@yaacov yaacov changed the title Kubevirt.vm template details Kubevirt VmTemplateDetails Jun 19, 2019
@yaacov yaacov mentioned this pull request Jun 19, 2019
Merged
@yaacov yaacov changed the title Kubevirt VmTemplateDetails [WIP]Kubevirt VmTemplateDetails Jun 19, 2019
@openshift-ci-robot 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 19, 2019
@yaacov yaacov force-pushed the kubevirt.vmTemplateDetails branch from 41068a8 to 47c2cae Compare June 19, 2019 09:46
@yaacov
Copy link
Member Author

yaacov commented Jun 19, 2019

/retest

spadgett
spadgett requested changes Jun 19, 2019
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

This should be implemented as a special route instead of a resource page since we still need to handle templates that aren't VM templates.

cc @vojtechszocs

frontend/packages/kubevirt-plugin/src/models.ts Outdated
@@ -52,10 +53,9 @@ export const VirtualMachineInstanceReplicaSetModel: K8sKind = {
id: 'virtualmachineinstancereplicaset',
};

/* TODO(mlibra): migrate templates
export const VmTemplateModel: K8sKind = {
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't override the existing template model. It's already defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spadgett Thanks, this comment should be in:
https://github.com/openshift/console/pull/1673/files#diff-8066fd03cba594c7f8fb2be0890b95f9L55

This PR requires #1673, and this line is actually from the other PR.

I will adress this issue in #1673 👍

frontend/packages/kubevirt-plugin/src/plugin.ts Outdated
section: 'Workloads',
componentProps: {
name: 'Virtual Machine Templates',
resource: models.VmTemplateModel.plural,
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be a resource page since we have an existing resource page for templates (and not all templates are VM templates). This will need to be a special route.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, 👍 will address this in #1673 :-)

frontend/packages/kubevirt-plugin/src/yaml-templates.ts Outdated
);
)
.setIn(
[VmTemplateModel, 'default'],
Copy link
Member

Choose a reason for hiding this comment

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

This will override the default template YAML, which we don't want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I know ... see #1673 (comment) and #1673 (comment)

@spadgett Do we have other option to set specialized yaml if path is .../vmtemplates/~new ?

👍 also will address in #1673 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

This will override the default template YAML, which we don't want.

@spadgett @yaacov FYI, we already have an extension check to cover this:

packages/console-app/src/__tests__/extension-checks/yaml-templates.spec.ts

So even if a plugin tries to override an existing YAML template (modelRef & templateName composite key), yarn test still fails due to only one named template per model is allowed.

@spadgett spadgett added this to the v4.2 milestone Jun 19, 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 Jun 23, 2019
@yaacov yaacov force-pushed the kubevirt.vmTemplateDetails branch from 47c2cae to 852af8a Compare June 25, 2019 12:24
@openshift-ci-robot 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2019
@yaacov yaacov force-pushed the kubevirt.vmTemplateDetails branch 9 times, most recently from 8bdcc52 to 540be05 Compare June 26, 2019 11:02
@openshift-ci-robot 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 yaacov force-pushed the kubevirt.vmTemplateDetails branch from 540be05 to d3c3617 Compare June 26, 2019 11:58
@yaacov yaacov closed this Jun 27, 2019
@yaacov yaacov force-pushed the kubevirt.vmTemplateDetails branch from d3c3617 to 78628d9 Compare June 27, 2019 13:41
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2019
@openshift-ci-robot
Copy link
Contributor

[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

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 27, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 27, 2019 •
edited
Loading

@yaacov: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/backend 78628d9 link /test backend
ci/prow/frontend 78628d9 link /test frontend
ci/prow/e2e-aws-console-olm 78628d9 link /test e2e-aws-console-olm
ci/prow/e2e-aws 78628d9 link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett requested changes

@vojtechszocs vojtechszocs Awaiting requested review from vojtechszocs

@zherman0 zherman0 Awaiting requested review from zherman0

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 @vojtechszocs @spadgett