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

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

/retest

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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.36%. Comparing base (791a8b5) to head (86494e3).
Report is 47 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3657      +/-   ##
==========================================
+ Coverage   84.15%   84.36%   +0.21%     
==========================================
  Files        1453     1472      +19     
  Lines       33866    34004     +138     
  Branches     9384     9474      +90     
==========================================
+ Hits        28500    28688     +188     
+ Misses       5366     5316      -50

Files with missing lines	Coverage Δ
frontend/src/app/NavSidebar.tsx	`100.00% <100.00%> (ø)`
...s/hardwareProfiles/HardwareProfileEnableToggle.tsx	`86.95% <100.00%> (+4.34%)`	⬆️
...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% <ø> (ø)`
...wareProfiles/nodeResource/NodeResourceTableRow.tsx	`100.00% <100.00%> (ø)`
...d/src/pages/hardwareProfiles/nodeResource/const.ts	`100.00% <100.00%> (ø)`
frontend/src/pages/hardwareProfiles/utils.ts	`100.00% <100.00%> (ø)`
... and 5 more

... and 102 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...86494e3. 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.

frontend/src/pages/hardwareProfiles/HardwareProfilesTableRow.tsx

frontend/src/utilities/NavData.tsx

frontend/src/utilities/useWatchHardwareProfiles.ts

frontend/src/pages/hardwareProfiles/utils.ts

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

Gkrumbach07

/lgtm

openshift-ci · 2025-02-07T17:56:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07

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:

~~frontend/OWNERS~~ [Gkrumbach07]

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

Gkrumbach07 · 2025-02-07T18:25:53Z

/hold

frontend/src/__mocks__/mockHardwareProfile.ts

frontend/src/k8sTypes.ts

frontend/src/pages/hardwareProfiles/utils.ts

openshift-ci · 2025-02-10T21:21:47Z

New changes are detected. LGTM label has been removed.

rsun19 · 2025-02-11T14:19:00Z

/retest

Gkrumbach07 · 2025-02-13T13:33:40Z

frontend/src/types.ts

@@ -683,3 +683,8 @@ export type IntegrationAppStatus = {
  variablesValidationTimestamp?: string;
  error: string;
 };
+
+export type WarningNotification = {


keep this type within the scope of hardware profiles. so move it under one of those folders in a types.ts folder

Gkrumbach07 · 2025-02-13T13:36:39Z

frontend/src/pages/hardwareProfiles/HardwareProfileEnableToggle.tsx

+    hardwareProfile.spec.enabled &&
+    (typeof hardwareProfileWarningMessage === 'undefined' ||
+      hardwareProfileWarningMessage.message.includes(HARDWARE_PROFILES_MISSING_CPU_MEMORY_MESSAGE));


i would keep is enabled separate from the warning. Just use warning to additionally disable the hardware profile

Gkrumbach07 · 2025-02-13T13:42:54Z

frontend/src/pages/hardwareProfiles/utils.ts

+  return HardwareProfileBannerWarningTitles.SOME_INCOMPLETE;
+};
+
+export const generateWarningMessage = (


doesnt need to be exported if not used outside this file

Gkrumbach07 · 2025-02-13T13:43:29Z

frontend/src/pages/hardwareProfiles/utils.ts


 export const isHardwareProfileOOTB = (hardwareProfile: HardwareProfileKind): boolean =>
  hardwareProfile.metadata.labels?.['opendatahub.io/ootb'] === 'true';
+
+export const generateWarningTitle = (


doesnt need to be exported unless used outside this file

Gkrumbach07 · 2025-02-13T13:54:24Z

frontend/src/pages/hardwareProfiles/utils.ts

+export const hardwareProfileWarning = (
+  hardwareProfile: HardwareProfileKind,
+): WarningNotification | undefined => {
+  const identifiers = hardwareProfile.spec.identifiers ?? [];
+  let parentWarningMessage = '';
+  const isDefaultProfile = isHardwareProfileOOTB(hardwareProfile);
+  if (!hasCPUandMemory(identifiers)) {
+    parentWarningMessage = HARDWARE_PROFILES_MISSING_CPU_MEMORY_MESSAGE;
+  }
+  for (const identifier of identifiers) {
+    try {
+      if (identifier.minCount.toString().at(0) === '-') {
+        parentWarningMessage = `Minimum allowed ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot be negative.`;
+        continue;
+      }
+      if (identifier.maxCount.toString().at(0) === '-') {
+        parentWarningMessage = `Maximum allowed ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot be negative.`;
+        continue;
+      }
+      if (identifier.defaultCount.toString().at(0) === '-') {
+        parentWarningMessage = `Default count for ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot be negative.`;
+        continue;
+      }
+      const [minCount] = splitValueUnit(
+        identifier.minCount.toString(),
+        determineUnit(identifier),
+        true,
+      );
+      const [maxCount] = splitValueUnit(
+        identifier.maxCount.toString(),
+        determineUnit(identifier),
+        true,
+      );
+      const [defaultCount] = splitValueUnit(
+        identifier.defaultCount.toString(),
+        determineUnit(identifier),
+        true,
+      );
+      if (!Number.isInteger(minCount)) {
+        parentWarningMessage = `Minimum count for ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot be a decimal.`;
+      }
+      if (!Number.isInteger(maxCount)) {
+        parentWarningMessage = `Maximum count for ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot be a decimal.`;
+      }
+      if (!Number.isInteger(defaultCount)) {
+        parentWarningMessage = `Default count for ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot be a decimal.`;
+      }
+      if (minCount > maxCount) {
+        parentWarningMessage = `Minimum allowed ${
+          identifier.resourceType ?? identifier.displayName
+        } cannot exceed maximum allowed ${identifier.resourceType ?? identifier.displayName}.`;
+      } else if (defaultCount < minCount || defaultCount > maxCount) {
+        parentWarningMessage = `The default count for ${
+          identifier.resourceType ?? identifier.displayName
+        } must be between the minimum allowed ${
+          identifier.resourceType ?? identifier.displayName
+        } and maximum allowed ${identifier.resourceType ?? identifier.displayName}.`;
+      }
+    } catch (e) {
+      parentWarningMessage = `The resource count for ${
+        identifier.resourceType ?? identifier.displayName
+      } has an invalid unit.`;
+    }
+  }
+
+  return parentWarningMessage
+    ? {
+        title: `${!hasCPUandMemory(identifiers) ? 'Incomplete' : 'Invalid'} ${
+          isDefaultProfile === true ? 'default hardware' : 'hardware'
+        } profile`,
+        message: `${parentWarningMessage} ${
+          isDefaultProfile
+            ? HARDWARE_PROFILES_DEFAULT_WARNING_MESSAGE
+            : 'Edit the profile to make the profile valid.'
+        }`,
+      }
+    : undefined;
+};


this function should basically validate the hardware profile. It looks like you are returning only one error becuase parentWarningMessage keeps getting overwritten.

I think this function should return an array of all the warnings with the profile. so...

enum HardwareProfileWarningType { HARDWARE_PROFILES_DEFAULT_WARNING, HARDWARE_PROFILES_MISSING_CPU_MEMORY, OTHER } type HardwareProfileWarning = { title: string, message: string type: HardwareProfileWarningType } export const validateProfileWarning = ( hardwareProfile: HardwareProfileKind): HardwareProfileWarning[] => { // same func body but instead add to this array and return that }

Gkrumbach07 · 2025-02-13T14:07:46Z

frontend/src/pages/hardwareProfiles/utils.ts

+export const generateWarningForHardwareProfiles = (
+  hardwareProfiles: HardwareProfileKind[],
+): WarningNotification | undefined => {
+  const hasInvalid = hardwareProfiles.some((profile) => {
+    const warning = hardwareProfileWarning(profile);
+    return (
+      typeof warning !== 'undefined' &&
+      !warning.message.includes(HARDWARE_PROFILES_MISSING_CPU_MEMORY_MESSAGE)
+    );
+  });
+  const hasEnabled = hardwareProfiles.some((profile) => profile.spec.enabled);
+  const allInvalid = hardwareProfiles.every((profile) => {
+    const warning = hardwareProfileWarning(profile);
+    return (
+      typeof warning !== 'undefined' &&
+      !warning.message.includes(HARDWARE_PROFILES_MISSING_CPU_MEMORY_MESSAGE)
+    );
+  });
+  const allIncomplete = hardwareProfiles.every((profile) => {
+    const warning = hardwareProfileWarning(profile);
+    return (
+      typeof warning !== 'undefined' &&
+      warning.message.includes(HARDWARE_PROFILES_MISSING_CPU_MEMORY_MESSAGE)
+    );
+  });
+  const hasInvalidIncludingIncompleteProfiles = hardwareProfiles.some(
+    (profile) => typeof hardwareProfileWarning(profile) !== 'undefined',
+  );
+  if (hardwareProfiles.length === 0 || (!hasInvalidIncludingIncompleteProfiles && hasEnabled)) {
+    return undefined;
+  }
+  return {
+    title: generateWarningTitle(hasEnabled, allInvalid, hasInvalid, allIncomplete),
+    message: generateWarningMessage(hasEnabled, allInvalid, hasInvalid, allIncomplete),
+  };
+};


Now this function after updating the other func (hardwareProfileWarning) you can instead look for the specific warnings like

const warnings = hardwareProfileWarning(profile); const invalid === warnings.some(warning => warning.type === HARDWARE_PROFILES_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 from 2ebd5a8 to 46b5431 Compare January 24, 2025 22:01

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

Gkrumbach07 approved these changes Feb 7, 2025

View reviewed changes

openshift-ci bot assigned Gkrumbach07 Feb 7, 2025

openshift-ci bot added the lgtm label Feb 7, 2025

openshift-ci bot added the approved label Feb 7, 2025

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Feb 7, 2025

andrewballantyne reviewed Feb 7, 2025

View reviewed changes

frontend/src/__mocks__/mockHardwareProfile.ts Outdated Show resolved Hide resolved

Gkrumbach07 reviewed Feb 7, 2025

View reviewed changes

frontend/src/k8sTypes.ts Outdated Show resolved Hide resolved

Gkrumbach07 reviewed Feb 7, 2025

View reviewed changes

frontend/src/pages/hardwareProfiles/utils.ts Outdated Show resolved Hide resolved

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 4899f28 to 27a4312 Compare February 10, 2025 21:21

openshift-ci bot removed the lgtm label Feb 10, 2025

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 27a4312 to 5eed089 Compare February 10, 2025 23:12

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

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

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

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

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 5eed089 to 3e8d3d3 Compare February 11, 2025 14:20

added warnings and tests for hardware profiles

86494e3

rsun19 force-pushed the hardware-profile-edge-case-alerts branch from 3e8d3d3 to 86494e3 Compare February 11, 2025 14:57

Gkrumbach07 reviewed Feb 13, 2025

View reviewed changes

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

DaoDaoNoCode commented Jan 20, 2025

codecov bot commented Jan 20, 2025 •

edited

Loading

rsun19 commented Jan 20, 2025

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

Gkrumbach07 left a comment

openshift-ci bot commented Feb 7, 2025

Gkrumbach07 commented Feb 7, 2025

openshift-ci bot commented Feb 10, 2025

rsun19 commented Feb 11, 2025

Gkrumbach07 Feb 13, 2025

Gkrumbach07 Feb 13, 2025

Gkrumbach07 Feb 13, 2025

Gkrumbach07 Feb 13, 2025

Gkrumbach07 Feb 13, 2025

Gkrumbach07 Feb 13, 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:

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

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

Gkrumbach07 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 7, 2025

Gkrumbach07 commented Feb 7, 2025

openshift-ci bot commented Feb 10, 2025

rsun19 commented Feb 11, 2025

Gkrumbach07 Feb 13, 2025

Choose a reason for hiding this comment

Gkrumbach07 Feb 13, 2025

Choose a reason for hiding this comment

Gkrumbach07 Feb 13, 2025

Choose a reason for hiding this comment

Gkrumbach07 Feb 13, 2025

Choose a reason for hiding this comment

Gkrumbach07 Feb 13, 2025

Choose a reason for hiding this comment

Gkrumbach07 Feb 13, 2025

Choose a reason for hiding this comment

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