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.

Sign up for GitHub

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

Jump to bottom

Add cluster in url - Newest cleaned up code #7

Open
wants to merge 3 commits into
base: multi-cluster
Choose a base branch
from
+343 −18
Open

Add cluster in url - Newest cleaned up code #7

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99a35fc
Add cluster in url
kdoberst Aug 29, 2021
788d60f
Feedback from PR
kdoberst Sep 28, 2021
64cb38e
Feedback from PR
kdoberst Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 38 additions & 5 deletions frontend/packages/console-app/src/components/detect-cluster/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,17 @@ import * as React from 'react';
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore: FIXME missing exports due to out-of-sync @types/react-redux version
import { useDispatch } from 'react-redux';
import { setActiveCluster } from '@console/internal/actions/ui';
import { useLocation } from 'react-router-dom';
import isMultiClusterEnabled from '@console/app/src/utils/isMultiClusterEnabled';
import { setActiveCluster, formatNamespaceRoute } from '@console/internal/actions/ui';
import { getCluster } from '@console/internal/components/utils/link';
import { history } from '@console/internal/components/utils/router';
// import { useActiveNamespace } from '@console/shared';
import { LAST_CLUSTER_USER_SETTINGS_KEY } from '@console/shared/src/constants';
import { useUserSettings } from '@console/shared/src/hooks/useUserSettings';
import store from '../../../../../public/redux';
Copy link
Author

Choose a reason for hiding this comment

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

Add the store isn't the best option, but there were weird bugs when using useActiveNamespace. The bug is most notable when going to the pods list page for all projects and then clicking on a pod. Instead of going to the details page, the user was routed back to the list page.

Copy link
Owner

Choose a reason for hiding this comment

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

Do you see the same behavior is you use useSelector directly?

  return useSelector(({ UI }) => UI.get('activeNamespace'));

Copy link
Author

Choose a reason for hiding this comment

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

Good question. I thought about that last night. I'll check it out.

Copy link
Author

Choose a reason for hiding this comment

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

First changed the path to the store to import store from '@console/internal/redux'; so at least it isn't relative.

So neither the useSelector nor the activeNamespace from useActiveNamespace worked. Here is what I think is going on. We need to know the current Namespace inside an useEffect hook, but since useSelector and useActiveNamespace are hooks and React gets grumppy when trying to put a hook in hook, I need to put them outside of the useEffect block. I'm thinking that when I do that the namespace from the hook is a step behind when calling the useEffect.

So not sure what to do.


export const multiClusterRoutePrefixes = ['/k8s/all-namespaces', '/k8s/cluster', '/k8s/ns'];

type ClusterContextType = {
cluster?: string;
Expand All @@ -28,14 +36,39 @@ export const useValuesForClusterContext = () => {
[dispatch, setLastCluster],
);

// const [activeNamespace] = useActiveNamespace();
const urlCluster = getCluster(useLocation().pathname);
React.useEffect(() => {
// TODO: Detect cluster from URL.
if (lastClusterLoaded && lastCluster) {
if (urlCluster) {
setLastCluster(urlCluster);
dispatch(setActiveCluster(urlCluster));
} else if (lastClusterLoaded && lastCluster) {
dispatch(setActiveCluster(lastCluster));
}
// Only run this hook after last cluster is loaded.

if (
isMultiClusterEnabled() &&
lastClusterLoaded &&
lastCluster &&
!urlCluster &&
multiClusterRoutePrefixes.some((pattern) => window.location.pathname.startsWith(pattern))
) {
const activeNamespace = store.getState().UI.get('activeNamespace');
const newPath = formatNamespaceRoute(
activeNamespace,
window.location.pathname,
window.location,
false,
lastCluster,
);

if (newPath !== window.location.pathname) {
history.pushPath(newPath);
}
}
// Only run this hook after last cluster is loaded or window path changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [lastClusterLoaded]);
}, [lastClusterLoaded, urlCluster, window.location.pathname]);
Copy link
Author

Choose a reason for hiding this comment

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

Putting window.location.pathname seems a little weird, but it works.


return {
cluster: lastCluster,
Expand Down
6 changes: 5 additions & 1 deletion frontend/packages/console-shared/src/utils/__tests__/user-settings.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ describe('createConfigMap', () => {

expect(actual).toEqual(configMap);
expect(coFetchMock).toHaveBeenCalledTimes(1);
expect(coFetchMock).lastCalledWith('/api/console/user-settings', { method: 'POST' });
expect(coFetchMock).lastCalledWith('/api/console/user-settings', {
headers: { 'X-Cluster': 'local-cluster' },
method: 'POST',
});
});
});

Expand All @@ -61,6 +64,7 @@ describe('updateConfigMap', () => {
headers: {
Accept: 'application/json',
'Content-Type': 'application/merge-patch+json;charset=UTF-8',
'X-Cluster': 'local-cluster',
},
body: '{"data":{"key":"value"}}',
},
Expand Down
19 changes: 17 additions & 2 deletions frontend/public/actions/ui.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
ALL_NAMESPACES_KEY,
LAST_NAMESPACE_NAME_LOCAL_STORAGE_KEY,
} from '@console/shared/src/constants';
// import { multiClusterRoutePrefixes } from '@console/app/src/components/detect-cluster/cluster'
Copy link
Author

Choose a reason for hiding this comment

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

Using the import caused most of the tests to fail. Unsure of cause, so the prefixes are duplicated in the code below.

Copy link
Author

Choose a reason for hiding this comment

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

I tested this again. When you import the multiClusterRoutePrefixes it causes this test suite to fail:
packages/console-shared/src/hooks/__tests__/useUserSettings.spec.ts

Each test fails with this message:

useUserSettings › should return default value for an empty configmap after switching from loading to loaded

    TypeError: user_settings_1.seralizeData is not a function

Seems kind of crazy pants to me, but I tested it numerous times and it fails without fail (hahaha) when using the import.

Copy link
Author

Choose a reason for hiding this comment

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

Stack trace for failed test:

      at invokePassiveEffectCreate (node_modules/react-dom/cjs/react-dom.development.js:23487:20)
      at HTMLUnknownElement.callCallback (node_modules/react-dom/cjs/react-dom.development.js:3945:14)
      at Object.invokeGuardedCallbackDev (node_modules/react-dom/cjs/react-dom.development.js:3994:16)
      at invokeGuardedCallback (node_modules/react-dom/cjs/react-dom.development.js:4056:31)
      at flushPassiveEffectsImpl (node_modules/react-dom/cjs/react-dom.development.js:23574:9)
      at unstable_runWithPriority (node_modules/scheduler/cjs/scheduler.development.js:646:12)
      at runWithPriority$1 (node_modules/react-dom/cjs/react-dom.development.js:11276:10)
      at flushPassiveEffects (node_modules/react-dom/cjs/react-dom.development.js:23447:14)
      at Object.<anonymous>.flushWork (node_modules/react-dom/cjs/react-dom-test-utils.development.js:992:10)
      at flushWorkAndMicroTasks (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1001:5)
      at node_modules/react-dom/cjs/react-dom-test-utils.development.js:1080:11
      ```

Copy link

Choose a reason for hiding this comment

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

This is most likely an issue with a jest.mock in the user settings hook unit test.

import { K8sResourceKind, PodKind, NodeKind } from '../module/k8s';
import { allModels } from '../module/k8s/k8s-models';
import { detectFeatures, clearSSARFlags } from './features';
Expand Down Expand Up @@ -152,10 +153,23 @@ export const formatNamespaceRoute = (
originalPath,
location?,
forceList?: boolean,
activeCluster?: string,
) => {
let path = originalPath.substr(window.SERVER_FLAGS.basePath.length);

let parts = path.split('/').filter((p) => p);

if (parts[0] === 'cluster') {
// The url pattern that includes the cluster name starts with /cluster/:clusterName
parts.splice(0, 2);
}

const multiClusterRoutePrefixes = ['/k8s/all-namespaces', '/k8s/cluster', '/k8s/ns'];
const clusterPathPart =
activeCluster && multiClusterRoutePrefixes.includes(`/${parts[0]}/${parts[1]}`)
? `/cluster/${activeCluster}`
: '';

const prefix = parts.shift();

let previousNS;
Expand All @@ -168,7 +182,7 @@ export const formatNamespaceRoute = (
}

if (!previousNS) {
return originalPath;
return `${clusterPathPart}${originalPath}`;
}

if (
Expand All @@ -193,7 +207,7 @@ export const formatNamespaceRoute = (
path += `${location.search}${location.hash}`;
}

return path;
return `${clusterPathPart}${path}`;
};

export const setCurrentLocation = (location: string) =>
Expand All @@ -211,6 +225,7 @@ export const setActiveNamespace = (namespace: string = '') => {
// make it noop when new active namespace is the same
// otherwise users will get page refresh and cry about
// broken direct links and bookmarks

if (namespace !== getActiveNamespace()) {
const oldPath = window.location.pathname;
const newPath = formatNamespaceRoute(namespace, oldPath, window.location);
Expand Down
Loading