refactoring (support ODF in any Namespace) #1099

SanjalKatiyar · 2023-11-13T10:43:43Z

Adds a new package "immer".
Adds a new extension "console.redux-reducer", for redux state.
Adds a new hook "useOprInstallNamespaceSelector", for detecting ODF install namespace.
Removes dependency on "openshift-storage" namespace.

bipuladh · 2023-11-20T04:57:47Z

packages/odf/redux/reducers/opr-install-namespace.ts

+  switch (action.type) {
+    case nsActions.SetOprInstallNs:
+      return {
+        ...state,


This performs a shallow copy. The internal properties will be equal by reference, this can cause bugs. Use immutable to help you work with state.
see https://immutable-js.com/

done, added immer as per the suggestion...

bipuladh · 2023-11-22T01:52:08Z

packages/odf/hooks/useSafeK8sGet.ts

+ * Wrapper around "useK8sGet" hook.
+ * Ensures no API request is made unless its "safe" to do so (ODF installed namespace is fetched successfully).
+ */
+export const useSafeK8sGet = <R extends K8sResourceCommon = K8sResourceCommon>(


Is this required for the usecase when the user is not an admin?

this can use used for any case (given we have required RBACs), it saves some unnecessary API calls... in case of non-admin, if we want we can pass allowFallback = true argument which would default to openshift-storage...

bipuladh · 2023-11-22T01:56:45Z

packages/odf/components/utils/safety-box.tsx

+  anyError,
+}) =>
+  !isAllSafe ? (
+    <StatusBox loaded={isAllLoaded} loadError={anyError} />


Doesn't StatusBox take a component as child?

it does, but current way makes it possible to display children even in case of any error (upto consumer of the FC to decide what to do)...
one of the example:
<StatusBox loaded={isAllLoaded} loadError={anyError} />{children}</StatusBox> >> then in case of non-admin this will always show error as anyError will not be empty (RBAC issue), but if I pass allowFallback argument and use ternary condition (current way) I can still display children, if I want to.

bipuladh · 2023-11-22T06:40:56Z

packages/ocs/storage-class/sc-form.tsx

+          onChange={(e) => {
+            onParamChange(parameterKey, e.currentTarget.value, false);
+          }}


Why this change?

I was testing something, then forgot to remove this... will update...

bipuladh · 2023-11-22T06:41:13Z

packages/ocs/storage-class/sc-form.tsx

+  // ToDo (epic 4422): Need to pass the namespace where ceph cluster is deployed (remove from here, add dropdown)
+  React.useEffect(() => {
+    sessionStorage.setItem(
+      CEPH_CLUSTER_NAMESPACE_SESSION_STORAGE,


Suggested change

CEPH_CLUSTER_NAMESPACE_SESSION_STORAGE,

CEPH_CLUSTER_NAMESPACE,

I agree it is a long name, but it is not really a CEPH_CLUSTER_NAMESPACE, it is infact the "key" that we use to store the namespace in the session... will update anyway...

bipuladh · 2023-11-22T06:42:56Z

packages/odf/hooks/useSafeK8sGet.ts

+    useOprInstallNamespaceSelector();
+
+  const canUseFallback = allowFallback && isFallbackSafe;
+  const useK8sGetArgs = getValidK8sOptions(


Suggested change

const useK8sGetArgs = getValidK8sOptions(

const K8sGetArgs = getValidK8sOptions(

use should be prefix for hooks.

bipuladh · 2023-11-22T06:46:41Z

packages/odf/hooks/useSafeK8sGet.ts

+
+/**
+ * Wrapper around "useK8sGet" hook.
+ * Ensures no API request is made unless its "safe" to do so (ODF installed namespace is fetched successfully).


IIUC this assumption is incorrect. A fetch request will go but it will just be a useless call. Nevertheless we are adding this call to our stack. If you really want to avoid this I suggest you use the fetch functions such as k8sGetResource instead of useK8sGet hook. This applies for other safe functions as well.

it won't send any api call at all because of the resource object that I am passing to useK8sGet (I defined getValidK8sOptions for the very same reason)...
and I already updated useK8sGet as well (in this same PR), there is a condition if (kind) fetch(); that would ensure no api call...

same is true for other "safe" hook wrapper that I defined...

I don't see if (kind) fetch(); anywhere. Can you point me to it?

https://github.com/red-hat-storage/odf-console/pull/1099/files#r1401607319

bipuladh · 2023-11-22T06:52:01Z

packages/odf/redux/dispatchers/opr-install-namespace.ts

+
+type UseOprInstallNamespaceDispatch = () => (payload: nsPayload) => void;
+
+export const useOprInstallNamespaceDispatch: UseOprInstallNamespaceDispatch =


I don't see much value of having a separate component. It's used in only one place. We can just simply dispatch the action with some args. Is this used in more than one place?

not really, but it is more of a directory structure to keep things logically grouped together...
in future if we need another dispatcher (diff use case) then we can create a separate file for that (here under /redux/dispatchers) and export via index...
maybe useOprInstallNamespaceDispatch is not reused, but others (in future) could be... so it's more of a way to group things under new redux directory...

bipuladh · 2023-11-22T06:52:30Z

packages/odf/redux/provider-hooks/useOprInstallNamespace.ts

+import { getNamespace } from '@odf/shared/selectors';
+import { SubscriptionKind } from '@odf/shared/types';
+import { k8sList } from '@openshift-console/dynamic-plugin-sdk';
+import { useOprInstallNamespaceDispatch } from '../dispatchers';


Please see that you are not adding circular dependencies with imports.

I don;t think we have any circular dependency, maybe I missed somewhere ??
also we use CircularDependencyPlugin in webpack config... so I am assuming we are safe...

bipuladh · 2023-11-22T06:53:31Z

packages/odf/redux/selectors/opr-install-namespace.ts

+
+  return {
+    // installed operator namespace
+    oprInstallNs,


Could we do a better name odfNamespace? Also how are we handling usecases for MCO? Do we need to detect MCO as well?

ack !
we don't need it in MCO, there are just couple of places where we need this namespace and for that as well we need not to rely on this hook/redux (anyway MCO will be on hub and ODF on managed cluster, so this redux won't work for MCO)...

gonna be lot of changes to rename everything though !!

bipuladh · 2023-11-22T06:55:22Z

packages/odf/redux/selectors/opr-install-namespace.ts

+    getOprInstallNamespace
+  );
+
+  return {


If you return it as an array end users will not have to use the same name for variables. It might be better as we might have to support multiple namespaces for multiple operators.

but array would mean to always read "return"ed values in same order... and if I don't want to read all values leaving gaps such as: const [oprInstallNs, _, _, isSafe] = useOprInstallNamespaceSelector();...

if we want to rename we can use const { oprInstallNs: test } = useOprInstallNamespaceSelector(); right ??

SanjalKatiyar · 2023-11-22T07:21:06Z

packages/shared/src/hooks/k8s-get-hook.ts

+    if (kind) fetch();
+    else setLoaded(true);


cc @bipuladh

same is true for useK8sList.ts

Adds a new package "immer". Adds a new extension "console.redux-reducer", for redux state. Adds a new hook "useOprInstallNamespaceSelector", for detecting ODF install namespace. Removes dependency on "openshift-storage" namespace.

SanjalKatiyar · 2023-11-22T09:32:04Z

/test odf-console-e2e-aws

bipuladh · 2023-11-23T04:00:42Z

/lgtm

openshift-ci · 2023-11-23T04:00:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-ci bot added do-not-merge/work-in-progress approved labels Nov 13, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor branch 7 times, most recently from 1120a41 to 712f0be Compare November 17, 2023 09:03

SanjalKatiyar changed the title ~~[WIP] refactoring (multiple StorageClusters + ODF in any Namespace)~~ refactoring (support ODF in any Namespace) Nov 17, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 17, 2023

bipuladh requested changes Nov 20, 2023

View reviewed changes

openshift-ci bot assigned bipuladh Nov 20, 2023

SanjalKatiyar force-pushed the multiple_sc_refactor branch from 712f0be to 6fee8e8 Compare November 20, 2023 06:53

SanjalKatiyar requested a review from bipuladh November 20, 2023 08:04

bipuladh reviewed Nov 22, 2023

View reviewed changes

SanjalKatiyar commented Nov 22, 2023

View reviewed changes

refactoring (support ODF in any Namespace)

2396037

Adds a new package "immer". Adds a new extension "console.redux-reducer", for redux state. Adds a new hook "useOprInstallNamespaceSelector", for detecting ODF install namespace. Removes dependency on "openshift-storage" namespace.

SanjalKatiyar force-pushed the multiple_sc_refactor branch from 6fee8e8 to 2396037 Compare November 22, 2023 08:18

SanjalKatiyar requested a review from bipuladh November 22, 2023 08:21

bipuladh approved these changes Nov 23, 2023

View reviewed changes

openshift-ci bot added the lgtm label Nov 23, 2023

openshift-merge-bot bot merged commit 5117053 into red-hat-storage:master Nov 23, 2023
3 checks passed

SanjalKatiyar mentioned this pull request Nov 23, 2023

[tech-debt] Remove usage of OCS feature flags #835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactoring (support ODF in any Namespace) #1099

refactoring (support ODF in any Namespace) #1099

SanjalKatiyar commented Nov 13, 2023 •

edited

Loading

bipuladh Nov 20, 2023

SanjalKatiyar Nov 20, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023 •

edited

Loading

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

SanjalKatiyar Nov 22, 2023

SanjalKatiyar Nov 22, 2023

bipuladh Nov 22, 2023

SanjalKatiyar Nov 22, 2023

SanjalKatiyar Nov 22, 2023

SanjalKatiyar Nov 22, 2023

SanjalKatiyar commented Nov 22, 2023

bipuladh commented Nov 23, 2023

openshift-ci bot commented Nov 23, 2023

	CEPH_CLUSTER_NAMESPACE_SESSION_STORAGE,
	CEPH_CLUSTER_NAMESPACE,

	const useK8sGetArgs = getValidK8sOptions(
	const K8sGetArgs = getValidK8sOptions(


		type UseOprInstallNamespaceDispatch = () => (payload: nsPayload) => void;

		export const useOprInstallNamespaceDispatch: UseOprInstallNamespaceDispatch =

refactoring (support ODF in any Namespace) #1099

refactoring (support ODF in any Namespace) #1099

Conversation

SanjalKatiyar commented Nov 13, 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 Nov 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar commented Nov 22, 2023

bipuladh commented Nov 23, 2023

openshift-ci bot commented Nov 23, 2023

SanjalKatiyar commented Nov 13, 2023 •

edited

Loading

SanjalKatiyar Nov 22, 2023 •

edited

Loading