Fix being able to view clusters outside the current workspace #2355

Nokel81 · 2021-03-17T20:33:03Z

Completely removes ClusterStore.activeCluster
Every workspace now tracks it current activeCluster
If an active cluster is removed then the workspace's activeClusterId is set to undefined
Only show welcome notification on the first time a non-managed workspace is viewed in the workspace overview
Add unit tests for the WorkspaceStore
Add validation that only valid clusters can be set to the activeClusterId field
Marks the current workspace as disabled in the switch workspace's command

Signed-off-by: Sebastian Malton sebastian@malton.name

fixes #2348

future work:

I could make it so that if you disconnect from a cluster it pick another one to go to instead of popping you back to the overview screen (see end of video).

New look:

Screen.Recording.2021-03-17.at.4.30.55.PM.mov

ixrock

LGTM

ixrock · 2021-03-18T11:59:36Z

src/common/workspace-store.ts

@@ -152,16 +241,24 @@ export class WorkspaceStore extends BaseStore<WorkspaceStoreModel> {
  private static stateRequestChannel = "workspace:states";

  @observable currentWorkspaceId = WorkspaceStore.defaultId;
+
+  #seenWorkspaces = observable.set<WorkspaceId>();


Why do we use private fields here btw?

Do you mean instead of typescript's private modifier? I felt that this was a better way to prevent outside consumers from using the raw values

src/renderer/components/cluster-manager/clusters-menu.tsx

Nokel81 · 2021-03-18T14:51:00Z

Hopefully this can be in rc.2

src/renderer/components/+landing-page/landing-page.tsx

jim-docker

This is a lot of refactoring to introduce between 4.2.0-rc.1 and rc.2. Did you consider doing just a surgical fix for the issue now, and saving this for 4.2.1 (or even 4.3)?

src/common/workspace-store.ts

src/extensions/stores/cluster-store.ts

src/main/index.ts

Nokel81 · 2021-03-19T14:09:21Z

I did consider a more patchwork fix, we could push this to 4.2.1 depending on how other people feel about this.

I think that if we test it enough it should be fine to put it into rc.2 though.

src/extensions/stores/cluster-store.ts

jim-docker · 2021-03-19T14:37:00Z

src/renderer/components/+landing-page/landing-page.tsx


 @observer
 export class LandingPage extends React.Component {
+  private static storage = createStorage<WorkspaceId[]>("landing_page", []);


so it will persist, nice, great idea.

src/renderer/components/+landing-page/landing-page.tsx

Nokel81 · 2021-03-19T14:45:56Z

I have moved this to target 4.2.1

jim-docker · 2021-03-19T15:05:40Z

I have moved this to target 4.2.1

this will be a known issue for 4.2.0 then?

Nokel81 · 2021-03-19T15:07:00Z

Yes in the final release notes I'll add that.

src/extensions/stores/cluster-store.ts

jim-docker · 2021-03-19T22:45:41Z

src/renderer/components/+landing-page/landing-page.tsx

+      LandingPage
+        .storage
+        .get()
+        .filter(id => workspaceStore.getById(id))


src/renderer/components/+landing-page/landing-page.tsx

github-actions · 2021-03-23T12:44:20Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2021-04-06T14:24:01Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

stefcameron · 2021-04-07T21:44:43Z

@Nokel81 Can you give me an overview of what the new API to activate a cluster in a workspace looks like now, with changes in this PR, since it sounds like it'll fix #1759 ?

Nokel81 · 2021-04-07T23:26:15Z

@stefcameron I think it will be as simple as <LensExtensionInstance>.navigate("/cluster/<cluster-id>"); but I will verify this in the morning.

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 · 2021-04-08T17:46:13Z

@stefcameron Done, I have added it. I don't know why the LensExtension.navigate method is only limited to a specific extension but whatever.

You need to do the following:

import { Store } from "@k8slens/extensions";

...
const cluster = ...;

await Store.workspaceStore.setActiveCluster(cluster); // or cluster ID

stefcameron · 2021-04-08T18:17:52Z

@stefcameron Done, I have added it. I don't know why the LensExtension.navigate method is only limited to a specific extension but whatever.

You need to do the following:
import { Store } from "@k8slens/extensions";

...
const cluster = ...;

await Store.workspaceStore.setActiveCluster(cluster); // or cluster ID

Thanks for following-up on this, @Nokel81 ! I will watch for this in 4.2.1 (unless I get to it off master before then) and will update my code accordingly.

Based on the call, the cluster doesn't have to be in the active workspace. Could be in some other workspace, and Lens will also activate the workspace as a result? Of course, this means Lens now ensures (maybe it always has; I haven't specifically tested) that all clusters have unique IDs...

Nokel81 · 2021-04-08T18:19:23Z

No that call will update the active workspace too. Though now that I think about it. I think we should instead have it be Navigation.navigate("/cluster/<cluster-id"); and instead change the active workspace on the fly. So I might back out this change.

Nokel81 · 2021-04-08T18:21:26Z

But I need to think about it. I only mention this because workspace are going away and I would prefer if what I told you will also work in 5.0

stefcameron · 2021-04-08T18:26:04Z

But I need to think about it. I only mention this because workspace are going away and I would prefer if what I told you will also work in 5.0

Whaaaaat? 😉 OK, I'd like to know a way that will survive, though if workspaces disappear, that will definitely impact my extension.

BTW, extension.navigate("/cluster/<cluster-id>"); is what I'm already doing, but it's not reliable, which is the reason for #1759

Nokel81 · 2021-04-08T21:49:15Z

@stefcameron So as we discussed, I am going to leave this API as is since putting more work into this is misplaced effort.

stefcameron · 2021-04-08T22:20:29Z

@stefcameron So as we discussed, I am going to leave this API as is since putting more work into this is misplaced effort.

You mean it will be await Store.workspaceStore.setActiveCluster(cluster); // or cluster ID for now until Lens 5.0 when workspaces disappear? I assume there will be some equivalent method off the Catalogue API to activate an object at that point.

Nokel81 · 2021-04-08T23:47:15Z

Yeah that.

src/renderer/components/select/select.scss

Signed-off-by: Sebastian Malton <sebastian@malton.name>

nevalla

LGTM

Nokel81 added the bug Something isn't working label Mar 17, 2021

Nokel81 added this to the 4.2.0 milestone Mar 17, 2021

Nokel81 requested a review from a team March 17, 2021 20:33

Nokel81 self-assigned this Mar 17, 2021

ixrock previously approved these changes Mar 18, 2021

View reviewed changes

Nokel81 dismissed ixrock’s stale review via 22baee1 March 18, 2021 12:30

Nokel81 requested review from ixrock and a team March 18, 2021 12:30

Nokel81 mentioned this pull request Mar 18, 2021

Change Cluster Settings button to be consistent with cluster icon menu #2065

Merged

Nokel81 added the blocker label Mar 18, 2021

ixrock previously approved these changes Mar 18, 2021

View reviewed changes

jim-docker reviewed Mar 18, 2021

View reviewed changes

src/renderer/components/+landing-page/landing-page.tsx Outdated Show resolved Hide resolved

Nokel81 dismissed ixrock’s stale review via ba3cff4 March 18, 2021 19:55

jim-docker reviewed Mar 18, 2021

View reviewed changes

src/common/workspace-store.ts Outdated Show resolved Hide resolved

jim-docker reviewed Mar 18, 2021

View reviewed changes

Nokel81 force-pushed the issue-2348 branch from ba3cff4 to e2f5174 Compare March 19, 2021 13:43

Nokel81 modified the milestones: 4.2.0, 4.2.1 Mar 19, 2021

jim-docker reviewed Mar 19, 2021

View reviewed changes

Nokel81 requested review from jim-docker and ixrock March 19, 2021 19:23

jim-docker mentioned this pull request Mar 19, 2021

random UI "lockup" in 4.2.0-rc.1 #2368

Closed

jim-docker reviewed Mar 19, 2021

View reviewed changes

github-actions bot added the PR: needs rebase label Mar 23, 2021

Nokel81 force-pushed the issue-2348 branch from 39f4117 to 13a06c8 Compare April 6, 2021 14:23

github-actions bot removed the PR: needs rebase label Apr 6, 2021

add reliable mechanism for extensions to activate a cluster

872280d

Signed-off-by: Sebastian Malton <sebastian@malton.name>

Nokel81 linked an issue Apr 8, 2021 that may be closed by this pull request

An extension cannot reliably ask Lens to activate a cluster #1759

Closed

stefcameron mentioned this pull request Apr 8, 2021

Update how we activate and switch to a cluster once Lens issue fixed Mirantis/lens-extension-cc#26

Closed

Nokel81 modified the milestones: 4.2.1, 4.2.2 Apr 9, 2021

Nokel81 requested review from a team, jim-docker, jakolehm and nevalla and removed request for jim-docker April 14, 2021 12:48

jakolehm reviewed Apr 14, 2021

View reviewed changes

src/renderer/components/select/select.scss Outdated Show resolved Hide resolved

revert select.scss

0bce536

Signed-off-by: Sebastian Malton <sebastian@malton.name>

jakolehm approved these changes Apr 14, 2021

View reviewed changes

nevalla approved these changes Apr 14, 2021

View reviewed changes

Nokel81 merged commit b6c3ce7 into release/v4.2 Apr 14, 2021

Nokel81 deleted the issue-2348 branch April 14, 2021 13:49

Nokel81 mentioned this pull request Apr 15, 2021

release v4.2.2 #2538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix being able to view clusters outside the current workspace #2355

Fix being able to view clusters outside the current workspace #2355

Nokel81 commented Mar 17, 2021

ixrock left a comment

ixrock Mar 18, 2021

Nokel81 Mar 18, 2021

Nokel81 commented Mar 18, 2021

jim-docker left a comment

Nokel81 commented Mar 19, 2021

jim-docker Mar 19, 2021

Nokel81 commented Mar 19, 2021

jim-docker commented Mar 19, 2021

Nokel81 commented Mar 19, 2021

jim-docker Mar 19, 2021

github-actions bot commented Mar 23, 2021

github-actions bot commented Apr 6, 2021

stefcameron commented Apr 7, 2021

Nokel81 commented Apr 7, 2021

Nokel81 commented Apr 8, 2021

stefcameron commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

stefcameron commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

stefcameron commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

nevalla left a comment

Fix being able to view clusters outside the current workspace #2355

Fix being able to view clusters outside the current workspace #2355

Conversation

Nokel81 commented Mar 17, 2021

future work:

New look:

ixrock left a comment

Choose a reason for hiding this comment

ixrock Mar 18, 2021

Choose a reason for hiding this comment

Nokel81 Mar 18, 2021

Choose a reason for hiding this comment

Nokel81 commented Mar 18, 2021

jim-docker left a comment

Choose a reason for hiding this comment

Nokel81 commented Mar 19, 2021

jim-docker Mar 19, 2021

Choose a reason for hiding this comment

Nokel81 commented Mar 19, 2021

jim-docker commented Mar 19, 2021

Nokel81 commented Mar 19, 2021

jim-docker Mar 19, 2021

Choose a reason for hiding this comment

github-actions bot commented Mar 23, 2021

github-actions bot commented Apr 6, 2021

stefcameron commented Apr 7, 2021

Nokel81 commented Apr 7, 2021

Nokel81 commented Apr 8, 2021

stefcameron commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

stefcameron commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

stefcameron commented Apr 8, 2021

Nokel81 commented Apr 8, 2021

nevalla left a comment

Choose a reason for hiding this comment