Move AppListView route to be cluster-aware. #1793

absoludity · 2020-06-15T06:36:55Z

First of several PRs which will transition existing relevant routes in the UI to
be cluster-aware, defaulting to the "default" cluster so as to not break
the existing experience.

Ref: #1762

andresmgot

Thanks @absoludity I have some minor suggestions.

BTW, I use vscode with the typescript plugin, that helps a lot for example it automatically order imports and fix some lint issues when saving files.

andresmgot · 2020-06-15T10:25:56Z

dashboard/src/components/Header/Header.tsx

      to: "apps",
    },
    {
      children: "Catalog",
-      namespaced: true,
+      clustered: false,


why is the catalog not "clustered"?

Two reasons: 1) I've not yet updated the routes and related components etc: part of the reason for replacing namespaced (which was no longer used as all were transitioned) with clustered is so I can land code in reviewable chunks, but also 2) I'm not yet certain we'll want to make the catalog URI clustered, in that we won't (at least initially) be supporting per-namespace repos/catalogs on additional clusters and so the only catalog a user should see when viewing for an additional cluster is the global catalog. I may later move this to specifically use the default cluster with _all namespaces.

Does that make sense? EDIT: all that said, it no longer exists after updating to use url.app.catalog() as per below.

If it's something temporary it's better to leave a comment saying why.

IMO, the catalog URI should be eventually clustered:

"global" App repos may differ between clusters

namespaced repos should be eventually supported

Also, in my opinion, the multi-cluster support shouldn't be claimed as "complete" if secondary clusters have less functionality than the main one. WDYT?

If it's something temporary it's better to leave a comment saying why.

As above, the code no longer exists since updating to use shared/url.

IMO, the catalog URI should be eventually clustered:

In a following PR, you'll see that I did add the /c/:cluster/ prefix to the catalog URI, because importantly, the URI defines the catalog available for install in that location (cluster+namespace), ie. the target.

* "global" App repos may differ between clusters

Global app repos are global for the Kubeapps installation in the current design. If I switched to browse a different Kubeapps installed on a different cluster (which happened to also support a union of clusters as the first installation), it may have a different set of global app repos. But I don't plan for a single Kubeapps instalation to have different global app repos depending on the cluster - there is one Kubeapps installation (with a set of global app repos) which supports multiple clusters. Let's chat about this tonight?

* namespaced repos should be eventually supported

Yep, they should, but this is tied together with deciding on the future of AppRepos with OCI in the mix, which we should do separately.

Also, in my opinion, the multi-cluster support shouldn't be claimed as "complete" if secondary clusters have less functionality than the main one. WDYT?

Do you mean in that initially it won't be possible to add a namespaced app repository to an additional cluster? If so, as above, if not - let me know :)

Global app repos are global for the Kubeapps installation in the current design. If I switched to browse a different Kubeapps installed on a different cluster (which happened to also support a union of clusters as the first installation), it may have a different set of global app repos. But I don't plan for a single Kubeapps instalation to have different global app repos depending on the cluster - there is one Kubeapps installation (with a set of global app repos) which supports multiple clusters. Let's chat about this tonight?

I think that's a valid use case that we should take into account (even if we don't implement it for the moment). As administrator, I may want to enable certain repositories for everyone with access to one cluster and other repos for a second cluster (in a dev vs prod environment for example).

Definitely - I just mean that I would do that as part of the one Kubeapps installation - enabling the configuration of different repos on different clusters as part of the one instance of Kubeapps' configuration.

andresmgot · 2020-06-15T10:27:19Z

dashboard/src/components/Header/Header.tsx

@@ -117,7 +117,7 @@ class Header extends React.Component<IHeaderProps, IHeaderState> {
                <ul className="header__nav__menu" role="menubar">
                  {this.links.map(link => (
                    <li key={link.to}>
-                      <HeaderLink {...link} currentNamespace={namespace.current} />
+                      <HeaderLink {...link} currentCluster={namespace.cluster} currentNamespace={namespace.current} />


we probably want to store the cluster in a different object. namespace.cluster is a bit confusing.

Yes - I thought the same, and then realised that the current namespace object has everything we want for a cluster: .current(Namespace), .(available)namespaces, so I was thinking of renaming the namespace object (in a separate PR) to cluster, so we'd then have:

cluster: { name: default currentNamespace: "foo", namespaces: [...], }

Switching clusters would cause the info to be reloaded in this case (which may be useful, as mentioned in the design doc, see strike-through text in 3. UX integration). The other option would be to retain data for each cluster with:

clusters: { "default": { currentNamespace: "foo", namespaces: [...], }, },

But with this option, the state is retained (though we can choose to refresh it when the cluster changes, I guess?). We could also pre-create this map in the state during initialization based on the config and use it to populate the dropdown. Thinking about it, I like this option better. Either way, I'm keen to do the state change as a separate, focused PR if that's OK with you (hence just adding the .cluster to the current namespace object).

What's your preference or thoughts with the above two options?

Cool, thanks for the explanation. I'd go with the second too so we can populate the selectors and show everything. I am not worried about refreshing this data since it's just a matter of refreshing the page.

Great, I started that yesterday, should have it done today.

andresmgot · 2020-06-15T10:31:34Z

dashboard/src/components/Header/HeaderLink.tsx

+    let link = currentNamespace ? `/ns/${currentNamespace}/${to}` : to;
+    link = clustered ? `/c/${currentCluster}${link}` : link;


let's move all the links to shared/url so we have a single place with all the links. We can have a section header there with these links.

let's move all the links to shared/url so we have a single place with all the links.

+1, I did that for the apps.list url, I'll do the same for header. That also simplifies the HeaderLink (seems to be some unused code there). II really wanted to just delete the Service Instances menu, but updated to keep including it for now.

We can have a section header there with these links.

We don't really need that, eg. the application links are just app.apps.list, so it may make more sense to just have them under app.? I've done that for app.catalog which was the only new link - see what you think.

that's fine, thanks

andresmgot · 2020-06-15T10:33:21Z

dashboard/src/containers/RoutesContainer/Routes.tsx

@@ -101,7 +102,7 @@ class Routes extends React.Component<IRoutesProps> {
  }
  private rootNamespacedRedirect = () => {
    if (this.props.namespace && this.props.authenticated) {
-      return <Redirect to={`/ns/${this.props.namespace}/apps`} />;
+      return <Redirect to={`/c/${this.props.cluster}/ns/${this.props.namespace}/apps`} />;


this link should be obtained from shared/url as well

Yep, my last-minute grep missed this one.

First of several PRs which will transition existing routes in the UI to be cluster-aware, defaulting to the "default" cluster so as to not break the existing experience.

absoludity · 2020-06-16T03:23:25Z

Thanks @absoludity I have some minor suggestions.

Updated :)

BTW, I use vscode with the typescript plugin, that helps a lot for example it automatically order imports and fix some lint issues when saving files.

Huh - so I've been using vscode for the past month or so (since getting a beefier local computer), and the Javascript/Typescript plugin (from Microsoft) is actually built-in (you can install a plugin for the nightly build, but I assume you're not doing that). It does highlight any lint errors (but they don't stand out to my slightly colour-blind eyes), but it was not automatically ordering imports. So I needed the following config (and installing the tslint extension from MS):

    "[typescript]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports": true,
            "source.fixAll.tslint": true
        }
    },
    "[typescriptreact]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports": true,
            "source.fixAll.tslint": true
        }
    }

though I don't see it fixing any lint on save yet (I'll look at this later). Thanks :)

andresmgot · 2020-06-16T09:34:00Z

LGTM but there are a couple of comments/questions that you forgot to reply.

So I needed the following config (and installing the tslint extension from MS):

Maybe those two options are conflicting. I have just the tslint option enabled globally (not per language) and it works for me:

  "editor.codeActionsOnSave": {
    "source.fixAll.tslint": false
  },

absoludity

Sorry - I meant to add a review with all the comments I had pending, but turns out I just replied to your main comment and left the comments pending :/ You should see them now.

absoludity · 2020-06-16T01:33:34Z

dashboard/src/components/Header/Header.tsx

      to: "apps",
    },
    {
      children: "Catalog",
-      namespaced: true,
+      clustered: false,


Two reasons: 1) I've not yet updated the routes and related components etc: part of the reason for replacing namespaced (which was no longer used as all were transitioned) with clustered is so I can land code in reviewable chunks, but also 2) I'm not yet certain we'll want to make the catalog URI clustered, in that we won't (at least initially) be supporting per-namespace repos/catalogs on additional clusters and so the only catalog a user should see when viewing for an additional cluster is the global catalog. I may later move this to specifically use the default cluster with _all namespaces.

Does that make sense? EDIT: all that said, it no longer exists after updating to use url.app.catalog() as per below.

absoludity · 2020-06-16T01:47:18Z

dashboard/src/components/Header/Header.tsx

@@ -117,7 +117,7 @@ class Header extends React.Component<IHeaderProps, IHeaderState> {
                <ul className="header__nav__menu" role="menubar">
                  {this.links.map(link => (
                    <li key={link.to}>
-                      <HeaderLink {...link} currentNamespace={namespace.current} />
+                      <HeaderLink {...link} currentCluster={namespace.cluster} currentNamespace={namespace.current} />


Yes - I thought the same, and then realised that the current namespace object has everything we want for a cluster: .current(Namespace), .(available)namespaces, so I was thinking of renaming the namespace object (in a separate PR) to cluster, so we'd then have:

cluster: { name: default currentNamespace: "foo", namespaces: [...], }

Switching clusters would cause the info to be reloaded in this case (which may be useful, as mentioned in the design doc, see strike-through text in 3. UX integration). The other option would be to retain data for each cluster with:

clusters: { "default": { currentNamespace: "foo", namespaces: [...], }, },

But with this option, the state is retained (though we can choose to refresh it when the cluster changes, I guess?). We could also pre-create this map in the state during initialization based on the config and use it to populate the dropdown. Thinking about it, I like this option better. Either way, I'm keen to do the state change as a separate, focused PR if that's OK with you (hence just adding the .cluster to the current namespace object).

What's your preference or thoughts with the above two options?

absoludity · 2020-06-16T01:49:28Z

dashboard/src/components/Header/HeaderLink.tsx

+    let link = currentNamespace ? `/ns/${currentNamespace}/${to}` : to;
+    link = clustered ? `/c/${currentCluster}${link}` : link;


let's move all the links to shared/url so we have a single place with all the links.

+1, I did that for the apps.list url, I'll do the same for header. That also simplifies the HeaderLink (seems to be some unused code there). II really wanted to just delete the Service Instances menu, but updated to keep including it for now.

We can have a section header there with these links.

We don't really need that, eg. the application links are just app.apps.list, so it may make more sense to just have them under app.? I've done that for app.catalog which was the only new link - see what you think.

absoludity · 2020-06-16T01:54:29Z

dashboard/src/containers/RoutesContainer/Routes.tsx

@@ -101,7 +102,7 @@ class Routes extends React.Component<IRoutesProps> {
  }
  private rootNamespacedRedirect = () => {
    if (this.props.namespace && this.props.authenticated) {
-      return <Redirect to={`/ns/${this.props.namespace}/apps`} />;
+      return <Redirect to={`/c/${this.props.cluster}/ns/${this.props.namespace}/apps`} />;


Yep, my last-minute grep missed this one.

absoludity · 2020-06-17T01:40:29Z

Sorry - I meant to add a review with all the comments I had pending, but turns out I just replied to your main comment and left the comments pending :/ You should see them now.

@andresmgot Thanks for the +1, I'll land but am still keen for your input on the state, in particular, before I refactor that (though we can do that on the separate PR, I guess - I'll move ahead as suggested above).

absoludity requested a review from andresmgot June 15, 2020 06:36

andresmgot reviewed Jun 15, 2020

View reviewed changes

absoludity force-pushed the 1761-mc-shared-backend-routes-2 branch from ffaf5dc to 41ffa6b Compare June 16, 2020 00:37

Base automatically changed from 1761-mc-shared-backend-routes-2 to master June 16, 2020 01:24

absoludity added 3 commits June 16, 2020 11:25

Move AppListView route to be cluster-aware.

f8fc28c

First of several PRs which will transition existing routes in the UI to be cluster-aware, defaulting to the "default" cluster so as to not break the existing experience.

Fix app list link from operator deletion

799231b

Add reusable url for app list.

d5e89e6

absoludity force-pushed the 1762-mc-ui-config branch from a151ba9 to d5e89e6 Compare June 16, 2020 01:29

Updated and simplified use of HeaderLink. Fixed lint.

0dd916e

andresmgot approved these changes Jun 16, 2020

View reviewed changes

absoludity commented Jun 16, 2020

View reviewed changes

Merge master and resolve

2ac9786

absoludity merged commit a0fabad into master Jun 19, 2020

absoludity deleted the 1762-mc-ui-config branch June 19, 2020 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move AppListView route to be cluster-aware. #1793

Move AppListView route to be cluster-aware. #1793

absoludity commented Jun 15, 2020 •

edited

andresmgot left a comment

andresmgot Jun 15, 2020

absoludity Jun 16, 2020

andresmgot Jun 17, 2020

absoludity Jun 18, 2020

andresmgot Jun 18, 2020

absoludity Jun 19, 2020

andresmgot Jun 15, 2020

absoludity Jun 16, 2020

andresmgot Jun 17, 2020

absoludity Jun 18, 2020

andresmgot Jun 15, 2020

absoludity Jun 16, 2020

andresmgot Jun 17, 2020

andresmgot Jun 15, 2020

absoludity Jun 16, 2020

absoludity commented Jun 16, 2020

andresmgot commented Jun 16, 2020

absoludity left a comment

absoludity Jun 16, 2020

absoludity Jun 16, 2020

absoludity Jun 16, 2020

absoludity Jun 16, 2020

absoludity commented Jun 17, 2020

		let link = currentNamespace ? `/ns/${currentNamespace}/${to}` : to;
		link = clustered ? `/c/${currentCluster}${link}` : link;

Move AppListView route to be cluster-aware. #1793

Move AppListView route to be cluster-aware. #1793

Conversation

absoludity commented Jun 15, 2020 • edited

andresmgot 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

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 Jun 16, 2020

andresmgot commented Jun 16, 2020

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

absoludity commented Jun 17, 2020

absoludity commented Jun 15, 2020 •

edited