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

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page #5295

Merged
merged 6 commits into from May 6, 2020
Merged

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page #5295

merged 6 commits into from May 6, 2020

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented May 5, 2020 •
edited

After changing the Virtualization structure, when a user navigates to
k8s//virtualmachines
There is list of virtual machines, which displays only some VM attributes and does't allow all actions from the kebab menu.

This PR add redirects to virtualization page:

  • Changing of namespace while in details page redirect to virtualization correctly.
  • Link from overview dashbord redirect to virtualization correctly.
  • Breadcrumbs from template page redirect to virtualization templates correctly.

Screenshots:
Dashbord link:
link-from-dashboard

Template breadcrumbs:
template-breadcrumbs

Nmaespace change:
namespace

Ref:
https://bugzilla.redhat.com/show_bug.cgi?id=1829213
https://bugzilla.redhat.com/show_bug.cgi?id=1827688

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 5, 2020
@openshift-ci-robot
Copy link
Contributor

@yaacov: This pull request references Bugzilla bug 1827688, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page

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 component/kubevirt Related to kubevirt-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 5, 2020
@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

@suomiy @pcbailey @glekner @irosenzw please review

@openshift-ci-robot
Copy link
Contributor

@yaacov: This pull request references Bugzilla bug 1827688, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page

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.

2 similar comments
@openshift-ci-robot
Copy link
Contributor

@yaacov: This pull request references Bugzilla bug 1827688, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page

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
Copy link
Contributor

@yaacov: This pull request references Bugzilla bug 1827688, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page

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.

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/test e2e-gcp-console

2 similar comments
@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/test e2e-gcp-console

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/test e2e-gcp-console

atiratree
atiratree reviewed May 5, 2020
View reviewed changes
frontend/packages/kubevirt-plugin/src/plugin.tsx Outdated
'/k8s/ns/:ns/virtualmachines',
'/k8s/all-namespaces/virtualmachines',
'/k8s/ns/:ns/virtualmachineinstances',
'/k8s/all-namespaces/virtualmachineinstances',
Copy link
Member

Choose a reason for hiding this comment

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

IMO we shouldn't overwrite virtualmachineinstances path as it might be useful to have a view just for simple CRDs (eg you just want to drill to the yaml of the VMI once it comes up)

I don't see a big problem of having path for simple VirtualMachine CR too since we are not advertising this path anywhere (just the CRDs page), but I guess we could redirect to virtualization

can we get more opinions on this? @matthewcarleton @jelkosz

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to override the vanila vm and vmtemplate links because:
a - autogenerated urls use the object type to generate the link, link to vm and vmtemplate lists, if we do not redirect this links, we will send users to the wrong page.
b - the vm and vmtemplate list views do not give the user extra data.

vmi list view different, the list view of vmis show all vims while the virtualization list view show only vmis that are not owned by vms.

@matthewcarleton we need you help with the vmi list hiding issue:
a - is it ok to hide the vanila vmi list, without giving users alternative way to see a list of all vmis ?
b - can we add an option to the vm/vmi list to show only vms or only vmi's in a way that when showing only vmi's we will show all of them including those with vm owners ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to override the vanila vm and vmtemplate links because:
a - autogenerated urls use the object type to generate the link, link to vm and vmtemplate lists, if we do not redirect this links, we will send users to the wrong page.
b - the vm and vmtemplate list views do not give the user extra data.

vmi list view different, the list view of vmis show all vims while the virtualization list view show only vmis that are not owned by vms.

@matthewcarleton we need you help with the vmi list hiding issue:
a - is it ok to hide the vanila vmi list, without giving users alternative way to see a list of all vmis ?

b - can we add an option to the vm/vmi list to show only vms or only vmi's in a way that when showing only vmi's we will show all of them including those with vm owners ?

Both of these questions sound like filtering questions. I am reluctant to say we should allow the user to filter the list based on VMI/VM because we'd have to customize the filtering mechanism right? I know that OCP will be delivering a new toolbar which we could make use of then. Do we have use cases where we know this type of capability is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am reluctant to say we should allow the user to filter the list based on VMI/VM because we'd have to customize the filtering mechanism right?

it's already customized ... so no problem here :-)
we should open a BZ saying we want to filter by vm vs vmi ...

Do we have use cases where we know this type of capability is needed?

no, just guesses based on what we ( not so avarage users :-) ) think will be nice :-)

@atiratree
Copy link
Member

/lgtm

@atiratree
Copy link
Member

/lgtm cancel

@atiratree
Copy link
Member

atiratree commented May 5, 2020 •
edited

url parameters shoul be propagated for list sorting:

/k8s/ns/default/virtualmachines?orderBy=asc&sortBy=Namespace
-> /k8s/default/virtualization?orderBy=asc&sortBy=Namespace

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

@suomiy redirect now propagate params 🍰

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/retest

@atiratree
Copy link
Member

/retest
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/retest

@spadgett
Copy link
Member

spadgett commented May 5, 2020

/lgtm cancel

This change has build errors

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
@spadgett
Copy link
Member

spadgett commented May 5, 2020

Gentle reminder to look at job failures before retesting. Docker build strategy has failed usually indicates the code has a build error.

ERROR in /go/src/github.com/openshift/console/frontend/packages/kubevirt-plugin/src/plugin.tsx
(214,9): Type 'Promise<ComponentClass<Partial<RouteComponentProps<{}, StaticContext, any>>, any> | FunctionComponent<Partial<RouteComponentProps<{}, StaticContext, any>>> | FC<...>>' is not assignable to type 'Promise<ComponentType<Partial<RouteComponentProps<{}, StaticContext, any>>>>'.
  Type 'ComponentClass<Partial<RouteComponentProps<{}, StaticContext, any>>, any> | FunctionComponent<Partial<RouteComponentProps<{}, StaticContext, any>>> | FC<...>' is not assignable to type 'ComponentType<Partial<RouteComponentProps<{}, StaticContext, any>>>'.
    Type 'FC<VirtualizationPageProps>' is not assignable to type 'ComponentType<Partial<RouteComponentProps<{}, StaticContext, any>>>'.
      Type 'FunctionComponent<VirtualizationPageProps>' is not assignable to type 'FunctionComponent<Partial<RouteComponentProps<{}, StaticContext, any>>>'.
        The types of 'propTypes.location' are incompatible between these types.
          Type 'Validator<{ search?: string; }>' is not assignable to type 'Validator<Location<any>>'.
            Type '{ search?: string; }' is not assignable to type 'Location<any>'.

ERROR in /go/src/github.com/openshift/console/frontend/packages/kubevirt-plugin/src/plugin.tsx
(226,9): Type 'Promise<ComponentClass<Partial<RouteComponentProps<{}, StaticContext, any>>, any> | FunctionComponent<Partial<RouteComponentProps<{}, StaticContext, any>>> | FC<...>>' is not assignable to type 'Promise<ComponentType<Partial<RouteComponentProps<{}, StaticContext, any>>>>'.
Child html-webpack-plugin for "index.html":

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

@spadgett Thanks !! fixing

@yaacov
Copy link
Member Author

yaacov commented May 5, 2020

/test e2e-gcp-console

@yaacov yaacov force-pushed the redirect-to-virtualization branch from 4a91906 to 342a821 Compare May 5, 2020 16:15
@yaacov yaacov force-pushed the redirect-to-virtualization branch from 342a821 to 487003b Compare May 5, 2020 17:14
@yaacov
Copy link
Member Author

yaacov commented May 6, 2020

typing thingy fixed 🍰

@irosenzw
Copy link
Contributor

irosenzw commented May 6, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irosenzw, 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:

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

@openshift-merge-robot openshift-merge-robot merged commit e56c346 into openshift:master May 6, 2020
@openshift-ci-robot
Copy link
Contributor

@yaacov: All pull requests linked via external trackers have merged: openshift/console#5295. Bugzilla bug 1827688 has been moved to the MODIFIED state.

In response to this:

Bug 1827688: kubvirt redirect vm, vmi and vmtemplates to virtualization page

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.

@spadgett spadgett added this to the v4.5 milestone May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewcarleton matthewcarleton matthewcarleton left review comments

@atiratree atiratree atiratree left review comments

@jelkosz jelkosz Awaiting requested review from jelkosz

@mareklibra mareklibra Awaiting requested review from mareklibra

Assignees

@spadgett spadgett

@atiratree atiratree

@irosenzw irosenzw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@yaacov @openshift-ci-robot @atiratree @openshift-bot @spadgett @irosenzw @matthewcarleton @openshift-merge-robot