cdb(front): Add namespace selector #7030

orfeas-k · 2023-03-06T14:51:06Z

This PR is part of the Rewrite the Central Dashboard in Angular effort and comes right after this one.

Namespace Service

Responsible for handling all actions related to the namespace in CDB (keeping information about the current namespace, retrieving most recent namespace used, updating components about namespace change etc) will be the new NamespaceService. Any component that needs to interact with the app's namespace will use this service.

Initializing the namespace

Once we receive the namespaces available, we need to initialize the selected namespace. We follow the below steps in order to decide upon the namespace.

Check if namespace is set through the query parameters of current URL.
Check if the user's previous namespace is present in the browser's local storage
Check in the namespaces list for a namespace with Owner role (and keep the first one)
Check in the namespaces list for the first valid namespace

In every of these steps, if we find a namespace, we return it only if this namespace option is also valid (considering also the All namespaces option).

After we finalize on the namespace, we also:

Emit an observable with it to the currentNamespace subscribers
Update the namespace for this specific user in the browser's localStorage
Update the browser's URL's query parameters with the initialized namespace

Select namespace

The last three steps also take place when a user changes the selected namespace via the Namespace selector dropdown.

Single user mode

In the early days, there was the notion of running Kubeflow for a single user. In that case there was no header in the requests with the user info. Extending what we decided here for isolationMode variable (at that point we did it in regard to Kubeflow's Version), we decide to drop the single-user mode from CDB. Thus, we will also drop requests at /api/workgroup/exists endpoint, from where we received until now user related information.

Based though on that information, we also enabled/disabled the Registration page and modified the content of the Manage contributors page.

Manage contributors page

We want admins to manage users in a GitOps way, and not via that UI.

Registration page

We also had the Registration page where the user can create a user Profile (which creates a namespace).

For the moment, we move forward without implementing these two pages in the new Central Dashboard.

Post `PARENT_CONNECTED_EVENT` to iframed WA

We decided to not post a message of type PARENT_CONNECTED_EVENT as we did in the old dashboard (we will only post a message with the selected namespace). We don't see any use of it currently and thus, we will not include it in CDB.

Fix Cypress and Karma types conflicts resulting in VSCode errors about expect() statements Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

tasos-ale

@orfeas-k Thanks for your PR. Please take a look at my comments.

tasos-ale · 2023-03-14T11:54:21Z

...s/centraldashboard-angular/frontend/src/app/pages/iframe-wrapper/iframe-wrapper.component.ts

+  /**
+   * Receives a message from an iframe page and passes the selected namespace.
+   * @param {MessageEvent} event
+   */
+  private onMessageReceived(event: any) {


Since we are using TS we don't need JSDoc types, so we can replace the any type here. I don't have a strong opinion about the description, we can keep it even if this method is private.

tasos-ale · 2023-03-14T17:17:30Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+  private setCurrentNamespace(namespaces: Namespace[], user: string) {
+    if (user) {


Is it possible to not have the user when we use the setCurrentNamespace()?

tasos-ale · 2023-03-14T17:18:35Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+    this.router.events.subscribe(event => {
+      if (event instanceof NavigationEnd) {


We can use a filter() and remove the if statement since we only care for NavigationEnd

tasos-ale · 2023-03-14T17:32:42Z

...s/centraldashboard-angular/frontend/src/app/pages/iframe-wrapper/iframe-wrapper.component.ts

+        /**
+         * Append current namespace to query parameters before using router.navigate
+         * since iframe's internal URL doesn't hold any information regarding the
+         * namespace and we want to prevent it from discarding this infromation from
+         * the Browser's URL.
+         */


FYI https://google.github.io/styleguide/jsguide.html#formatting-block-comment-style. There is no need to use /**, you can change it to /*.

tasos-ale · 2023-03-14T17:35:01Z

components/centraldashboard-angular/frontend/src/app/services/environment.service.ts

+      if (res.user) {
+        this.user.next(res.user);
+      }
+      if (res.platform) {
+        this.platform.next(res.platform);
+      }
+      if (res.namespaces) {
+        this.namespaces.next(res.namespaces);
+      }
+    });


Do we really need 3 different subjects for these values?

We don't really need 3 different subjects since we fetch all of the above simultaneously from the same request. I followed the above approach because I found it to be cleaner since some components may need only one of these properties. However, I 'm open to reconsidering it, since this results in some other components having to subscribe more than once.

I don't have a strong opinion either. So, let's not change it.

tasos-ale · 2023-03-14T17:43:25Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+  private updateStoragedNamespace(namespace: Namespace | undefined) {
+    // Save the user's choice so we are able to restore it,
+    // when re-loading the page without a queryParam
+    const localStorageKey =
+      'selectedNamespace/' + ((this.user && '.' + this.user) || '');
+    this.localStorage.set(localStorageKey, namespace?.namespace);
+  }


In general, try to pass everything as a parameter. It will make testing easier.

tasos-ale

A few more comments @orfeas-k .

tasos-ale · 2023-03-21T16:21:48Z

components/centraldashboard-angular/frontend/src/app/services/environment.service.ts

+  public platform = new ReplaySubject<PlatformInfo>();
+  public user = new ReplaySubject<string>();
+  public namespaces = new ReplaySubject<Namespace[]>();
+  public dashboardLinks = new ReplaySubject<DashboardLinks>();


Let's add 1 as the buffer size here.

tasos-ale · 2023-03-21T16:22:21Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+  public namespacesSubject = new ReplaySubject<Namespace[]>();
+  public currentNamespaceSubject = new ReplaySubject<Namespace>();
+  public allNamespacesAllowedSubject = new ReplaySubject<boolean>();


Same here with buffer size.

tasos-ale · 2023-03-21T16:24:31Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+    this.env.user.subscribe((user: string) => {
+      this.user = user;
+    });
+
+    this.env.namespaces.subscribe((namespaces: Namespace[]) => {
+      namespaces = [this.allNamespacesOption, ...namespaces];
+      this.namespaces = namespaces;
+      this.namespacesSubject.next(namespaces);
+
+      this.allNamespacesAllowed = this.calcAllNamespacesOption(this.router.url);
+      this.allNamespacesAllowedSubject.next(this.allNamespacesAllowed);
+
+      this.setCurrentNamespace(namespaces, this.user);
+    });
+
+    this.router.events.subscribe(event => {


In order to avoid having 3 different subscriptions we can subscribe to events first and then switch to user and namespaces.

tasos-ale · 2023-03-21T16:25:50Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+        this.currentNamespaceSubject.next(this.currentNamespace);
+        this.updateStoragedNamespace(this.currentNamespace);
+        this.updateQueryParams(this.currentNamespace.namespace);


We could probably reuse selectNamespace here with a few changes.

orfeas-k · 2023-03-24T12:20:33Z

@tasos-ale Thank you for your comments. I pushed commits that address them. This includes as well a rewrite of the namespace service core functionality, as well as tests for different parts of this PR. Let me know what you think!

tasos-ale

@orfeas-k a few minor comments.

tasos-ale · 2023-03-24T14:38:58Z

...s/centraldashboard-angular/frontend/src/app/pages/iframe-wrapper/iframe-wrapper.component.ts

+  }
+
+  // Receives a message from an iframe page and passes the selected namespace.
+  private onMessageReceived(event: any) {


Suggested change

private onMessageReceived(event: any) {

private onMessageReceived(event: MessageEvent) {

tasos-ale · 2023-03-24T14:55:00Z

...-angular/frontend/src/app/pages/main-page/namespace-selector/namespace-selector.component.ts

+    });
+
+    this.ns.currentNamespaceSubject.subscribe((namespace: Namespace) => {
+      console.log(namespace);


Suggested change

console.log(namespace);

tasos-ale · 2023-03-24T15:20:07Z

components/centraldashboard-angular/frontend/src/app/services/namespace.service.ts

+  namespacesSubject = this._namespacesSubject.asObservable();
+  currentNamespaceSubject = this._currentNamespaceSubject.asObservable();
+
+  private user: string;
+  private currentNamespace: Namespace | undefined;


Maybe we can do this:

Suggested change

namespacesSubject = this._namespacesSubject.asObservable();

currentNamespaceSubject = this._currentNamespaceSubject.asObservable();

private user: string;

private currentNamespace: Namespace | undefined;

namespaces = this._namespacesSubject.asObservable();

currentNamespace = this._currentNamespaceSubject.asObservable();

private _user: string;

private _currentNamespace: Namespace | undefined;

wdyt @orfeas-k ?

Sure, but I 'll rename them from _variable to prvVariable because that's what we do the in the rest of KF WAs as well.

google-oss-prow · 2023-03-24T15:40:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tasos-ale

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/centraldashboard-angular/OWNERS~~ [tasos-ale]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Introduce EnvironmentService that will be responsible for storing information coming from the backend service and that are related to the environment. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Introduce LocalStorageService in order to wrap the native localStorage API and use the same prefix for items used by CDB. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Introduce and use SvgIconsService service in order to register custom icons MatIcon directive and provide them app-wide. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Introduce CDBNamespaceService that will be responsible managing the namespace for the whole app. Any component that may need to access/change the namespace being used, will have to use this service. More specifically, it will: - Hold information about namespace and emit observables when this changes. - Inform components about available namespaces. - Initialize the namespace when it receives the available namespaces. - Modify the namespace's value. - Decide if All namespaces option is allowed at all times. - Provide components with namespace related constants. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Introduce and use NamespaceSelector component in the top toolbar which allows the user to view and modify the selected namespace. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Pass the namespace query parameter (if present) to the iframe component, using the routerLink's queryParams property. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Post message containing the current namespace to the iframed WA. This communication becomes possible because of library.js. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Include subcase for getBackendErrorLog when neither error.error nor error.log are available. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Left sidebar menu now handles namespaced links by replacing '{ns}' with the current namespace. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Add UI tests with Cypress that check that the namespace selector is initialized prioritizing correctly between options available. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Add unit tests for MainPageComponent's isLinkActive() and getNamespaceParams(). Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

Add unit tests for CDB's namespace service to check that it: - validates a namespace as expected - updates query parameters with namespace Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

tasos-ale · 2023-03-25T13:13:14Z

/lgtm

cdb(front): Fix cypress and Karma types conflict

5d10764

Fix Cypress and Karma types conflicts resulting in VSCode errors about expect() statements Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

google-oss-prow bot requested review from kimwnasptd and tasos-ale March 6, 2023 14:51

google-oss-prow bot added the size/XL label Mar 6, 2023

orfeas-k force-pushed the feature-orfeas-cdb-namespace-selector branch from 9c9f1bf to 2056634 Compare March 6, 2023 15:52

tasos-ale suggested changes Mar 14, 2023

View reviewed changes

google-oss-prow bot added size/XXL and removed size/XL labels Mar 21, 2023

tasos-ale reviewed Mar 21, 2023

View reviewed changes

tasos-ale suggested changes Mar 24, 2023

View reviewed changes

tasos-ale approved these changes Mar 24, 2023

View reviewed changes

google-oss-prow bot added the approved label Mar 24, 2023

orfeas-k added 12 commits March 24, 2023 17:46

cdb(front): Introduce EnvironmentService

73d76bd

Introduce EnvironmentService that will be responsible for storing information coming from the backend service and that are related to the environment. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Introduce LocalStorageService

41b68c4

Introduce LocalStorageService in order to wrap the native localStorage API and use the same prefix for items used by CDB. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Introduce SvgIconsService

806aef5

Introduce and use SvgIconsService service in order to register custom icons MatIcon directive and provide them app-wide. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Introduce NamespaceSelector component

e6fb352

Introduce and use NamespaceSelector component in the top toolbar which allows the user to view and modify the selected namespace. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Pass query parameters to iframe

0388341

Pass the namespace query parameter (if present) to the iframe component, using the routerLink's queryParams property. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Post message with namespace to iframed WA

63ab237

Post message containing the current namespace to the iframed WA. This communication becomes possible because of library.js. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Improve ErrorInterceptor getBackendError

7d7a1a9

Include subcase for getBackendErrorLog when neither error.error nor error.log are available. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Handle namespaced menu items

d979038

Left sidebar menu now handles namespaced links by replacing '{ns}' with the current namespace. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Add UI tests for namespace selector

c882b46

Add UI tests with Cypress that check that the namespace selector is initialized prioritizing correctly between options available. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Add unit tests for MainPageComponent

d27b3df

Add unit tests for MainPageComponent's isLinkActive() and getNamespaceParams(). Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

cdb(front): Add unit tests for CDBNamespaceService

e38f99c

Add unit tests for CDB's namespace service to check that it: - validates a namespace as expected - updates query parameters with namespace Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>

orfeas-k force-pushed the feature-orfeas-cdb-namespace-selector branch from a57908d to e38f99c Compare March 24, 2023 15:47

google-oss-prow bot assigned tasos-ale Mar 25, 2023

google-oss-prow bot added the lgtm label Mar 25, 2023

google-oss-prow bot merged commit 0aba01d into kubeflow:master Mar 25, 2023

tasos-ale deleted the feature-orfeas-cdb-namespace-selector branch March 25, 2023 13:14

orfeas-k mentioned this pull request Mar 28, 2023

cdb(front): Introduce 404 not found page #7071

Merged

orfeas-k mentioned this pull request Apr 22, 2024

Rewrite the Central Dashboard in Angular #6000

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cdb(front): Add namespace selector #7030

cdb(front): Add namespace selector #7030

orfeas-k commented Mar 6, 2023 •

edited

Loading

tasos-ale left a comment

tasos-ale Mar 14, 2023

tasos-ale Mar 14, 2023

tasos-ale Mar 14, 2023

tasos-ale Mar 14, 2023

tasos-ale Mar 14, 2023

orfeas-k Mar 20, 2023

tasos-ale Mar 21, 2023

tasos-ale Mar 14, 2023

tasos-ale left a comment

tasos-ale Mar 21, 2023

tasos-ale Mar 21, 2023

tasos-ale Mar 21, 2023

tasos-ale Mar 21, 2023

orfeas-k commented Mar 24, 2023

tasos-ale left a comment

tasos-ale Mar 24, 2023

tasos-ale Mar 24, 2023

tasos-ale Mar 24, 2023

orfeas-k Mar 24, 2023

google-oss-prow bot commented Mar 24, 2023

tasos-ale commented Mar 25, 2023

		private setCurrentNamespace(namespaces: Namespace[], user: string) {
		if (user) {

		this.router.events.subscribe(event => {
		if (event instanceof NavigationEnd) {

	private onMessageReceived(event: any) {
	private onMessageReceived(event: MessageEvent) {

cdb(front): Add namespace selector #7030

cdb(front): Add namespace selector #7030

Conversation

orfeas-k commented Mar 6, 2023 • edited Loading

Namespace Service

Initializing the namespace

Select namespace

Single user mode

Manage contributors page

Registration page

Post PARENT_CONNECTED_EVENT to iframed WA

tasos-ale 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

tasos-ale 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

orfeas-k commented Mar 24, 2023

tasos-ale 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

google-oss-prow bot commented Mar 24, 2023

tasos-ale commented Mar 25, 2023

orfeas-k commented Mar 6, 2023 •

edited

Loading

Post `PARENT_CONNECTED_EVENT` to iframed WA