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.

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

frontend: Details: Make multi cluster aware #2520

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions frontend/src/components/common/Resource/Resource.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import React, { PropsWithChildren, ReactNode } from 'react';
import { useTranslation } from 'react-i18next';
import { generatePath, NavLinkProps, useLocation } from 'react-router-dom';
import YAML from 'yaml';
import { labelSelectorToQuery, ResourceClasses } from '../../../lib/k8s';
import { labelSelectorToQuery, ResourceClasses, useClusterFromURLVar } from '../../../lib/k8s';
import { ApiError } from '../../../lib/k8s/apiProxy';
import { KubeCondition, KubeContainer, KubeContainerStatus } from '../../../lib/k8s/cluster';
import { KubeEvent } from '../../../lib/k8s/event';
Expand Down Expand Up @@ -89,6 +89,8 @@ export interface DetailsGridProps<T extends KubeObjectClass>
name: string;
/** Namespace of the resource. If not provided, it's assumed the resource is not namespaced. */
namespace?: string;
/** Cluster of the resource. If not provided, it's assumed single cluster mode. */
cluster?: string;
/** Sections to show in the details grid (besides the default ones). */
extraSections?:
| ((item: InstanceType<T>) => boolean | DetailsViewSection[] | ReactNode[])
Expand Down Expand Up @@ -125,13 +127,15 @@ export function DetailsGrid<T extends KubeObjectClass>(props: DetailsGridProps<T
state => state.detailsViewSection.detailsViewSectionsProcessors
);
const dispatchHeadlampEvent = useEventCallback();
const clusterFromURLVar = useClusterFromURLVar();
const cluster = props.cluster || clusterFromURLVar;

// This component used to have a MainInfoSection with all these props passed to it, so we're
// using them to accomplish the same behavior.
const { extraInfo, actions, noDefaultActions, headerStyle, backLink, title, headerSection } =
otherMainInfoSectionProps;

const [item, error] = resourceType.useGet(name, namespace) as [
const [item, error] = resourceType.useGet(name, namespace, { cluster }) as [
InstanceType<T> | null,
ApiError | null
];
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/lib/k8s/KubeObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ export class KubeObject<T extends KubeObjectInterface | KubeEvent = any> {
namespace: this.getNamespace(),
name: this.getName(),
};
const link = createRouteURL(this.detailsRoute, params);
const link = createRouteURL(this.detailsRoute, params, { cluster: this.cluster });
return link;
}

Expand Down
11 changes: 11 additions & 0 deletions frontend/src/lib/k8s/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,17 @@ export function useClusterGroup(): string[] {
return clusterGroup;
}

/**
* Use the cluster name from the URL query parameters if it's there.
*
* @returns the cluster name from the URL. If no cluster is defined in the URL, undefined is returned.
*/
export function useClusterFromURLVar(): string | undefined {
Copy link
Contributor

@sniok sniok Nov 8, 2024

Choose a reason for hiding this comment

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

I think we should be very explicit about what this means, because we already have a cluster 'variable' in the url /c/cluster/. To disambiguate I think this should be named something like useClusterFromQueryParam

of course if the approach of storing single cluster in query params stays, I only just saw discussion above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean, never mind?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking “query” is maybe overloaded with react query stuff. But maybe it’s ok.

useClusterFromSearchParam is also not quite right making it sound like it’s from a search.

useClusterFromURLQuery?
useURLQueryCluster?

Copy link
Contributor

@sniok sniok Nov 8, 2024

Choose a reason for hiding this comment

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

well there's a separate discussion about the url format, so this comment may not be relevant, let me rephrase it better

Copy link
Contributor

Choose a reason for hiding this comment

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

useURLQueryCluster sounds good
hm or maybe an approach of naming it for what it is instead of where it's stored, something like useDetailsSingleCluster?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah maybe useDetailsSingleCluster is the least misdirecting/confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well there's a separate discussion about the url format, so this comment may not be relevant, let me rephrase it better

Can you please let us know which proposal you’d prefer and why?

Copy link
Contributor

Choose a reason for hiding this comment

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

posted it in a separate comment

const location = useLocation();
const searchParams = new URLSearchParams(location.search);
return searchParams.get('cluster') || undefined;
}

export function getVersion(clusterName: string = ''): Promise<StringDict> {
return clusterRequest('/version', { cluster: clusterName || getCluster() });
}
Expand Down
19 changes: 17 additions & 2 deletions frontend/src/lib/router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,19 @@ export interface RouteURLProps {
[prop: string]: any;
}

export function createRouteURL(routeName: string, params: RouteURLProps = {}) {
/**
* Creates a URL for the given route, params and query parameters.
*
* @param routeName - The name of the route
* @param params - The optional parameters to use in the route.
* @param queryParams - The optional query parameters to use in the route.
* @returns the URL for the route as a string.
*/
export function createRouteURL(
routeName: string,
params: RouteURLProps = {},
queryParams: {} = {}
): string {
const storeRoutes = store.getState().routes.routes;
const route = (storeRoutes && storeRoutes[routeName]) || getRoute(routeName);

Expand Down Expand Up @@ -880,7 +892,10 @@ export function createRouteURL(routeName: string, params: RouteURLProps = {}) {
}

const url = getRoutePath(route);
return generatePath(url, fullParams);
const fullURL = generatePath(url, fullParams);

const searchParams = new URLSearchParams(queryParams).toString();
return searchParams ? `${fullURL}?${searchParams}` : fullURL;
}

export function getDefaultRoutes() {
Expand Down
1 change: 1 addition & 0 deletions frontend/src/plugin/__snapshots__/pluginLib.snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@
"default": [Function],
},
"useCluster": [Function],
"useClusterFromURLVar": [Function],
"useClusterGroup": [Function],
"useClustersConf": [Function],
"useClustersVersion": [Function],
Expand Down
Loading