Assign policy wizard #906

GowthamShanmugam · 2023-06-22T15:26:48Z

On top of: #903

GowthamShanmugam · 2023-06-22T15:27:05Z

/hold

GowthamShanmugam · 2023-06-28T10:08:30Z

packages/shared/dropdown/multiselectdropdown.tsx

 }) => {
  const [isOpen, setOpen] = React.useState(false);
-  const [selected, setSelected] = React.useState<string[]>(


In the Assign policy modal, the use case is to clear all selected labels for the clear button. So i want to control setSelected from outside of this component.

GowthamShanmugam · 2023-06-28T10:09:47Z

packages/shared/utils/NameValueEditor.tsx

@@ -190,6 +193,8 @@ export const NameValueEditor: React.FC<NameValueEditorProps> =
      toolTip,
      nameString,
      valueString,
+      extraProps,
+      PairElementComponent = PairElement,


I am passing my own custom component to change the NameValueEditor body, But functionality is re-used.

GowthamShanmugam · 2023-06-28T10:13:42Z

...ents/create-storage-system/create-storage-system-steps/create-local-volume-set-step/body.tsx

@@ -337,10 +338,6 @@ export const LocalVolumeSetBody: React.FC<LocalVolumeSetBodyProps> = ({
            onChange={(selectedValues: string[]) =>
              formHandler('deviceType', selectedValues)
            }
-            defaultSelected={[


Since I removed setSelected from multiselecteddropdown.tsx, it no longer has defaultSelected props. I am expecting the state variable "selected" should be pre-initialized with the default variable.

state.deviceType has default redux initialized value of this defaultSelected, no need to pass explicitly.

GowthamShanmugam · 2023-06-28T10:14:24Z

packages/mco/components/modals/app-manage-policies/utils/common.ts

@@ -0,0 +1,137 @@
+import { DRPC_STATUS } from '@odf/mco/constants';
+import { DisasterRecoveryFormatted } from '@odf/mco/hooks';


I can reuse this file for a subscription app also, so created separate util for that

GowthamShanmugam · 2023-06-28T10:28:22Z

Screencast.from.2023-06-28.15-57-10.webm

GowthamShanmugam · 2023-07-04T21:46:06Z

/hold cancel

GowthamShanmugam · 2023-07-05T10:55:06Z

packages/mco/constants/common.ts

@@ -14,3 +14,5 @@ export const enum VOLUME_REPLICATION_HEALTH {
 export const ACM_OBSERVABILITY_FLAG = 'ACM_OBSERVABILITY';
 export const ADMIN_FLAG = 'ADMIN';
 export const ODFMCO_OPERATOR_NAMESPACE = 'openshift-operators';
+export const ACM_SEARCH_PROXY_URL =
+  '/api/proxy/plugin/acm/console/multicloud/proxy/search';


Need to move all ACM related constant in separate file

GowthamShanmugam · 2023-07-05T10:56:28Z

packages/mco/hooks/argo-application-set.ts

+    drPlacementControl?: DRPlacementControlKind;
+    drPolicy?: DRPolicyKind;
+    drClusters?: DRClusterKind[];


drpc resources are optional for unprotected apps

GowthamShanmugam · 2023-07-05T10:58:59Z

packages/shared/src/dropdown/multiselectdropdown.tsx

 }) => {
  const [isOpen, setOpen] = React.useState(false);
-  const [selected, setSelected] = React.useState<string[]>(


I have a use case like removing the selected Placement from outside of this component,

SanjalKatiyar · 2023-07-07T12:10:14Z

locales/en/plugin__odf-console.json

@@ -214,7 +225,17 @@
  "Assigned on": "Assigned on",
  "View configurations": "View configurations",
  "No activity": "No activity",
-  "Back": "Back",
+  "Delete": "Delete",
+  "Select  a placement": "Select  a placement",


Suggested change

"Select a placement": "Select a placement",

"Select a placement": "Select a placement",

SanjalKatiyar · 2023-07-07T12:26:19Z

packages/mco/components/modals/app-manage-policies/app-manage-policies-modal.tsx

+    };
+  } else {
+    return {
+      title: t('Manage data policy'),


are we still calling it Manage data policy even though we only allow assign as of now ?

Ya, But Title and description needs to be decided, doc team is working on this

SanjalKatiyar · 2023-07-08T07:04:15Z

packages/mco/components/modals/app-manage-policies/assign-policy-view.tsx

+
+  const assignPolicy = () => {
+    setAssignDisabled(true);
+    const updateContext = async (


Suggested change

const updateContext = async (

const updateContext = (

SanjalKatiyar · 2023-07-08T07:29:46Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+const getPlacementTags = (drpcs: DRPlacementControlType[]) =>
+  !!drpcs.length
+    ? drpcs.map(
+        (drpc) => [getName(drpc.placementInfo), drpc?.pvcSelector] || []


it will never reach this condition...

Suggested change

(drpc) => [getName(drpc.placementInfo), drpc?.pvcSelector] || []

(drpc) => [getName(drpc.placementInfo), drpc?.pvcSelector]

SanjalKatiyar · 2023-07-08T07:34:47Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+const getLabelsFromSearchResult = (searchResult: SearchResult): string[] => {
+  const pvcLabels =
+    searchResult?.data.searchResult?.[0]?.items.reduce(
+      (acc, item) => [...acc, ...item?.[LABEL]?.split(SPLIT_CHAR)],


spread will give error when item?.[LABEL]?.split(SPLIT_CHAR) is undefined...

SanjalKatiyar · 2023-07-08T07:39:25Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+const getLabelsFromSearchResult = (searchResult: SearchResult): string[] => {
+  const pvcLabels =
+    searchResult?.data.searchResult?.[0]?.items.reduce(
+      (acc, item) => [...acc, ...item?.[LABEL]?.split(SPLIT_CHAR)],


this should work ??

Suggested change

(acc, item) => [...acc, ...item?.[LABEL]?.split(SPLIT_CHAR)],

(acc, item) => [...acc, ...item[LABEL]?.split(SPLIT_CHAR)],

SanjalKatiyar · 2023-07-08T07:50:50Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+
+const getPlacementsFromTags = (tags: TagsType, currIndex: number): string[] =>
+  tags.reduce((acc, tag, index) => {
+    const placement = tag?.[0];


this is not an array like labels ??

tag is an array type and it has only two indexes, one is the placement of string type, other one is the label of string[] type.

type TagsType = (string | string[])[][];

basically tag?.[0] is string, tag?.[1]; is array

SanjalKatiyar · 2023-07-08T07:54:58Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+const NameValueEditorComponent = (props) => (
+  <AsyncLoader
+    loader={() =>
+      import('@odf/shared/utils/NameValueEditor').then((c) => c.NameValueEditor)
+    }
+    {...props}
+  />
+);


nits: better to export this as LazyNameValueEditor from shared and use that everywhere...

also do we even need lazy import ??

We are using lazy loading against NameValueEditor component in multiple places for code splitting: https://legacy.reactjs.org/docs/code-splitting.html

To avoid increasing the size of the bundle using some complex library.

Thanks, @bipuladh for the ref link

yup, I got that... but my doubt was whether we are saving lot of bundle size by doing splitting here ??
Default NameValueEditor had code for PairElement, but in MCO we are passing that as a prop as well PairElementComponent (which means it is getting bundled in same code as the one which is calling the FC)... so is there any major advantage of splitting here ??

anyway, no issues with using lazy loading, but don;t see any specific reason as well here... by some complex library do u mean these ??

import { DndProvider } from 'react-dnd'; import { HTML5Backend } from 'react-dnd-html5-backend';

Maybe i am wrong here but I thought bundle creation is nothing to do with runtime calls, MCO calls the PairElementComponent, or PairElement is decided at runtime, so it will still affect the bundle size when i use NameValueEditor directly.

SanjalKatiyar · 2023-07-08T08:08:55Z

packages/mco/components/modals/app-manage-policies/utils/acm-utils.ts

+            values: clusterNames,
+          },
+        ],
+        limit: 20,


do we need limit ??

in the workloadNamespace, can there be PVCs which are not related to this Application, how we decide whether a PVC is independent or belongs to an ACM Application ??

Yes, UI API read needs a limit. UI can't read large amounts of PVCs. It will affect the performance. As per PM and QE, the number of PVCs per app is around 20 only, the count may go a little bit higher in the future but never read all PVCs blindly.

UI can't decide which PVCs are related to the app and which PVCs are not realted. UI is just assuming all PVCs under the namespace are realted to the app only. It is up to the user to decide which label to select.

so it will mean that for cases where app has more than 20 PVCs, our UI will not work as expected and can show some missing labels ??

UI needs a limit, If they wants to use more PVC, then they have to use CLI way

my question here would be limit just tells us how my data points the response from the API call will be right ??
If limit=20 or limit=200, in both cases it will still be a single API request right ?? so will it make diff to hard limit it to be 20 PVCs ?? @GowthamShanmugam

in list pages, say Secrets a cluster can have 300 secrets as well... all we display on UI and we send a single API call which gives us all 300 secret in a single response...

UI just need to maintain a list of all PVCs (which it can easily), any other performance impact which I might be missing ??

We usually send an API request to the local cluster, But here ACM search API is sending an API request to the managed clusters.

ACM is sending an API request to its managed cluster agent, and an agent will receive the API request and read all PVC from the namespace and convert the result to a custom response object and send it back to the hub cluster.

if we give a limit of 20 then it will read only first 20 PVCs from the namespace. it means the API request will be waiting for the 20 PVC results to be parsed. If we increase the limit then it will take increase the API response time.

Per namespace, I dont think the user will create more than 20 PVCs, If QE thinks the number is not enough i will increase more.

The idea here is, not all PVCs should have a unique namespace, the user has to reuse the labels as much as possible between PVCs

SanjalKatiyar · 2023-07-08T08:12:11Z

packages/mco/components/modals/app-manage-policies/utils/parser-utils.ts

+export const getClusterNamesFromPlacements = (placements: PlacementType[]) =>
+  placements?.reduce(
+    (acc, placemnt) => [...acc, ...placemnt?.deploymentClusters],
+    []
+  );


just to confirm, r u sure we already do not have this function defined somewhere ?? seems like something we have done at other places as well.

this is for the custom object type, the old one for the PlacementDecisionKind object which takes from clusters from status?.decisions

SanjalKatiyar · 2023-07-08T08:18:02Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+    );
+
+    // ACM search proxy api call
+    const [searchResult] = useACMSafeFetch(


what about handling loading and error ??

I thought about this, but with the current UX I dont see any places to display error message or any other option, Need to check with ux what to do

SanjalKatiyar · 2023-07-08T08:20:06Z

packages/mco/components/modals/app-manage-policies/helper/pvc-details-wizard-content.tsx

+const createDRPlacementControlObj = (
+  placement: PlacementType,
+  dataPolicyRef: ObjectReference
+): DRPlacementControlType => ({
+  apiVersion: DRPlacementControlModel.apiVersion,
+  kind: DRPlacementControlModel.kind,
+  metadata: {
+    name: `${getName(placement)}-drpc`,
+    namespace: getNamespace(placement),
+  },
+  drPolicyRef: dataPolicyRef,
+  placementInfo: placement,
+  pvcSelector: [],
+});
+
+const updateLabels = (
+  drPlacementControl: DRPlacementControlType,
+  labels: string[]
+): DRPlacementControlType => ({
+  ...drPlacementControl,
+  pvcSelector: labels,
+});


can't we re-use getDRPCKindObj from utils ??

This is a custom-type object for wizard flow,
All the components under the manage policy modal will understand this custom type,
So I am keeping this custom type to reuse components, for example, policy configuration view (used for review and create page, as well as to display policy configurations)
At the end of the assign policy wizard i am converting this custom type to using getDRPCKindObj to create DRPC.

SanjalKatiyar · 2023-07-10T08:34:01Z

packages/mco/hooks/acm-safe-fetch.ts

+      .catch((err) => {
+        setError(err);
+      });
+  }, [setResponse, searchQuery, safeFetch]);


Suggested change

}, [setResponse, searchQuery, safeFetch]);

}, [setResponse, searchQuery]);

safeFetch is also one dependency

not needed as it won;t change (check useURLPoll)...
if needed anyway then wrap safeFetch in a useCallback before returning it from the hook...

currently if u add it as it is then it will cause issues...

not sure how it works for useURLPoll, i am getting lint error if i am not adding it in dependency

u can disable the lint error... or if u do not want to do that let;s use useCallback so that ref of safeFetch do not changes...

for disabling use // eslint-disable-next-line <error_name>

if want to use useCallback u need to use it in the hook itself that returns safeFetch.

Signed-off-by: Gowtham Shanmugasundaram <[email protected]>

bipuladh

lgtm

SanjalKatiyar · 2023-07-10T12:37:13Z

/approve
/lgtm

openshift-ci · 2023-07-10T12:37:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar

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:

~~OWNERS~~ [SanjalKatiyar]

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

bipuladh · 2023-07-10T13:32:25Z

/test odf-console-e2e-aws

openshift-merge-robot added the needs-rebase label Jun 22, 2023

GowthamShanmugam changed the title ~~Assign policy wizard~~ Assign policy wizard - WIP Jun 22, 2023

openshift-ci bot added the do-not-merge/hold label Jun 22, 2023

GowthamShanmugam force-pushed the RHSTOR-4605 branch 2 times, most recently from 236d1fc to fed040c Compare June 26, 2023 06:43

openshift-ci bot added the approved label Jun 26, 2023

openshift-merge-robot removed the needs-rebase label Jun 26, 2023

GowthamShanmugam force-pushed the RHSTOR-4605 branch from fed040c to 7d54c68 Compare June 27, 2023 15:19

openshift-ci bot removed the approved label Jun 27, 2023

GowthamShanmugam force-pushed the RHSTOR-4605 branch from 7d54c68 to 0c69e3c Compare June 28, 2023 10:05

GowthamShanmugam commented Jun 28, 2023

View reviewed changes

GowthamShanmugam force-pushed the RHSTOR-4605 branch from 0c69e3c to b0faa16 Compare June 28, 2023 10:26

openshift-merge-robot added the needs-rebase label Jul 4, 2023

GowthamShanmugam force-pushed the RHSTOR-4605 branch from b0faa16 to 0c11eea Compare July 4, 2023 17:23

openshift-merge-robot removed the needs-rebase label Jul 4, 2023

GowthamShanmugam changed the title ~~Assign policy wizard - WIP~~ Assign policy wizard Jul 4, 2023

GowthamShanmugam force-pushed the RHSTOR-4605 branch from 0c11eea to fcc4846 Compare July 4, 2023 17:36

openshift-ci bot removed the do-not-merge/hold label Jul 4, 2023

GowthamShanmugam force-pushed the RHSTOR-4605 branch from a772c18 to 4bc26d9 Compare July 5, 2023 10:33

GowthamShanmugam commented Jul 5, 2023

View reviewed changes

GowthamShanmugam force-pushed the RHSTOR-4605 branch 3 times, most recently from b1ce37f to 1f026c2 Compare July 7, 2023 11:11

SanjalKatiyar reviewed Jul 7, 2023

View reviewed changes

SanjalKatiyar reviewed Jul 8, 2023

View reviewed changes

SanjalKatiyar reviewed Jul 10, 2023

View reviewed changes

GowthamShanmugam force-pushed the RHSTOR-4605 branch 2 times, most recently from d478a7e to 08d45c9 Compare July 10, 2023 11:23

Assign policy wizard

f01c7a1

Signed-off-by: Gowtham Shanmugasundaram <[email protected]>

GowthamShanmugam force-pushed the RHSTOR-4605 branch from 08d45c9 to f01c7a1 Compare July 10, 2023 11:50

bipuladh reviewed Jul 10, 2023

View reviewed changes

openshift-ci bot assigned SanjalKatiyar Jul 10, 2023

openshift-ci bot added the lgtm label Jul 10, 2023

openshift-ci bot added the approved label Jul 10, 2023

openshift-merge-robot merged commit e40d0bf into red-hat-storage:master Jul 10, 2023

		@@ -0,0 +1,137 @@
		import { DRPC_STATUS } from '@odf/mco/constants';
		import { DisasterRecoveryFormatted } from '@odf/mco/hooks';

	"Select a placement": "Select a placement",
	"Select a placement": "Select a placement",

	(drpc) => [getName(drpc.placementInfo), drpc?.pvcSelector] \|\| []
	(drpc) => [getName(drpc.placementInfo), drpc?.pvcSelector]

	(acc, item) => [...acc, ...item?.[LABEL]?.split(SPLIT_CHAR)],
	(acc, item) => [...acc, ...item[LABEL]?.split(SPLIT_CHAR)],

	}, [setResponse, searchQuery, safeFetch]);
	}, [setResponse, searchQuery]);

Assign policy wizard #906

Assign policy wizard #906

Conversation

GowthamShanmugam commented Jun 22, 2023 • edited Loading

GowthamShanmugam commented Jun 22, 2023

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

GowthamShanmugam commented Jun 28, 2023

GowthamShanmugam commented Jul 4, 2023

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

SanjalKatiyar Jul 8, 2023 • edited Loading

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

SanjalKatiyar Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

SanjalKatiyar Jul 10, 2023 • edited Loading

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

SanjalKatiyar Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

GowthamShanmugam Jul 10, 2023 • edited Loading

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

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

SanjalKatiyar Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh left a comment

Choose a reason for hiding this comment

SanjalKatiyar commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

bipuladh commented Jul 10, 2023

GowthamShanmugam commented Jun 22, 2023 •

edited

Loading

SanjalKatiyar Jul 8, 2023 •

edited

Loading

SanjalKatiyar Jul 10, 2023 •

edited

Loading

SanjalKatiyar Jul 10, 2023 •

edited

Loading

SanjalKatiyar Jul 10, 2023 •

edited

Loading

GowthamShanmugam Jul 10, 2023 •

edited

Loading

SanjalKatiyar Jul 10, 2023 •

edited

Loading