[Workspace] Feature/workspace service core module #5092

ruanyl · 2023-09-22T14:25:12Z

Description

The core workspace module(WorkspaceService) is a foundational component that enables the implementation of workspace features within OSD plugins. The purpose of the core workspace module is to provide a framework for workspace implementations.

This module does not implement specific workspace functionality but provides the essential infrastructure for plugins to
extend and customize workspace features, it maintains a shared workspace state(observables) across the entire application to ensure a consistent and up-to-date view of workspace-related information to all parts of the application.

Why introducing these changes to core?

The main consideration of introducing core workspace module includes:

The workspace is a fundamental feature for OSD, the workspace state, such as currentWorkspace, workspaceList, etc maybe/is used by other core modules and core plugins, having it in core seems the most reasonable approached and it also makes it easy to be accessed.
We don't want to make the workspace implementation opinionated, if encapsulating these changes in workspace plugin, it will make other core modules and core plugins to depend on workspace plugin. So we moved some abstractions to core(considering the efforts of maintaining such a small workspace module in core which I think worth it).

Issues Resolved

Partially resolved #5091

Screenshot

Testing the changes

Check List

codecov · 2023-09-22T14:41:59Z

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 66.79%. Comparing base (f28b729) to head (a21adee).
Report is 735 commits behind head on main.

Files with missing lines	Patch %	Lines
src/core/public/workspace/workspaces_service.ts	88.88%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5092      +/-   ##
==========================================
+ Coverage   66.73%   66.79%   +0.05%     
==========================================
  Files        3284     3286       +2     
  Lines       63095    63129      +34     
  Branches    10049    10053       +4     
==========================================
+ Hits        42108    42164      +56     
+ Misses      18589    18570      -19     
+ Partials     2398     2395       -3

Flag	Coverage Δ
Linux_1	`35.25% <26.66%> (-0.01%)`	⬇️
Linux_2	`55.29% <94.11%> (?)`
Linux_3	`43.82% <26.66%> (-0.02%)`	⬇️
Linux_4	`35.35% <26.66%> (-0.01%)`	⬇️
Windows_1	`35.27% <26.66%> (-0.01%)`	⬇️
Windows_2	`55.26% <94.11%> (+0.05%)`	⬆️
Windows_3	`43.83% <26.66%> (-0.02%)`	⬇️
Windows_4	`35.35% <26.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kavilla · 2023-09-26T11:43:43Z

src/core/types/workspace.ts

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export interface WorkspaceAttribute {


if this a core type would be able to have a fast follow to use this one within the workspace plugin?

Yes, this type WorkspaceAttribute is the same as the one used in #5075 and this core type will be used by the plugin, is this what you are referring?

Gotcha! So it's here for the sake of breaking down the PRs. Thanks!

kavilla

Thanks for doing this! People do love extending core stuff.

Would you potentially be able to add inline commenting could be like something like this:
opensearch-project/opensearch-dashboards-sdk-js#47

which produced https://github.com/kavilla/opensearch-dashboards-sdk-js/blob/avillk/sdk_base_2_no_doc/docs/modules.md. That way, in the future if we have to we can run a skip to dump documentation since usually the hardest part about the extending core functionality for plugins usually tends to be lack of knowing the usages and what to respect.

kavilla · 2023-09-26T12:16:04Z

src/core/public/workspace/workspaces_service.ts

+  currentWorkspaceId$: BehaviorSubject<string>;
+  currentWorkspace$: BehaviorSubject<WorkspaceObject | null>;
+  workspaceList$: BehaviorSubject<WorkspaceObject[]>;
+  workspaceEnabled$: BehaviorSubject<boolean>;


I wonder if this is redundant? Since a plugin being disabled is kind of a before run time operation.

When setting this up as well, the workspaces plugin can set the value for /api/core/capabilities and plugins can check it is available to use
http://localhost:5609/vzu/api/core/capabilities

That's a very good point. I will remove this and update to use capabilities in the follow PRs

@kavilla Just to confirm that you mean we could use core.capabilities.registerProvider to register a flag when workspace plugin is loaded. Then we could read this at frontend via, for example application.capabilities.workspaces.loaded

Cool. I've fine with being part of a backlog but to be revisited prior to 2.11

cc: @manasvinibs (since she has worked a lot with it)

src/core/public/workspace/workspaces_service.test.ts

kavilla · 2023-09-28T07:30:12Z

I see the cypress tests failed I think it was cached will re-run it. But would you like me to run this against a branch of cypress tests within a fork of the ftrepo?

kavilla · 2023-09-28T07:35:52Z

Overall, I think this fine. It's only going into unreleased for now so if it's unused by core functionality and then probably better off to get it merged sooner than later and let it bake in main for a little before getting into 2.x.

Will check if the cypress tests are successful and also could we update any of the files such as: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/80758aae438560445f8a1f8e69008e1c90603c84/src/core/public/public.api.md if they exist and require such changes.

ruanyl · 2023-09-28T23:52:13Z

Would you potentially be able to add inline commenting could be like something like this:
opensearch-project/opensearch-dashboards-sdk-js#47

Hi @kavilla , thanks for suggesting this, I've added some comments, but I'm not sure if that meets the standards. Please take a look if you have time :)

Also could you help to rerun the bwc test? Error shows OSD did not load properly, looks like a temporary issue. And I checked the failed cypress tests as well, it looks not related to this PR.

ruanyl · 2023-09-28T23:56:12Z

also could we update any of the files such as: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/80758aae438560445f8a1f8e69008e1c90603c84/src/core/public/public.api.md if they exist and require such changes.

Will keep this in mind and update those accordingly when appropriate.

kavilla

Cypress tests are failing like other PRs. Known issue we are working on.

ruanyl · 2023-10-07T01:42:10Z

Hi @AMoo-Miki, could you take another look at the PR and see if everything looks good to you? Much appreciate!

joshuarrrr

A couple minor questions, but no large concerns. I would like to have in this PR or the related issue a summary of why this needs to be exposed as a core service rather than entirely encapsulated in a plugin.

joshuarrrr · 2023-10-12T18:48:34Z

src/core/types/workspace.ts

+  features?: string[];
+  color?: string;
+  icon?: string;
+  defaultVISTheme?: string;


Is there a design doc that explains this interface, and the use of this prop in particular?

Had a discussion with team, the original requirement was to support different theme on workspace level. But we're not going to support this in our initial release. I will remove defaultVISTheme for now to avoid confusion.

joshuarrrr · 2023-10-12T18:52:48Z

src/core/public/workspace/workspaces_service.ts

+   * The workspace that is derived from `currentWorkspaceId` and `workspaceList`, if
+   * `currentWorkspaceId` cannot be found from `workspaceList`, it will return an error
+   *
+   * This value MUST NOT set manually from outside of WorkspacesService


Is there any way to enforce this?

Yes, I think we can enforce it, that requires to refactor the current implementation a bit. Since this is the first PR, there will be other PRs will depends on this one, to avoid impacting others, what do you think if I create a task and come back to refactor this later?

We can definitely do it in a follow-up; the concern here is to make sure we don't expose any public interfaces that we don't intend to, because those will all be subject to semver once they go out in a release.

joshuarrrr · 2023-10-12T18:54:22Z

src/core/public/workspace/workspaces_service.ts

+   * The list of available workspaces. This workspace list should be set by whoever
+   * the workspace functionalities


"by whoever the workspace functionalities" - sorry, I'm not following this. Can we rephrase?

Nice catch! I think there was one line been removed accidentally, will fix this.

joshuarrrr · 2023-10-12T18:55:20Z

src/core/public/workspace/workspaces_service.ts

+}
+
+enum WORKSPACE_ERROR {
+  WORKSPACE_STALED = 'WORKSPACE_STALED',


Can we rename? Maybe WORKSPACE_IS_STALE

joshuarrrr · 2023-10-12T18:58:54Z

src/core/public/workspace/workspaces_service.ts

+           * Do a simple idempotent verification here
+           */
+          if (!isEqual(currentWorkspace, this.currentWorkspace$.getValue())) {
+            this.currentWorkspace$.next(currentWorkspace ?? null);


why do we want to use null here?

I think no particular reason, undefined also works, just null was used at the beginning :)

joshuarrrr · 2023-10-12T18:59:09Z

src/core/public/workspace/workspaces_service.ts

+
+          if (currentWorkspaceId && !currentWorkspace?.id) {
+            /**
+             * Current workspace is staled


Suggested change

* Current workspace is staled

* Current workspace is stale

joshuarrrr · 2023-10-12T19:03:16Z

src/core/public/index.ts

@@ -341,3 +347,5 @@ export {
 };

 export { __osdBootstrap__ } from './osd_bootstrap';
+
+export { WorkspacesStart, WorkspacesSetup, WorkspacesService } from './workspace';


Do we need to export WorkspacesService? Doesn't it just get accessed via the core lifecycle methods?

You're right, this is not necessary.

The core workspace module(WorkspaceService) is a foundational component that enables the implementation of workspace features within OSD plugins. The purpose of the core workspace module is to provide a framework for workspace implementations. This module does not implement specific workspace functionality but provides the essential infrastructure for plugins to extend and customize workspace features, it maintains a shared workspace state(observables) across the entire application to ensure a consistent and up-to-date view of workspace-related information to all parts of the application. --------- Signed-off-by: Yulong Ruan <[email protected]>

Signed-off-by: Yulong Ruan <[email protected]>

Co-authored-by: Miki <[email protected]> Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: Yulong Ruan <[email protected]>

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl · 2023-10-13T09:18:46Z

I would like to have in this PR or the related issue a summary of why this needs to be exposed as a core service rather than entirely encapsulated in a plugin.

@joshuarrrr Updated it in the PR description

zengyan-amazon · 2023-10-13T22:22:28Z

src/core/public/workspace/index.ts

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export { WorkspacesStart, WorkspacesService, WorkspacesSetup } from './workspaces_service';


shall we add a README.md to talk about some internals and usage of workspace service like https://github.com/opensearch-project/OpenSearch-Dashboards/tree/main/src/core/public/chrome

Good point, we have a separate PR that adds the documentation of workspace feature design and some implementation details #5282, I can add the one more section there about the WorkspacesService

zengyan-amazon · 2023-10-13T22:26:56Z

src/core/public/workspace/workspaces_service.ts

+  currentWorkspaceId$: BehaviorSubject<string>;
+
+  /**
+   * The workspace that is derived from `currentWorkspaceId` and `workspaceList`, if
+   * `currentWorkspaceId` cannot be found from `workspaceList`, it will return an error
+   *
+   * This value MUST NOT set manually from outside of WorkspacesService
+   */
+  currentWorkspace$: BehaviorSubject<WorkspaceObject | null>;


doesn't the currentWorkspace object include workspace id that we need to have another observable for workspace id?

Right, ideally having currentWorkspaceId and workspaceList is good enough, because currentWorkspace can be derived from currentWorkspaceId and workspaceList.

However, in the actual code, the places that SET the current workspace normally only have the id and the places that GET the current workspace normally will need the entire workspace object.

The currentWorkspace$ is more like a helper, its computed from currentWorkspaceId$ and workspaceList$ once they changed.

zengyan-amazon · 2023-10-13T22:27:42Z

src/core/public/workspace/workspaces_service.ts

+   * This is a flag which indicates the WorkspacesService module is initialized and ready
+   * for consuming by others. For example, the `workspaceList` has been set, etc
+   */
+  initialized$: BehaviorSubject<boolean>;


this can only go from not initialized to initialized, does it needs to be an observable?

The intention of making it reactive is, the value changing from not initialized to initialized maybe happen at any time, this is unlike reading regular configurations which we get value when application is mount. Using an observable so that the application can react to the value change.

zengyan-amazon · 2023-10-13T22:45:55Z

do we want to save the selected workspace in browser localStorage so that it can be remembered for next login?

ruanyl · 2023-10-16T01:35:19Z

do we want to save the selected workspace in browser localStorage so that it can be remembered for next login?

@zengyan-amazon Sounds good to me, could be an improvement task, I will create a task for this and discuss it with the team and UX.

* feat: add core workspace module (#145) The core workspace module(WorkspaceService) is a foundational component that enables the implementation of workspace features within OSD plugins. The purpose of the core workspace module is to provide a framework for workspace implementations. This module does not implement specific workspace functionality but provides the essential infrastructure for plugins to extend and customize workspace features, it maintains a shared workspace state(observables) across the entire application to ensure a consistent and up-to-date view of workspace-related information to all parts of the application. Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: Yulong Ruan <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit 9e3e3a7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* [Workspace] Feature/workspace service core module (#5092) * feat: add core workspace module (#145) The core workspace module(WorkspaceService) is a foundational component that enables the implementation of workspace features within OSD plugins. The purpose of the core workspace module is to provide a framework for workspace implementations. This module does not implement specific workspace functionality but provides the essential infrastructure for plugins to extend and customize workspace features, it maintains a shared workspace state(observables) across the entire application to ensure a consistent and up-to-date view of workspace-related information to all parts of the application. Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: Yulong Ruan <[email protected]> Co-authored-by: Miki <[email protected]> (cherry picked from commit 9e3e3a7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> --------- Signed-off-by: Anan Zhuang <[email protected]> Signed-off-by: Miki <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Anan Zhuang <[email protected]> Co-authored-by: Miki <[email protected]>

ruanyl requested review from ananzh, kavilla, seanneumann, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, kristenTian, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, BSFishy and curq as code owners September 22, 2023 14:25

github-actions bot added the first-time-contributor label Sep 22, 2023

ruanyl changed the title ~~Feature/workspace service core module~~ [Workspace] Feature/workspace service core module Sep 26, 2023

kavilla added the v2.11.0 label Sep 26, 2023

kavilla reviewed Sep 26, 2023

View reviewed changes

AMoo-Miki reviewed Sep 27, 2023

View reviewed changes

src/core/public/workspace/workspaces_service.test.ts Outdated Show resolved Hide resolved

kavilla added the workspace label Sep 28, 2023

ruanyl force-pushed the feature/workspace-service-core-module branch from 80758aa to 59b0a3e Compare September 28, 2023 08:17

kavilla previously approved these changes Sep 30, 2023

View reviewed changes

joshuarrrr reviewed Oct 12, 2023

View reviewed changes

ruanyl and others added 11 commits October 13, 2023 16:39

add unit tests for workspace core service (#191)

e7552b6

Signed-off-by: Yulong Ruan <[email protected]>

add missing type definition

7a1df82

Signed-off-by: Yulong Ruan <[email protected]>

add changelog for workspace service module

65aa8a1

Signed-off-by: Yulong Ruan <[email protected]>

remove unnecessary workspace menu register

a8e9582

Signed-off-by: Yulong Ruan <[email protected]>

Update test description per comment

2ee8939

Co-authored-by: Miki <[email protected]> Signed-off-by: Yulong Ruan <[email protected]> Signed-off-by: Yulong Ruan <[email protected]>

remove unnecessary workspace enabled flag from core workspace module

60ab8c8

Signed-off-by: Yulong Ruan <[email protected]>

fix tests and add comments

55c4b08

Signed-off-by: Yulong Ruan <[email protected]>

update failed snapshot

f4cf5a3

Signed-off-by: Yulong Ruan <[email protected]>

update failed snapshots

018395f

Signed-off-by: Yulong Ruan <[email protected]>

update per PR comments

a21adee

Signed-off-by: Yulong Ruan <[email protected]>

ruanyl dismissed kavilla’s stale review via a21adee October 13, 2023 08:40

ruanyl force-pushed the feature/workspace-service-core-module branch from b6dff78 to a21adee Compare October 13, 2023 08:40

zengyan-amazon reviewed Oct 13, 2023

View reviewed changes

joshuarrrr approved these changes Oct 17, 2023

View reviewed changes

kavilla approved these changes Oct 17, 2023

View reviewed changes

kavilla merged commit 9e3e3a7 into opensearch-project:main Oct 17, 2023
57 checks passed

zengyan-amazon mentioned this pull request Oct 17, 2023

[Workspace][Feature]Setup workspace skeleton and implement basic CRUD API #5075

Merged

8 tasks

kavilla added backport 2.x v2.12.0 labels Nov 1, 2023

opensearch-trigger-bot bot mentioned this pull request Nov 1, 2023

[Backport 2.x] [Workspace] Feature/workspace service core module #5416

Merged

ruanyl mentioned this pull request Mar 15, 2024

[Workspace] Filter left nav menu according to the current workspace #6153

Closed

AMoo-Miki added the in-next label Sep 27, 2024

		* The list of available workspaces. This workspace list should be set by whoever
		* the workspace functionalities

[Workspace] Feature/workspace service core module #5092

[Workspace] Feature/workspace service core module #5092

Conversation

ruanyl commented Sep 22, 2023 • edited Loading

Description

Why introducing these changes to core?

Issues Resolved

Screenshot

Testing the changes

Check List

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla commented Sep 28, 2023 • edited Loading

kavilla commented Sep 28, 2023

ruanyl commented Sep 28, 2023

ruanyl commented Sep 28, 2023

kavilla left a comment

Choose a reason for hiding this comment

ruanyl commented Oct 7, 2023

joshuarrrr 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

ruanyl commented Oct 13, 2023

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

zengyan-amazon commented Oct 13, 2023

ruanyl commented Oct 16, 2023

ruanyl commented Sep 22, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading

ruanyl Sep 27, 2023 •

edited

Loading

kavilla commented Sep 28, 2023 •

edited

Loading