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.

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

kubevirt: move VmStatus from web-ui-components #2924

Merged
merged 4 commits into from Oct 21, 2019
Merged

kubevirt: move VmStatus from web-ui-components #2924

merged 4 commits into from Oct 21, 2019

Conversation

mareklibra
Copy link
Contributor

@mareklibra mareklibra commented Oct 7, 2019

The VmStatus component is migrated from the web-ui-components library to openshift/console and addapted to codestyle here and PF4.

Depends on:

@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin labels Oct 7, 2019
@mareklibra
Copy link
Contributor Author

mareklibra commented Oct 7, 2019

Design of the VmStatus before this change:

01_former designColours


02_former_detail

@mareklibra
Copy link
Contributor Author

Design introduced by this change:

03_new


04_new_pod

@mareklibra
Copy link
Contributor Author

@andybraren , wdyt about this change, please?

As the PF4 via pf-react icons are used here, the colours are gone (green for ok-state, red for the error-state). I can add them back, but please confirm this would be the right way.

Recently, even the icon is part of the link.

@andybraren
Copy link
Contributor

Correct, the icons should continue to be their normal green/red/yellow colors even when Status Popovers are available - only the text should turn blue. The icon being clickable sounds correct too.

@jtomasek would you be able to advise? I assume the StatusIconAndText component allows status icons to keep their normal colors, right? Or do they have to be specified manually again? (I'm not sure how the React side works here 🙂)

@mareklibra
Copy link
Contributor Author

mareklibra commented Oct 8, 2019

@andybraren , thanks, I will add the colors back.

The StatusIconAndText component is provided with an icon to render.
Such an icon needs to be "in-color" already. The color needs to be set manually via icon's 'color' property.

@mareklibra
Copy link
Contributor Author

@andybraren , wdyt?

statusIcons

</PopoverStatus>
);

const VmStatusInProgress: React.FC<VmStatusSpecificProps> = (props) => (
Copy link

Choose a reason for hiding this comment

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

There are already ProgressStatus, ErrorStatus and Status components which should be reused here. With those you wouldn't have to define the icons any more. Each of these components takes children prop which then becomes a content of the popover.

Feel free to add new PendingStatus component as that seems to be used quite frequently as well.

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

return labelValue ? ` (${labelValue})` : null;
};

const VmStatusPopover: React.FC<VmStatusPopoverProps> = ({
Copy link

Choose a reason for hiding this comment

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

This could be converted to VmStatusPopoverContent and be used as children for the status components (see below)

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

Comment on lines 127 to 135
<VmStatusError
title="Import error (VMware)"
message={IMPORTING_ERROR_VMWARE_MESSAGE}
linkMessage={VIEW_VM_EVENTS}
linkTo={linkToVMEvents}
>
{statusDetail.message}
{additionalText}
</VmStatusError>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<VmStatusError
title="Import error (VMware)"
message={IMPORTING_ERROR_VMWARE_MESSAGE}
linkMessage={VIEW_VM_EVENTS}
linkTo={linkToVMEvents}
>
{statusDetail.message}
{additionalText}
</VmStatusError>
<ErrorStatus title="Import error (VMware)">
  <VmStatusPopoverContent
  title="Import error (VMware)"
  message={IMPORTING_ERROR_VMWARE_MESSAGE}
  linkMessage={VIEW_VM_EVENTS}
  linkTo={linkToVMEvents}
  >
  {statusDetail.message}
  {additionalText}
  </VmStatusPopoverContent>
  </ErrorStatus>

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

@openshift-ci-robot openshift-ci-robot added component/shared Related to console-shared and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 8, 2019
};

const PendingStatus: React.FC<PendingStatusProps> = (props) => {
const icon = <HourglassHalfIcon />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a follow-up, I will refactor these components to reuse shared code.

@mareklibra mareklibra changed the title WIP kubevirt: move VmStatus from web-ui-components kubevirt: move VmStatus from web-ui-components Oct 8, 2019
@openshift-ci-robot 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 Oct 8, 2019
@openshift-ci-robot openshift-ci-robot added 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 Oct 8, 2019
@mareklibra
Copy link
Contributor Author

I moved this PR on top of #2930

@mareklibra
Copy link
Contributor Author

Rebased

@andybraren
Copy link
Contributor

Colors look correct, thank you @mareklibra! FYI @matthewcarleton

@mareklibra
Copy link
Contributor Author

/retest

@@ -7,23 +7,22 @@ import {
import {
global_warning_color_100 as warningColor,
global_danger_color_100 as dangerColor,
chart_color_green_400 as okColor,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a global_success_color_100 instead of chart_color_green_400? I'm not sure what the values are for these but using a chart variable for an icon could cause issues. There is a var(--pf-global--success-color--100) available but I'm not sure if these are 1-1. There is also var(--pf-global--success-color--200) which might be more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch Matt! var(--pf-global--success-color--200) looks best here, this should probably be fixed Console-wide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from #2930, I will fix there

@vojtechszocs
Copy link
Contributor

#2930 commit ef2953f isn't part of this PR's commits, please rebase.

@openshift-ci-robot openshift-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 14, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 14, 2019
@mareklibra
Copy link
Contributor Author

#2930 is merged now, so rebased here

@mareklibra
Copy link
Contributor Author

/retest

1 similar comment
@mareklibra
Copy link
Contributor Author

/retest

return labelValue ? ` (${labelValue})` : null;
};

const VmStatusPopoverContent: React.FC<VmStatusPopoverContentProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const VmStatusPopoverContent: React.FC<VmStatusPopoverContentProps> = ({
const VMStatusPopoverContent: React.FC<VMStatusPopoverContentProps> = ({

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

const getAdditionalImportText = (pod: PodKind): string => {
const labels = getLabels(pod, {});
const labelValue = labels[`${CDI_KUBEVIRT_IO}/${STORAGE_IMPORT_PVC_NAME}`];
return labelValue ? ` (${labelValue})` : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return labelValue ? ` (${labelValue})` : null;
return labelValue ? `(${labelValue})` : null;

The extra space can be handled in JSX as {' '} if necessary.

String concatenation shouldn't be a concern of getAdditionalImportText function, but rather a concern of code that uses it.

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

</>
);

export const VmStatus: React.FC<VmStatusProps> = ({ vm, pods, migrations, verbose = false }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const VmStatus: React.FC<VmStatusProps> = ({ vm, pods, migrations, verbose = false }) => {
export const VMStatus: React.FC<VMStatusProps> = ({ vm, pods, migrations, verbose = false }) => {

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

linkMessage={VIEW_VM_EVENTS}
linkTo={linkToVMEvents}
>
{statusDetail.message}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{statusDetail.message}
{statusDetail.message}{' '}

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

linkMessage={VIEW_VM_EVENTS}
linkTo={linkToVMEvents}
>
{statusDetail.message}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{statusDetail.message}
{statusDetail.message}{' '}

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


const getId = (value) => `${getNamespace(value)}-${getName(value)}`;

export const VmStatuses: React.FC<VmStatusesProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const VmStatuses: React.FC<VmStatusesProps> = (props) => {
export const VMStatuses: React.FC<VMStatusesProps> = (props) => {

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

return (
<>
{importerPods.map((pod) => (
<div key={getId(pod)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the <div> wrapper?

If not, you can simply declare the key prop on <VMStatus> below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The VMStatus can (rarely) render React.Fragment with text. So this div ensures the statuses to be shown one per line.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could use pod.metadata.uid instead

Suggested change
<div key={getId(pod)}>
<div key={pod.metadata.uid}>

Choose a reason for hiding this comment

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

Why? The reason for getId helper is to have a common way to access metadata.uid on a resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is using getId https://github.com/openshift/console/pull/2924/files#diff-c4803c82c2975654bafdd7fdc6c394a7R10 not getUID selector. Im proposing to use pod's uid as key and sure, we can use getUID selector for that

Choose a reason for hiding this comment

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

I see, thanks for clarifying @rawagner

))}
</>
);
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this (empty) default clause needed, given the code below?

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

The VmStatus component is migrated from the web-ui-components library
to openshift/console and addapted to codestyle here and PF4.
@mareklibra
Copy link
Contributor Author

/retest

1 similar comment
@mareklibra
Copy link
Contributor Author

/retest

Copy link
Contributor

@rawagner rawagner left a comment

Choose a reason for hiding this comment

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

just a two nits, not blocking

}
};

type VmStatusesProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type VmStatusesProps = {
type VMStatusesProps = {

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: #3058

return (
<>
{importerPods.map((pod) => (
<div key={getId(pod)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

we could use pod.metadata.uid instead

Suggested change
<div key={getId(pod)}>
<div key={pod.metadata.uid}>

@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jtomasek, mareklibra, rawagner

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

/retest

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

2 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants