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

antgamdia · 2021-10-20T17:22:57Z

Description of the change

This PR adds a bunch of improvements in the UI side, namely:

Bugs:

Avoid duplicated key in AppList view
- No user-faced mgs, but errors in the console if two apps in different ns have the same name.
Clear the state when fetching apps
- When navigating from the catalog, attempting to deploy sth and then, going back to the installed apps and trying to upgrade something, the values were the ones from the first visited package.
Prevent async errors when unmounting some components
- No user-faced msg, but when navigating, some errors appeared in the console because of a wrong unmount of the component.

Features:

Generate and use upgrade links with a fixed version (cf UI: "new package available" link should pre-fill the latest version #3622)

Refactor:

Use the package id instead of splitting the name by (/) to get the repo name.
Use the availablePkgSummary in the catalog item instead of using a custom data model.
Refactor PackageView and DeploymentForm to ensure proper usage of cluster/ns
Delete an unused route

Benefits

Avoid aforementioned bugs, new feature and cleaning-up of our code.

Possible drawbacks

Applicable issues

Additional information

Sending the PR as a draft for triggering the CI. Some tests are still pending (like the one for the named versioned upgrade link).
Edit: CI is failing? I still have to check why.

Adding the namespace to the "key" property should avoid dups when enabling the "allNs" toggle Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

If not, the UpgradeView was loading a formerly fetched package Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Instead of passing the info in yet another custom type. However, this file needs another refactor once the opertors are a plugin Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

…luster/ns This includes: - Remove the repo from the URL; using the pkg ID instead. - Remove trailing slash when replacing the version (prev. a bug if manually accessing the url) - Simplify the useEffect hooks being used. - Add comments to clarify which cluster/ns is each one. Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity

+1 when ci passes.

absoludity · 2021-10-21T03:07:18Z

dashboard/src/components/Catalog/CatalogItems.tsx

-            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,


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.

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.

Excellent. Thanks for the explanation :)

absoludity · 2021-10-21T03:10:39Z

dashboard/src/components/PackageHeader/PackageHeader.test.tsx

@@ -42,7 +42,7 @@ const testProps: IPackageHeaderProps = {

 it("renders a header for the package", () => {
  const wrapper = mount(<PackageHeader {...testProps} />);
-  expect(wrapper.text()).toContain("testrepo/test");
+  expect(wrapper.text()).toContain("testrepo/foo");


What's triggering an expectation change here?

It is a problem with the availablePackageDetail being passed. In fact, I'm fixing it instead of changing the assertion.

Let availablePackageDetail.name be test and availablePackageDetail.availablePackageRef.identifie be testrepo/foo,
before, we were concatening repo+name, that is testrepo/test and, now, we're just using the id, that is testrepo/foo

We just should change the availablePackageDetail.name from "test" to "foo".

absoludity · 2021-10-21T03:13:06Z

dashboard/src/components/UpgradeForm/UpgradeForm.tsx

-  installedAppAvailablePackageDetail: AvailablePackageDetail;
-  installedAppInstalledPackageDetail: InstalledPackageDetail;
-  selectedPackage: IPackageState["selected"];
-  chartsIsFetching: boolean;
-  error?: Error;


So these props stopped being used or moved into state in a previous commit?

Yeah, I forgot to remove them when I moved them to the state. Now I'm just passing the version for the fixed upgrade.

absoludity · 2021-10-21T05:50:45Z

Just noticed (while working on a PR on top of this branch) that the available package title appears to need decoding (guessing it's related here, but let me know if not):

antgamdia · 2021-10-21T10:01:33Z

dashboard/src/components/Catalog/CatalogItems.tsx

-            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,


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.

antgamdia · 2021-10-21T10:04:09Z

dashboard/src/components/Catalog/PackageCatalogItem.tsx

+
+  // 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)}
    />
  );
 }


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.

antgamdia · 2021-10-21T10:10:57Z

dashboard/src/components/PackageHeader/PackageHeader.test.tsx

@@ -42,7 +42,7 @@ const testProps: IPackageHeaderProps = {

 it("renders a header for the package", () => {
  const wrapper = mount(<PackageHeader {...testProps} />);
-  expect(wrapper.text()).toContain("testrepo/test");
+  expect(wrapper.text()).toContain("testrepo/foo");


It is a problem with the availablePackageDetail being passed. In fact, I'm fixing it instead of changing the assertion.

Let availablePackageDetail.name be test and availablePackageDetail.availablePackageRef.identifie be testrepo/foo,
before, we were concatening repo+name, that is testrepo/test and, now, we're just using the id, that is testrepo/foo

We just should change the availablePackageDetail.name from "test" to "foo".

antgamdia · 2021-10-21T10:24:15Z

dashboard/src/components/UpgradeForm/UpgradeForm.tsx

-  installedAppAvailablePackageDetail: AvailablePackageDetail;
-  installedAppInstalledPackageDetail: InstalledPackageDetail;
-  selectedPackage: IPackageState["selected"];
-  chartsIsFetching: boolean;
-  error?: Error;


Yeah, I forgot to remove them when I moved them to the state. Now I'm just passing the version for the fixed upgrade.

antgamdia · 2021-10-21T10:40:22Z

Just noticed (while working on a PR on top of this branch) that the available package title appears to need decoding (guessing it's related here, but let me know if not):

I can't repro it with Helm-direct :S, I'm using the the availablePackageDetail?.availablePackageRef?.identifier, so it shouldn't be encoded at this point, since there is no need for doing so.
Anyway, I'm adding a decodeURIComponent, which should be harmless unless the pkg does contain %-escaped characters, (like %2F), but I can't think of any proper package looking like that)

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity · 2021-10-22T00:53:14Z

Just noticed (while working on a PR on top of this branch) that the available package title appears to need decoding (guessing it's related here, but let me know if not):

I can't repro it with Helm-direct :S, I'm using the the availablePackageDetail?.availablePackageRef?.identifier, so it shouldn't be encoded at this point, since there is no need for doing so. Anyway, I'm adding a decodeURIComponent, which should be harmless unless the pkg does contain %-escaped characters, (like %2F), but I can't think of any proper package looking like that)

OK, I'll recheck in my branch (using flux) once this lands.

I'll land this so I can continue and propose mine too. Thanks!

antgamdia added 13 commits October 20, 2021 18:10

Avoid duplicated key in AppList view

0be7f0b

Adding the namespace to the "key" property should avoid dups when enabling the "allNs" toggle Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Clear the state when fetching apps

8fda9d7

If not, the UpgradeView was loading a formerly fetched package Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Prevent async errors when unmonting some components

08b3ced

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Generate and use upgrade links with a fixed version

145d152

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Use the package id instead

66482be

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Use the availablePkgSummary in the catalog item

570c9a9

Instead of passing the info in yet another custom type. However, this file needs another refactor once the opertors are a plugin Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Delete unused route

fcd3aa1

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix existing tests

a44869e

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Run prettier

5403428

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'master' into ui-improvements

22c503c

Fix wrong error handling

0094428

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Remove some trivial todos

5441292

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

absoludity approved these changes Oct 21, 2021

View reviewed changes

absoludity mentioned this pull request Oct 21, 2021

Enable viewing AvailablePackageDetail with flux. #3628

Merged

antgamdia commented Oct 21, 2021

View reviewed changes

antgamdia added 9 commits October 21, 2021 12:43

Minor fix in test

026945c

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Decode id in the packageHeader

c56660b

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'master' into ui-improvements

7a30d5c

Add test for duplicated appList names

f11d2ba

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix some tests and remove some unknown types

4eaaf26

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Test AppUpgrade calls the proper actions

14028b0

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add test for passing a manual version in the upgrade form

c014f1e

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Run prettier

e853742

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix failing test

d26db2d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia marked this pull request as ready for review October 21, 2021 14:33

absoludity merged commit 3429f6a into vmware-tanzu:master Oct 22, 2021

antgamdia deleted the ui-improvements branch October 25, 2021 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

antgamdia commented Oct 20, 2021 •

edited

Loading

absoludity left a comment

absoludity Oct 21, 2021

antgamdia Oct 21, 2021

absoludity Oct 22, 2021

absoludity Oct 21, 2021

antgamdia Oct 21, 2021

absoludity Oct 21, 2021

antgamdia Oct 21, 2021

absoludity commented Oct 21, 2021

antgamdia Oct 21, 2021

antgamdia Oct 21, 2021

antgamdia Oct 21, 2021

antgamdia Oct 21, 2021

antgamdia commented Oct 21, 2021 •

edited

Loading

absoludity commented Oct 22, 2021

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

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

Conversation

antgamdia commented Oct 20, 2021 • edited Loading

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity left a 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

absoludity commented Oct 21, 2021

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

antgamdia commented Oct 21, 2021 • edited Loading

absoludity commented Oct 22, 2021

antgamdia commented Oct 20, 2021 •

edited

Loading

antgamdia commented Oct 21, 2021 •

edited

Loading