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

Check cluster/ns in views + refactor + ui improvements #3623

Merged
merged 22 commits into from
Oct 22, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
2 changes: 0 additions & 2 deletions dashboard/src/actions/apps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ export function installPackage(
{ cluster: targetCluster, namespace: targetNamespace } as Context,
releaseName,
availablePackageDetail.availablePackageRef,
// TODO(agamez): check if this VersionReference we're using is what we expect
{ version: availablePackageDetail.version.pkgVersion } as VersionReference,
values,
);
Expand Down Expand Up @@ -233,7 +232,6 @@ export function updateInstalledPackage(
if (availablePackageDetail?.version?.pkgVersion) {
await App.UpdateInstalledPackage(
installedPackageRef,
// TODO(agamez): check if this VersionReference we're using is what we expect
{ version: availablePackageDetail.version.pkgVersion } as VersionReference,
values,
);
Expand Down
8 changes: 4 additions & 4 deletions dashboard/src/components/AppList/AppList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ context("when apps available", () => {
const wrapper = mountWrapper(getStore(state), <AppList />);
const itemList = wrapper.find(AppListItem);
expect(itemList).toExist();
expect(itemList.key()).toBe("bar/foo");
expect(itemList.key()).toBe("foobar-bar/foo");
});

it("filters apps", () => {
Expand All @@ -239,7 +239,7 @@ context("when apps available", () => {
installedPackageRef: {
identifier: "foo/bar",
pkgVersion: "1.0.0",
context: { cluster: "", namespace: "foobar" } as Context,
context: { cluster: "", namespace: "fooNs" } as Context,
plugin: { name: "my.plugin", version: "0.0.1" } as Plugin,
} as InstalledPackageReference,
status: {
Expand All @@ -257,7 +257,7 @@ context("when apps available", () => {
installedPackageRef: {
identifier: "foobar/bar",
pkgVersion: "1.0.0",
context: { cluster: "", namespace: "foobar" } as Context,
context: { cluster: "", namespace: "fooNs" } as Context,
plugin: { name: "my.plugin", version: "0.0.1" } as Plugin,
} as InstalledPackageReference,
status: {
Expand All @@ -280,7 +280,7 @@ context("when apps available", () => {
<AppList />
</MemoryRouter>,
);
expect(wrapper.find(AppListItem).key()).toBe("foobar/bar");
expect(wrapper.find(AppListItem).key()).toBe("fooNs-foobar/bar");
});
});

Expand Down
8 changes: 7 additions & 1 deletion dashboard/src/components/AppList/AppListGrid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,13 @@ function AppListGrid(props: IAppListProps) {
<Row>
<>
{filteredReleases.map(r => {
return <AppListItem key={r.installedPackageRef?.identifier} app={r} cluster={cluster} />;
return (
<AppListItem
key={`${r.installedPackageRef?.context?.namespace}-${r.installedPackageRef?.identifier}`}
app={r}
cluster={cluster}
/>
);
})}
{filteredCRs.map(r => {
const csv = props.csvs.find(c =>
Expand Down
6 changes: 4 additions & 2 deletions dashboard/src/components/AppList/AppListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ function AppListItem(props: IAppListItemProps) {
<></>
);

return (
return app?.installedPackageRef ? (
<InfoCard
key={app.installedPackageRef?.identifier}
link={url.app.apps.get(app?.installedPackageRef)}
link={url.app.apps.get(app.installedPackageRef)}
title={app.name}
icon={icon}
info={
Expand All @@ -79,6 +79,8 @@ function AppListItem(props: IAppListItemProps) {
tooltip={tooltip}
bgIcon={getPluginIcon(app.installedPackageRef?.plugin)}
/>
) : (
<></>
);
}

Expand Down
6 changes: 4 additions & 2 deletions dashboard/src/components/AppUpgrade/AppUpgrade.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ interface IRouteParams {
releaseName: string;
pluginName: string;
pluginVersion: string;
version?: string;
}

function AppUpgrade() {
const dispatch: ThunkDispatch<IStoreState, null, Action> = useDispatch();
const { cluster, namespace, releaseName, pluginName, pluginVersion } =
const { cluster, namespace, releaseName, pluginName, pluginVersion, version } =
ReactRouter.useParams() as IRouteParams;

const {
Expand All @@ -41,6 +42,7 @@ function AppUpgrade() {

// Initial fetch using the params in the URL
useEffect(() => {
dispatch(actions.packages.resetSelectedAvailablePackageDetail());
dispatch(
actions.apps.getApp({
context: { cluster: cluster, namespace: namespace },
Expand Down Expand Up @@ -70,7 +72,7 @@ function AppUpgrade() {
if (installedAppAvailablePackageDetail && installedAppInstalledPackageDetail && selectedPackage) {
return (
<div>
<UpgradeForm />
<UpgradeForm version={version} />
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ export default function StatusAwareButton<T extends IStatusAwareButtonProps>(pro
"The application is being deleted.",
[InstalledPackageStatus_StatusReason.STATUS_REASON_PENDING]:
"The application is pending installation.",
// 7: "The application is pending upgrade.", // TODO(agamez): do we have a standard code for that?
// 8: "The application is pending rollback.", // TODO(agamez): do we have a standard code for that?
};

const tooltip = releaseStatus?.reason ? tooltips[releaseStatus.reason] : undefined;
return (
<>
Expand Down
5 changes: 3 additions & 2 deletions dashboard/src/components/AppView/AppView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ const routeParams = {
cluster: "cluster-1",
namespace: "default",
releaseName: "mr-sunshine",
plugin: { name: "my.plugin", version: "0.0.1" } as Plugin,
};
const routePathParam = `/foo/${routeParams.cluster}/${routeParams.namespace}/${routeParams.releaseName}`;
const routePath = "/foo/:cluster/:namespace/:releaseName";
const routePathParam = `/c/${routeParams.cluster}/ns/${routeParams.namespace}/apps/${routeParams.plugin.name}/${routeParams.plugin.version}/${routeParams.releaseName}`;
const routePath = "/c/:cluster/ns/:namespace/apps/:pluginName/:pluginVersion/:releaseName";
let spyOnUseDispatch: jest.SpyInstance;
const appActions = { ...actions.apps };
const kubeaActions = { ...actions.kube };
Expand Down
5 changes: 3 additions & 2 deletions dashboard/src/components/AppView/AppView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,12 @@ export default function AppView() {

useEffect(() => {
if (!app || !app.manifest) {
return;
return () => {};
}

if (Object.values(resourceRefs).some(ref => ref.length)) {
// Already populated, skip
return;
return () => {};
}

let parsedManifest: IResource[] = YAML.parseAllDocuments(app.manifest).map(
Expand All @@ -231,6 +231,7 @@ export default function AppView() {
// Avoid setting refs if the manifest is empty
setResourceRefs(parsedRefs);
}
return () => {};
}, [app, cluster, kinds, resourceRefs]);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,17 @@ export default function PackageUpdateInfo({ installedPackageDetail }: IPackageUp
);
}
// App is up to date
return alertContent ? (
return alertContent && installedPackageDetail?.installedPackageRef ? (
<Alert>
{alertContent}
<Link to={appURL.apps.upgrade(installedPackageDetail.installedPackageRef)}>Update Now</Link>
<Link
to={appURL.apps.upgradeTo(
installedPackageDetail.installedPackageRef,
installedPackageDetail.latestVersion?.pkgVersion,
)}
>
Update Now
</Link>
</Alert>
) : (
<div className="color-icon-success">
Expand Down
3 changes: 2 additions & 1 deletion dashboard/src/components/Catalog/Catalog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,11 @@ export default function Catalog() {
if (!supportedCluster || namespace === kubeappsNamespace) {
// Global namespace or other cluster, show global repos only
dispatch(actions.repos.fetchRepos(kubeappsNamespace));
return;
return () => {};
}
// In other case, fetch global and namespace repos
dispatch(actions.repos.fetchRepos(namespace, true));
return () => {};
}, [dispatch, supportedCluster, namespace, kubeappsNamespace]);

useEffect(() => {
Expand Down
18 changes: 8 additions & 10 deletions dashboard/src/components/Catalog/CatalogItem.tsx
Original file line number Diff line number Diff line change
@@ -1,27 +1,25 @@
import { Plugin } from "gen/kubeappsapis/core/plugins/v1alpha1/plugins";
import { IRepo } from "shared/types";
import PackageCatalogItem from "./PackageCatalogItem";
import { AvailablePackageSummary } from "gen/kubeappsapis/core/packages/v1alpha1/packages";
import OperatorCatalogItem from "./OperatorCatalogItem";
import PackageCatalogItem from "./PackageCatalogItem";

// TODO: this file should be refactored after the operators have been integrated in a plugin

export interface ICatalogItem {
id: string;
name: string;
version: string;
description: string;
cluster: string;
namespace: string;
icon?: string;
}

export interface IPackageCatalogItem extends ICatalogItem {
id: string;
repo: IRepo;
plugin: Plugin;
availablePackageSummary: AvailablePackageSummary;
}

export interface IOperatorCatalogItem extends ICatalogItem {
id: string;
csv: string;
version: string;
description: string;
icon?: string;
}

export interface ICatalogItemProps {
Expand Down
31 changes: 12 additions & 19 deletions dashboard/src/components/Catalog/CatalogItems.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { AvailablePackageSummary } from "gen/kubeappsapis/core/packages/v1alpha1/packages";
import { Plugin } from "gen/kubeappsapis/core/plugins/v1alpha1/plugins";
import { useMemo } from "react";
import { getIcon } from "shared/Operators";
import { IClusterServiceVersion, IRepo } from "shared/types";
import placeholder from "../../placeholder.png";
import CatalogItem, { ICatalogItemProps } from "./CatalogItem";
import { IClusterServiceVersion } from "shared/types";
import CatalogItem, {
ICatalogItemProps,
IOperatorCatalogItem,
IPackageCatalogItem,
} from "./CatalogItem";
export interface ICatalogItemsProps {
availablePackageSummaries: AvailablePackageSummary[];
csvs: IClusterServiceVersion[];
Expand All @@ -28,25 +30,16 @@ export default function CatalogItems({
() =>
availablePackageSummaries.map(c => {
return {
// TODO: this should be simplified once the operators are also implemented as a plugin
type: `${c.availablePackageRef?.plugin?.name}/${c.availablePackageRef?.plugin?.version}`,
id: `package/${c.availablePackageRef?.identifier}`,
item: {
plugin: c.availablePackageRef?.plugin ?? ({ name: "", version: "" } as Plugin),
id: `package/${c.availablePackageRef?.identifier}/${c.latestVersion?.pkgVersion}`,
name: c.displayName,
icon: c.iconUrl ?? placeholder,
version: c.latestVersion?.pkgVersion ?? "",
description: c.shortDescription,
// TODO(agamez): get the repo name once available
// https://github.com/kubeapps/kubeapps/issues/3165#issuecomment-884574732
repo: {
name: c.availablePackageRef?.identifier.split("/")[0],
namespace: c.availablePackageRef?.context?.namespace,
} as IRepo,
Comment on lines -34 to -45
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me from context how all this data can be removed (or replaced with availablePackageSummary) - unless you've updated elsewhere to populate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't have the chance to write some comments to ease the review, but, yep, of course, I had to update it elsewhere. Particulary, this info is being used in the <InfoCard> that is rendered at PackageCatalogItem.tsx.

Ideally, these components would get simplified to just an AvailablePackageSummaryItem, but currently we sill to differentiate between a package and an operator.
Wherefore I just simply extracted the operator things (namespace, description, etc) back to the OperatorCatalogItem and simplify as much as I could the package item. However, I had to leave some fields (like the name: c.displayName as they were being used in the CatalogItem.

But, at least, we are converging, step by step, in simplifying the UI logic in favor of the new package data model.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent. Thanks for the explanation :)

cluster,
namespace,
},
};
availablePackageSummary: c,
} as IPackageCatalogItem,
} as ICatalogItemProps;
}),
[availablePackageSummaries, cluster, namespace],
);
Expand All @@ -68,8 +61,8 @@ export default function CatalogItems({
csv: csv.metadata.name,
cluster,
namespace,
},
};
} as IOperatorCatalogItem,
} as ICatalogItemProps;
});
} else {
return [];
Expand Down
59 changes: 37 additions & 22 deletions dashboard/src/components/Catalog/PackageCatalogItem.tsx
Original file line number Diff line number Diff line change
@@ -1,38 +1,53 @@
import { useSelector } from "react-redux";
import { IRepo, IStoreState } from "shared/types";
import { IStoreState } from "shared/types";
import * as url from "shared/url";
import { getPluginIcon, trimDescription } from "shared/utils";
import { getPluginIcon, PluginNames, trimDescription } from "shared/utils";
import placeholder from "../../placeholder.png";
import InfoCard from "../InfoCard/InfoCard";
import { IPackageCatalogItem } from "./CatalogItem";

export default function PackageCatalogItem(props: IPackageCatalogItem) {
const { icon, name, repo, version, description, namespace, id } = props;
const {
config: { kubeappsNamespace },
} = useSelector((state: IStoreState) => state);
const iconSrc = icon || placeholder;
const cluster = useSelector((state: IStoreState) => state.clusters.currentCluster);
const link = url.app.packages.get(
const { cluster, namespace, availablePackageSummary } = props;
const { config } = useSelector((state: IStoreState) => state);

// A package is "global" if its cluster and namespace are the ones in which Kubeapps is installed on
const isGlobal =
availablePackageSummary.availablePackageRef?.context?.namespace === config.kubeappsNamespace &&
availablePackageSummary.availablePackageRef?.context?.cluster === config.kubeappsCluster;

// Use the current cluster/namespace in the URL (passed as props here),
// but, if it is global a "global" segement will be included in the generated URL.
const packageViewLink = url.app.packages.get(
cluster,
namespace,
name,
repo || ({} as IRepo),
kubeappsNamespace,
props.plugin,
availablePackageSummary.availablePackageRef!,
isGlobal,
);
const bgIcon = getPluginIcon(props.plugin);

// Historically, this tag is used to show the repository a given package is from,
// but each plugin as its own way to describe the repository right now.
let repositoryName;
switch (availablePackageSummary.availablePackageRef?.plugin?.name) {
case PluginNames.PACKAGES_HELM:
repositoryName = availablePackageSummary.availablePackageRef?.identifier.split("/")[0];
break;
// TODO: consider the fluxv2 plugin
default:
// Fallback to the plugin name
repositoryName = availablePackageSummary.availablePackageRef?.plugin?.name;
break;
}

return (
<InfoCard
key={id}
title={decodeURIComponent(name)}
link={link}
info={version || ""}
icon={iconSrc}
description={trimDescription(description)}
tag1Content={<span>{repo.name}</span>}
bgIcon={bgIcon}
key={availablePackageSummary.availablePackageRef?.identifier}
title={decodeURIComponent(availablePackageSummary.displayName)}
link={packageViewLink}
info={availablePackageSummary?.latestVersion?.pkgVersion || ""}
icon={availablePackageSummary.iconUrl || placeholder}
description={trimDescription(availablePackageSummary.shortDescription)}
tag1Content={<span>{repositoryName}</span>}
bgIcon={getPluginIcon(availablePackageSummary.availablePackageRef?.plugin)}
/>
);
}
Comment on lines +26 to 53
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me from context how all this data can be removed (or replaced with availablePackageSummary) - unless you've updated elsewhere to populate that.

Here are the changes indeed. I've used this way to extract the repo name; not sure if we would want to include it as part of the API, though.

5 changes: 3 additions & 2 deletions dashboard/src/components/Config/AppRepoList/AppRepoList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,16 @@ function AppRepoList() {
if (!namespace) {
// All Namespaces
dispatch(actions.repos.fetchRepos(""));
return;
return () => {};
}
if (!supportedCluster || namespace === kubeappsNamespace) {
// Global namespace or other cluster, show global repos only
dispatch(actions.repos.fetchRepos(kubeappsNamespace));
return;
return () => {};
}
// In other case, fetch global and namespace repos
dispatch(actions.repos.fetchRepos(namespace, true));
return () => {};
}, [dispatch, supportedCluster, namespace, kubeappsNamespace]);

useEffect(() => {
Expand Down
Loading