[Workspace] [Data Source] feat: support workspace level default data source #7188

raintygao · 2024-07-08T05:01:15Z

Description

Support set default data source in workspace

Recover data source management when in workspace
Restrict workspace level default data source ui settings value extend from global when getting
Set default data source when creating work space and add check when editing workspace

Screenshot

ds.mp4

Testing the changes

Go to home and set the default data source
Enter a workspace and. go to data source management, the default data source would not be extended.
Create workspace and assign some data sources, go to data source management and you could see a default data source would be set.
Edit workspace and assign/ unassign data sources, if default data source is unassigned, another default data source would be set.

Changelog

feat: support workspace level default data source

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2024-07-08T05:15:45Z

Codecov Report

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

Project coverage is 67.63%. Comparing base (9893ce1) to head (f8dbb50).
Report is 329 commits behind head on main.

Files with missing lines	Patch %	Lines
src/plugins/workspace/server/utils.ts	87.50%	0 Missing and 1 partial ⚠️
src/plugins/workspace/server/workspace_client.ts	92.30%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7188   +/-   ##
=======================================
  Coverage   67.62%   67.63%           
=======================================
  Files        3471     3471           
  Lines       68613    68636   +23     
  Branches    11165    11171    +6     
=======================================
+ Hits        46399    46419   +20     
- Misses      19510    19511    +1     
- Partials     2704     2706    +2

Flag	Coverage Δ
Linux_1	`33.30% <92.00%> (+0.03%)`	⬆️
Linux_2	`55.30% <ø> (ø)`
Linux_3	`45.26% <100.00%> (-0.02%)`	⬇️
Linux_4	`34.69% <100.00%> (ø)`
Windows_1	`33.32% <92.00%> (+0.03%)`	⬆️
Windows_2	`55.25% <ø> (ø)`
Windows_3	`45.27% <100.00%> (ø)`
Windows_4	`34.69% <100.00%> (ø)`

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.

SuZhou-Joe · 2024-07-08T05:23:17Z

src/plugins/workspace/server/saved_objects/workspace_data_source_operation_wrapper.ts

+
+  public wrapperFactory: SavedObjectsClientWrapperFactory = (wrapperOptions) => {
+    const isDashboardAdmin = getWorkspaceState(wrapperOptions.request)?.isDashboardAdmin;
+    const isInWorkspace = getWorkspaceState(wrapperOptions.request)?.requestWorkspaceId;


What if the workspace is inside the options? I think this wrapper should be put behind id consumer and only consume the workspaces inside options.

I would prefer to use InWorkspace because what we want to limit is that user operations in the DSM inside workspace, it would look more directly. And this has more priority because id consumer would have some parse logic but this wrapper would limit directly based on role and operations, what do you think?

I would say: the logic to get workspaces should be unified. I do not see there is any special use case for this wrapper to get workspaceId directly from workspaceState.

Imaging that a user calls

/w/${workspaceId}/createObject, { options: { workspaces: null } }

should we consider it isInWorkspace or outOfWorkspace? All of other wrappers consider it as createObject outside workspace, only this wrapper will consider it as inWorkspace.

Agree with @SuZhou-Joe, options.workspaces should be the source of truth, that's actually the value we are using to call saved object client to create the object.

@raintygao Shall we restrict these data source operations when out of workspace? These client wrapper is for API level right? User can execute CUD operations if this API call doesn't include workspace info.

Thanks @SuZhou-Joe @ruanyl , that makes sense, I would try to do some updates and take a look.

@wanglam This PR is only aiming to limit data source operations in workspace, you could take a look at description of this PR.

@raintygao What about creating a separate PR for the wrapper? It seems it's not related to default data source setting. This way, we can get default data source setting reviewed asap.

That sounds good. I will do a separation.

@ruanyl @SuZhou-Joe I have separated wrapper, there is no unresolved comment about default DS, could we merge it?

SuZhou-Joe · 2024-07-08T05:28:32Z

src/plugins/workspace/server/utils.ts

@@ -113,3 +115,23 @@ export const getDataSourcesList = (client: SavedObjectsClientContract, workspace
      }
    });
 };
+
+export const checkAndSetDefaultDataSources = async (


Suggested change

export const checkAndSetDefaultDataSources = async (

export const checkAndSetDefaultDataSource = async (

defaultDataSource should be singular I suppose.

And I guess there are some similar logic in DSM that sets default data source, can we try reuse that?

src/plugins/workspace/server/workspace_client.ts

SuZhou-Joe · 2024-07-08T05:37:48Z

src/plugins/workspace/common/constants.ts

@@ -32,10 +33,12 @@ export const WORKSPACE_ID_CONSUMER_WRAPPER_ID = 'workspace_id_consumer';

 /**
 * The priority for these wrappers matters:
- * 1. WORKSPACE_ID_CONSUMER wrapper should be the first wrapper to execute, as it will add the `workspaces` field
+ * 1. WORKSPACE_DATA_SOURCE_OPERATION_WRAPPER could be the first wrapper to execute as it is an independent wrapper and not related to other wrappers.


The wrapper should sit behind id_consumer wrapper I think.

ruanyl · 2024-07-08T06:06:52Z

src/plugins/workspace/server/utils.ts

+      const isDefaultDSExist = dataSources.indexOf(defaultDSId) > -1;
+      if (!isDefaultDSExist) {


Nit:

Suggested change

const isDefaultDSExist = dataSources.indexOf(defaultDSId) > -1;

if (!isDefaultDSExist) {

if (!dataSources.includes(defaultDSId)) {

ruanyl · 2024-07-08T06:21:00Z

src/plugins/workspace/server/saved_objects/workspace_data_source_operation_wrapper.ts

+
+  public wrapperFactory: SavedObjectsClientWrapperFactory = (wrapperOptions) => {
+    const isDashboardAdmin = getWorkspaceState(wrapperOptions.request)?.isDashboardAdmin;
+    const isInWorkspace = getWorkspaceState(wrapperOptions.request)?.requestWorkspaceId;


Agree with @SuZhou-Joe, options.workspaces should be the source of truth, that's actually the value we are using to call saved object client to create the object.

wanglam · 2024-07-08T06:42:36Z

src/plugins/workspace/server/utils.ts

+export const checkAndSetDefaultDataSources = async (
+  uiSettingsClient: IUiSettingsClient,
+  dataSources: string[],
+  isNeededCheck: boolean


Shall we separate the checkAndSetDefaultDataSources to checkAndSetDefaultDataSource and setDefaultDataSource? The method name with a isNeededCheck flag looks a little bit confused to me.

Im open to this, but I think we could combine this logic together with some comments.

We'd better not call different methods in create and update.

Call with different methods is make sense to me.
For new creating workspace, we don't need to check if default data source has been set. We can just call setDefaultDataSource directly.
For new updating workspace, we always check if the default data source has been set. We will call setDefaultDataSources if default data source not set.
BTW, There are two cases about data source been deleted:

All data source has been unassigned during workspace update

The default data source has been deleted out of workspace.

Will current implementation handle these two cases? Do we need to clear the default data source after data source been deleted?

We have judged the length of array, we will only set when more than 0 new data sources.

Yeah. But we won't clear the old default data source if the length of array equal zero, right? For example, create workspace with 3 datasources. The default data source will be the data source 1. And then remove all the data sources in the workspace update page. Will the default data source be cleared? Is there any issues if the default data source still data source 1?

Could you list any issues if existing? Then we could do a research.

My comments just for pointing out the potential technical issues. Store a expired default data source id is the root cause. From my point of view, if any place read the default data source id directly. It would be a problem(For example the dashboards assistant plugin.). I'm not sure if there are any other issues. You can resolve my comments if there are not any issues after your investigations.

Thanks for your comments,that would be more safe if we do a clear operation if there is no data source left. I would add this.

Signed-off-by: tygao <tygao@amazon.com>

Signed-off-by: Tianyu Gao <tygao@amazon.com>

opensearch-trigger-bot · 2024-07-16T11:30:11Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7188-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e1d50967b2a707107240b49634fb00e4b919152e
# Push it to GitHub
git push --set-upstream origin backport/backport-7188-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7188-to-2.x.

opensearch-trigger-bot · 2024-07-16T13:50:17Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7188-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e1d50967b2a707107240b49634fb00e4b919152e
# Push it to GitHub
git push --set-upstream origin backport/backport-7188-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7188-to-2.x.

opensearch-trigger-bot · 2024-07-17T06:08:28Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7188-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e1d50967b2a707107240b49634fb00e4b919152e
# Push it to GitHub
git push --set-upstream origin backport/backport-7188-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7188-to-2.x.

…source (opensearch-project#7188) * feat: support set default ds in workspace Signed-off-by: tygao <tygao@amazon.com> * add updateWorkspaceState Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR opensearch-project#7188 created/updated * update workspace plugin test Signed-off-by: tygao <tygao@amazon.com> * add reset workspace state and update tests Signed-off-by: tygao <tygao@amazon.com> * update and add tests for workspace client Signed-off-by: tygao <tygao@amazon.com> * clear default data source if no data source left and seperate wrapper Signed-off-by: tygao <tygao@amazon.com> * add try catch and add tests Signed-off-by: tygao <tygao@amazon.com> * disable DSM in workspace Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Signed-off-by: Tianyu Gao <tygao@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Yulong Ruan <ruanyl@amazon.com> (cherry picked from commit e1d5096)

…source (#7188) (#7281) * feat: support set default ds in workspace Signed-off-by: tygao <tygao@amazon.com> * add updateWorkspaceState Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR #7188 created/updated * update workspace plugin test Signed-off-by: tygao <tygao@amazon.com> * add reset workspace state and update tests Signed-off-by: tygao <tygao@amazon.com> * update and add tests for workspace client Signed-off-by: tygao <tygao@amazon.com> * clear default data source if no data source left and seperate wrapper Signed-off-by: tygao <tygao@amazon.com> * add try catch and add tests Signed-off-by: tygao <tygao@amazon.com> * disable DSM in workspace Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Signed-off-by: Tianyu Gao <tygao@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Yulong Ruan <ruanyl@amazon.com> (cherry picked from commit e1d5096)

raintygao requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi, mengweieric and LDrago27 as code owners July 8, 2024 05:01

github-actions bot added the seasoned-contributor label Jul 8, 2024

raintygao force-pushed the feat-default-ds branch from 9f9ee33 to a1dc92b Compare July 8, 2024 05:01

opensearch-changeset-bot bot added a commit to raintygao/OpenSearch-Dashboards that referenced this pull request Jul 8, 2024

Changeset file for PR opensearch-project#7188 created/updated

3d7a31d

SuZhou-Joe reviewed Jul 8, 2024

View reviewed changes

src/plugins/workspace/server/workspace_client.ts Show resolved Hide resolved

SuZhou-Joe reviewed Jul 8, 2024

View reviewed changes

ruanyl reviewed Jul 8, 2024

View reviewed changes

wanglam reviewed Jul 8, 2024

View reviewed changes

raintygao added 3 commits July 9, 2024 14:08

update and add tests for workspace client

40d39c5

Signed-off-by: tygao <tygao@amazon.com>

clear default data source if no data source left and seperate wrapper

815d61c

Signed-off-by: tygao <tygao@amazon.com>

add try catch and add tests

7ade58e

Signed-off-by: tygao <tygao@amazon.com>

raintygao force-pushed the feat-default-ds branch from 641b9b1 to 7ade58e Compare July 9, 2024 06:08

disable DSM in workspace

65f09a2

Signed-off-by: tygao <tygao@amazon.com>

SuZhou-Joe previously approved these changes Jul 11, 2024

View reviewed changes

ruanyl added the workspace label Jul 16, 2024

ruanyl previously approved these changes Jul 16, 2024

View reviewed changes

Merge branch 'main' into feat-default-ds

c533ba8

Signed-off-by: Tianyu Gao <tygao@amazon.com>

raintygao dismissed stale reviews from ruanyl and SuZhou-Joe via c533ba8 July 16, 2024 05:17

Merge branch 'main' into feat-default-ds

f8dbb50

SuZhou-Joe approved these changes Jul 16, 2024

View reviewed changes

ruanyl approved these changes Jul 16, 2024

View reviewed changes

SuZhou-Joe added backport 2.x v2.16.0 labels Jul 16, 2024

SuZhou-Joe merged commit e1d5096 into opensearch-project:main Jul 16, 2024
69 of 71 checks passed

opensearch-trigger-bot bot added the failed backport label Jul 16, 2024

SuZhou-Joe added backport 2.x and removed backport 2.x labels Jul 16, 2024

SuZhou-Joe added backport 2.x and removed backport 2.x labels Jul 17, 2024

raintygao mentioned this pull request Jul 17, 2024

[Manual Backport][2.x][Workspace] [Data Source] feat: support workspace level default data source #7281

Merged

7 tasks

AMoo-Miki removed the failed backport label Aug 22, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workspace] [Data Source] feat: support workspace level default data source #7188

[Workspace] [Data Source] feat: support workspace level default data source #7188

raintygao commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

SuZhou-Joe Jul 8, 2024

raintygao Jul 8, 2024

SuZhou-Joe Jul 8, 2024 •

edited

Loading

ruanyl Jul 8, 2024

wanglam Jul 8, 2024

raintygao Jul 8, 2024

raintygao Jul 8, 2024

ruanyl Jul 8, 2024

raintygao Jul 8, 2024

raintygao Jul 8, 2024

SuZhou-Joe Jul 8, 2024

SuZhou-Joe Jul 8, 2024

ruanyl Jul 8, 2024

ruanyl Jul 8, 2024

wanglam Jul 8, 2024

raintygao Jul 8, 2024

raintygao Jul 8, 2024

wanglam Jul 8, 2024 •

edited

Loading

raintygao Jul 8, 2024 •

edited

Loading

wanglam Jul 8, 2024

raintygao Jul 8, 2024 •

edited

Loading

wanglam Jul 8, 2024 •

edited

Loading

raintygao Jul 8, 2024

opensearch-trigger-bot bot commented Jul 16, 2024

opensearch-trigger-bot bot commented Jul 16, 2024

opensearch-trigger-bot bot commented Jul 17, 2024

	export const checkAndSetDefaultDataSources = async (
	export const checkAndSetDefaultDataSource = async (

		const isDefaultDSExist = dataSources.indexOf(defaultDSId) > -1;
		if (!isDefaultDSExist) {

	const isDefaultDSExist = dataSources.indexOf(defaultDSId) > -1;
	if (!isDefaultDSExist) {
	if (!dataSources.includes(defaultDSId)) {

[Workspace] [Data Source] feat: support workspace level default data source #7188

[Workspace] [Data Source] feat: support workspace level default data source #7188

Conversation

raintygao commented Jul 8, 2024 • edited Loading

Description

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Jul 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Jul 8, 2024 • edited Loading

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

wanglam Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

raintygao Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raintygao Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

wanglam Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jul 16, 2024

opensearch-trigger-bot bot commented Jul 16, 2024

opensearch-trigger-bot bot commented Jul 17, 2024

raintygao commented Jul 8, 2024 •

edited

Loading

codecov bot commented Jul 8, 2024 •

edited

Loading

SuZhou-Joe Jul 8, 2024 •

edited

Loading

wanglam Jul 8, 2024 •

edited

Loading

raintygao Jul 8, 2024 •

edited

Loading

raintygao Jul 8, 2024 •

edited

Loading

wanglam Jul 8, 2024 •

edited

Loading