added warnings and tests for hardware profiles #3657

rsun19 · 2025-01-17T21:45:41Z

Description

Invalid inputs will now show warnings in the UI

How Has This Been Tested?

You can directly insert hardware profiles in the cluster dashboard. Input invalid values as specified in the jira issue and mockups.

Test Impact

I added unit tests and cypress tests

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

openshift-ci · 2025-01-17T21:45:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

frontend/OWNERS

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

DaoDaoNoCode · 2025-01-20T19:20:30Z

/retest

codecov · 2025-01-20T19:57:26Z

Codecov Report

Attention: Patch coverage is 99.43503% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.23%. Comparing base (791a8b5) to head (4899f28).

Files with missing lines	Patch %	Lines
...wareProfiles/nodeResource/NodeResourceTableRow.tsx	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3657      +/-   ##
==========================================
+ Coverage   84.15%   84.23%   +0.08%     
==========================================
  Files        1453     1455       +2     
  Lines       33866    34020     +154     
  Branches     9384     9457      +73     
==========================================
+ Hits        28500    28658     +158     
+ Misses       5366     5362       -4

Files with missing lines	Coverage Δ
frontend/src/app/NavSidebar.tsx	`100.00% <100.00%> (ø)`
frontend/src/k8sTypes.ts	`100.00% <ø> (ø)`
...s/hardwareProfiles/HardwareProfileEnableToggle.tsx	`85.00% <100.00%> (+2.39%)`	⬆️
...nd/src/pages/hardwareProfiles/HardwareProfiles.tsx	`100.00% <100.00%> (ø)`
...c/pages/hardwareProfiles/HardwareProfilesTable.tsx	`100.00% <100.00%> (ø)`
...ages/hardwareProfiles/HardwareProfilesTableRow.tsx	`92.85% <100.00%> (+8.48%)`	⬆️
...dwareProfiles/manage/ManageNodeResourceSection.tsx	`97.56% <100.00%> (+0.33%)`	⬆️
...ardwareProfiles/nodeResource/NodeResourceTable.tsx	`100.00% <ø> (ø)`
...d/src/pages/hardwareProfiles/nodeResource/const.ts	`100.00% <100.00%> (ø)`
frontend/src/pages/hardwareProfiles/utils.ts	`100.00% <100.00%> (ø)`
... and 6 more

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 791a8b5...4899f28. Read the comment docs.

rsun19 · 2025-01-20T20:06:19Z

ahhh time to add more tests for the code coverage 🤣

frontend/src/__tests__/cypress/cypress/pages/hardwareProfile.ts

DaoDaoNoCode

Besides the comments I left, one important note is missing. You need to automatically disable the invalid hardware profiles and disable the toggle to prevent them from re-enable it before resolving the invalid values.

Other than that, it works well, good job! Most of my comments are about code quality because we want to make it reusable and easier to maintain in the future. :) Let me know if you have any questions with any of my comments and I am happy to help and walk you through it.

DaoDaoNoCode · 2025-01-20T20:18:45Z

frontend/src/pages/hardwareProfiles/HardwareProfilesTableRow.tsx

-            description={hardwareProfile.spec.description}
-            truncateDescriptionLines={2}
-          />
+          <Flex>


There is a layout issue with the invalid hardware profiles that have descriptions.

Checking the use of TableRowTitleDescription, I think you just need to do 3 things.

Remove the Flex layout here and put the whole Popover into the titleIcon of TableRowTitleDescription

Add resource={hardwareProfile} to TableRowTitleDescription (I had discussed this with Vince before but forgot to add it in my last PR, you can add it here, so it will show a QuestionIcon popover like other tables to help users find the k8s name of the hardware profile)

Add wrapResourceTitle={false} to TableRowTitleDescription to make sure the warning icon won't go to the next row.

After all the changes, your table title should look like this:

DaoDaoNoCode · 2025-01-20T20:28:31Z

frontend/src/utilities/NavData.tsx

  useAreaCheck<NavDataHref>(SupportedArea.HARDWARE_PROFILES, [
    {
      id: 'settings-hardware-profiles',
-      label: 'Hardware profiles',
+      label: isValid ? (


IsValid? The logic here works well but it looks like you are using an opposite naming for the variable. Maybe change it to isWarning? or pass!isWarning into this component and change the logic here.

frontend/src/utilities/NavData.tsx

DaoDaoNoCode · 2025-01-20T21:06:21Z

frontend/src/utilities/useWatchHardwareProfiles.ts

+import { groupVersionKind, HardwareProfileModel } from '~/api';
+import useK8sWatchResourceList from './useK8sWatchResourceList';
+
+export const useWatchHardwareProfiles = (


If you are planning to use websocket to watch the hardware profiles, maybe also use it when fetching the hardware profile table. Otherwise, when user edits something on the cluster to something invalid, there is an error message on the nav item but nothing on the table because they are using different ways to fetch the hardware profiles.

frontend/src/pages/hardwareProfiles/utils.ts

DaoDaoNoCode · 2025-01-20T21:31:08Z

frontend/src/pages/hardwareProfiles/utils.ts

+      ...profile,
+      spec: {
+        ...profile.spec,
+        warning: {


So as I said above, there is no need to use warningStatus, you can simply do

warning: parentWarningMessage ? { title: xxx(your conditions) message: parentWarningMessage } : undefined

DaoDaoNoCode · 2025-01-20T21:34:08Z

frontend/src/pages/hardwareProfiles/utils.ts

+  warningStatus: boolean;
+  title: string;
+  message: string;
+  isDefaultProfile: boolean;


This prop isDefaultProfile is also not very useful here. It's only used to decide whether to show a button to restore default or not when rendering the popover, you can just do isHardwareProfileOOTB(profile) there in the table row. No need to pass it around and make the type complicated. Keep it as simple as possible (P.S. isDefaultProfile should not be in warning, when structuring a type, we also need to consider the context and see whether we really need it or if it's reasonable enough to be here, just a suggestion)

DaoDaoNoCode · 2025-01-20T21:48:47Z

frontend/src/pages/hardwareProfiles/utils.ts

+  let isEnabled = false;
+  let warningCount = 0;
+  for (const profile of hardwareProfiles) {
+    if (profile.spec.warning?.warningStatus === true) {
+      warningCount += 1;
+    }
+    if (profile.spec.enabled === true) {
+      isEnabled = true;
+    }
+  }
+
+  return {
+    warning: warningCount > 0 || !isEnabled,
+    title: generateWarningTitle(isEnabled, warningCount === hardwareProfiles.length, warningCount),
+    message: generateWarningMessage(
+      isEnabled,
+      warningCount === hardwareProfiles.length,
+      warningCount,
+    ),
+    isEnabled,
+  };
+};


You can do all of these easily. There are some and every methods in array.
As for the isEnabled prop, you don't need to return it because you are not using it anywhere.

Suggested change

let isEnabled = false;

let warningCount = 0;

for (const profile of hardwareProfiles) {

if (profile.spec.warning?.warningStatus === true) {

warningCount += 1;

}

if (profile.spec.enabled === true) {

isEnabled = true;

}

}

return {

warning: warningCount > 0 || !isEnabled,

title: generateWarningTitle(isEnabled, warningCount === hardwareProfiles.length, warningCount),

message: generateWarningMessage(

isEnabled,

warningCount === hardwareProfiles.length,

warningCount,

),

isEnabled,

};

};

const hasInvalid = hardwareProfiles.some((profile) => profile.spec.warning);

const hasEnabled = hardwareProfiles.some((profile) => profile.spec.enabled);

const allInvalid = hardwareProfiles.every((profile) => profile.spec.warning);

return {

warning: hasInvalid || !hasEnabled,

title: generateWarningTitle(hasEnabled, allInvalid),

message: generateWarningMessage(hasEnabled, allInvalid),

};

DaoDaoNoCode · 2025-01-20T21:50:00Z

frontend/src/pages/hardwareProfiles/utils.ts

+export const generateWarningMessage = (
+  isEnabled: boolean,
+  isAllWarnings: boolean,
+  warningCount: number,


You don't need warningCount here. There is no way warningCount could be 0 in this situation when showing the alert.

DaoDaoNoCode · 2025-01-20T21:50:43Z

frontend/src/pages/hardwareProfiles/utils.ts

+export const generateWarningTitle = (
+  isEnabled: boolean,
+  isAllWarnings: boolean,
+  warningCount: number,


Same here, no need to add warningCount here.
If the first 2 conditions are not met, directly return the third error title.

DaoDaoNoCode · 2025-01-20T22:00:56Z

I haven't finished testing all the scenarios yet. I will do another round of review after you address the comments I left above and test it overall again. 😄

rsun19 · 2025-01-23T17:09:07Z

@DaoDaoNoCode made the requested changes. However, for some methods, I had to fall back to identifier.displayName because for some reason resourceType is not allowed in the YAML for hardware profiles. Have you seen this before? Simply referring to resourceType worked a few days ago but now it disappears when trying to parse the hardware profiles

Edit: May be related to this: https://issues.redhat.com/browse/RHOAIENG-18118

pnaik1 · 2025-01-24T06:46:22Z

I had to fall back to identifier.displayName because for some reason resourceType is not allowed in the YAML for hardware profiles. Have you seen this before? Simply referring to resourceType worked a few days ago but now it disappears when trying to parse the hardware profiles

@rsun19 , Here's a possibility that you might be using old hardwareProfile CRD, Juntao added new field resourceType to CRD few days back, can you try applying the latest CRD?
let me know if you need any help here

DaoDaoNoCode

LGTM most of the parts. There are still 2 major things.

The use of the types - We should try to avoid redundant types and make best use of the available/existing types, so it could be easier to maintain/add features in the future.
For "Incomplete" hardware profiles (Hardware profiles that miss CPU/Memory), we should not block them from enabling because that's still valid, just not recommended. If you want we can have a talk when you are online, so we can walk through all the comments together to make it work as expected.

frontend/src/__mocks__/mockHardwareProfile.ts

frontend/src/pages/hardwareProfiles/HardwareProfileEnableToggle.tsx

frontend/src/pages/hardwareProfiles/manage/ManageNodeResourceSection.tsx

frontend/src/pages/hardwareProfiles/utils.ts

frontend/src/utilities/valueUnits.ts

frontend/src/pages/hardwareProfiles/HardwareProfiles.tsx

frontend/src/pages/hardwareProfiles/utils.ts

DaoDaoNoCode · 2025-01-24T07:46:10Z

frontend/src/pages/hardwareProfiles/utils.ts

+        enabled: profile.spec.enabled && !warningStatus,
+        warning: parentWarningMessage
+          ? {
+              title:
+                warningStatus === true
+                  ? `${!hasCPUandMemory(identifiers) ? 'Incomplete' : 'Invalid'} ${
+                      isDefaultProfile === true ? 'default hardware' : 'hardware'
+                    } profile`
+                  : '',
+              message: warningStatus === true ? parentWarningMessage : '',
+            }
+          : undefined,


Suggested change

enabled: profile.spec.enabled && !warningStatus,

warning: parentWarningMessage

? {

title:

warningStatus === true

? `${!hasCPUandMemory(identifiers) ? 'Incomplete' : 'Invalid'} ${

isDefaultProfile === true ? 'default hardware' : 'hardware'

} profile`

: '',

message: warningStatus === true ? parentWarningMessage : '',

}

: undefined,

enabled: profile.spec.enabled && !parentWarningMessage,

warning: parentWarningMessage

? {

title: `${!hasCPUandMemory(identifiers) ? 'Incomplete' : 'Invalid'} ${

isDefaultProfile === true ? 'default hardware' : 'hardware'

} profile`,

message: parentWarningMessage,

}

: undefined,

DaoDaoNoCode · 2025-01-24T07:55:35Z

frontend/src/pages/hardwareProfiles/utils.ts

+      for (let identifier of identifiers) {
+        if (
+          identifier.resourceType === IdentifierResourceType.CPU ||
+          identifier.displayName === IdentifierResourceType.CPU ||
+          identifier.resourceType === IdentifierResourceType.MEMORY ||
+          identifier.displayName === IdentifierResourceType.MEMORY
+        ) {
+          identifier = {
+            ...identifier,
+            warning: true,
+          };
+        }
+      }


I didn't get what this is trying to do... Can you explain it to me? What I am reading is: "If this is an OOTB hardware profile and it lacks CPU/Memory resource, add a warning icon to all of its CPU and Memory resource row" - Which doesn't make sense to me. There is no need to add the warning to any resource row in this situation because it's just lacking CPU and/or Memory for the whole table, and needs to be added.

Ah ok that makes sense

rsun19 · 2025-01-24T22:09:23Z

@DaoDaoNoCode Made requested changes. I have a few points though:

I have not tested yet with the resourceType added into the CRD, but I assume it would work the same because of the fallback. If we could sync on Monday to get that sorted out, that would be great.
Do you think there should be warning symbols on the sidebar and a banner on the top if there is a missing CPU/Memory in the default profile? I think we are missing guidance on this. If we do, we might need to figure out the warning message in that case. Currently, I just have the warning symbol for the row when this case is hit.

DaoDaoNoCode · 2025-01-27T02:36:39Z

@rsun19 Hi, thanks for the updates. I am not sure what's the best way to show the warning icon in this situation, let's check with @vconzola today and sort it out together.

rsun19 · 2025-01-27T20:08:43Z

Made further changes.

Juntao and I talked with Vince, and we clarified 2 points:

The warning icon on the Navbar should only be shown when all hardware profiles are invalid
We should show warnings and messages for hardware profiles that are missing CPU/Memory

openshift-ci bot requested review from mturley and ppadti January 17, 2025 21:45

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from a244f6b to 6f9c516 Compare January 17, 2025 21:56

rsun19 changed the title ~~added warnings and tests for hardware profiles~~ [WIP] added warnings and tests for hardware profiles Jan 20, 2025

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jan 20, 2025

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 6f9c516 to ab511bc Compare January 20, 2025 16:13

rsun19 changed the title ~~[WIP] added warnings and tests for hardware profiles~~ added warnings and tests for hardware profiles Jan 20, 2025

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jan 20, 2025

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from ab511bc to 9125256 Compare January 20, 2025 16:16

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 9125256 to 723e629 Compare January 20, 2025 19:38

bobbravo2 reviewed Jan 20, 2025

View reviewed changes

frontend/src/__tests__/cypress/cypress/pages/hardwareProfile.ts Outdated Show resolved Hide resolved

DaoDaoNoCode requested changes Jan 20, 2025

View reviewed changes

openshift-ci bot assigned DaoDaoNoCode Jan 20, 2025

rsun19 force-pushed the hardware-profile-edge-case-alerts branch 9 times, most recently from e934b0d to 2ebd5a8 Compare January 23, 2025 17:03

DaoDaoNoCode reviewed Jan 24, 2025

View reviewed changes

rsun19 force-pushed the hardware-profile-edge-case-alerts branch 2 times, most recently from 46b5431 to 77982f6 Compare January 24, 2025 22:04

rsun19 force-pushed the hardware-profile-edge-case-alerts branch 3 times, most recently from fa0ed7d to 940b5dd Compare January 27, 2025 20:04

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 940b5dd to 63543c4 Compare January 27, 2025 21:39

added warnings and tests for hardware profiles

4899f28

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 63543c4 to 4899f28 Compare January 30, 2025 17:37

rsun19 changed the title ~~added warnings and tests for hardware profiles~~ [wip] added warnings and tests for hardware profiles Jan 30, 2025

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jan 30, 2025

rsun19 changed the title ~~[wip] added warnings and tests for hardware profiles~~ added warnings and tests for hardware profiles Jan 30, 2025

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added warnings and tests for hardware profiles #3657

added warnings and tests for hardware profiles #3657

rsun19 commented Jan 17, 2025 •

edited

Loading

openshift-ci bot commented Jan 17, 2025

DaoDaoNoCode commented Jan 20, 2025

codecov bot commented Jan 20, 2025 •

edited

Loading

rsun19 commented Jan 20, 2025

DaoDaoNoCode left a comment

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode Jan 20, 2025

DaoDaoNoCode commented Jan 20, 2025 •

edited

Loading

rsun19 commented Jan 23, 2025 •

edited

Loading

pnaik1 commented Jan 24, 2025 •

edited

Loading

DaoDaoNoCode left a comment

DaoDaoNoCode Jan 24, 2025

DaoDaoNoCode Jan 24, 2025

rsun19 Jan 24, 2025

rsun19 commented Jan 24, 2025

DaoDaoNoCode commented Jan 27, 2025

rsun19 commented Jan 27, 2025

added warnings and tests for hardware profiles #3657

Are you sure you want to change the base?

added warnings and tests for hardware profiles #3657

Conversation

rsun19 commented Jan 17, 2025 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Jan 17, 2025

DaoDaoNoCode commented Jan 20, 2025

codecov bot commented Jan 20, 2025 • edited Loading

Codecov Report

rsun19 commented Jan 20, 2025

DaoDaoNoCode left a 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

Choose a reason for hiding this comment

DaoDaoNoCode commented Jan 20, 2025 • edited Loading

rsun19 commented Jan 23, 2025 • edited Loading

pnaik1 commented Jan 24, 2025 • edited Loading

DaoDaoNoCode left a 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

rsun19 commented Jan 24, 2025

DaoDaoNoCode commented Jan 27, 2025

rsun19 commented Jan 27, 2025

rsun19 commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 20, 2025 •

edited

Loading

DaoDaoNoCode commented Jan 20, 2025 •

edited

Loading

rsun19 commented Jan 23, 2025 •

edited

Loading

pnaik1 commented Jan 24, 2025 •

edited

Loading