-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Workspace][Data Source] Support data source assignment in workspace #7101
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7101 +/- ##
==========================================
+ Coverage 67.50% 67.56% +0.06%
==========================================
Files 3468 3469 +1
Lines 68402 68508 +106
Branches 11117 11141 +24
==========================================
+ Hits 46174 46287 +113
+ Misses 19532 19515 -17
- Partials 2696 2706 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/plugins/workspace/public/components/workspace_form/select_data_source_panel.tsx
Show resolved
Hide resolved
src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx
Show resolved
Hide resolved
permissions: { invalid_type: { users: ['foo'] } }, | ||
settings: { | ||
permissions: { invalid_type: { users: ['foo'] } }, | ||
dataSources: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Though workspace is an experimental feature, I still not recommend we do the breaking change in the API side. Maybe next time we can add a parameter.
@@ -110,6 +115,12 @@ export class WorkspaceClient implements IWorkspaceClientImpl { | |||
permissions, | |||
} | |||
); | |||
if (dataSources) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we assign dataSources before create
the workspace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can, also i'm thinking whether we should add a logic that call deleteFromWorkspaces if creation is failed, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be better, for now I think adjusting the order should be good enough.
@@ -192,6 +205,27 @@ export class WorkspaceClient implements IWorkspaceClientImpl { | |||
throw new Error(DUPLICATE_WORKSPACE_NAME_ERROR); | |||
} | |||
} | |||
|
|||
if (newDataSources) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write unit test to cover the diff check code below? It is kind of complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, currently there is not any tests for workspace client, maybe we need to do some work to fulfill workspace client test mock.
const object = await this.get<T>(type, id); | ||
const existingWorkspaces = object.workspaces ?? []; | ||
const mergedWorkspaces = existingWorkspaces.concat(workspaces); | ||
const nonDuplicatedWorkspaces = Array.from(new Set(mergedWorkspaces)); | ||
|
||
return await this.update<T>(type, id, object.attributes, { | ||
workspaces: nonDuplicatedWorkspaces, | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it is not an atomic operation, it may have race condition on high concurrency case. I would recommend we do a optimistic lock here by adding version into SavedObjectsUpdateOptions
return await this.update<T>(type, id, object.attributes, {
workspaces: nonDuplicatedWorkspaces,
version: object.version,
});
perPage: 10000, | ||
workspaces, | ||
}) | ||
.then((response) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we handle exception here? if user don't have permission to list all data sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In new design only OSD admin will call this function and there will be a new PR to return empty array if user doesn't have permission. So i think we don't need to handle exception here.
src/plugins/workspace/public/components/workspace_form/select_data_source_panel.tsx
Show resolved
Hide resolved
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
…7101) * feat: add data source assign Signed-off-by: tygao <tygao@amazon.com> * update workspace update Signed-off-by: tygao <tygao@amazon.com> * update workspace update test Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR #7101 created/updated * update name Signed-off-by: tygao <tygao@amazon.com> * update repository test Signed-off-by: tygao <tygao@amazon.com> * update integration test Signed-off-by: tygao <tygao@amazon.com> * update integration test Signed-off-by: tygao <tygao@amazon.com> * add test cases for utils Signed-off-by: tygao <tygao@amazon.com> * update utils test Signed-off-by: tygao <tygao@amazon.com> * add tests for saved object client Signed-off-by: tygao <tygao@amazon.com> * update and add tests for workspace client Signed-off-by: tygao <tygao@amazon.com> * use overwrite in deleteFromWorkspaces and update find Signed-off-by: tygao <tygao@amazon.com> * update saved ojects client test Signed-off-by: tygao <tygao@amazon.com> * use Promise.all and updates comment and type Signed-off-by: tygao <tygao@amazon.com> * add rescriction for title field to make it optional Signed-off-by: tygao <tygao@amazon.com> * use string array to send selected data sources Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 5a4aba9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…7101) (#7173) * feat: add data source assign * update workspace update * update workspace update test * Changeset file for PR #7101 created/updated * update name * update repository test * update integration test * update integration test * add test cases for utils * update utils test * add tests for saved object client * update and add tests for workspace client * use overwrite in deleteFromWorkspaces and update find * update saved ojects client test * use Promise.all and updates comment and type * add rescriction for title field to make it optional * use string array to send selected data sources --------- (cherry picked from commit 5a4aba9) Signed-off-by: tygao <tygao@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Screenshot
demo.mp4
Testing the changes
Use find API to find any data sources and what workspaces they belong to.
http://localhost:6601/w/${OSD_ID}/api/saved_objects/_find?fields=id&fields=title&fields=workspaces&per_page=10000&type=data-source&workspaces=*
Enter workspace create/update page to create workspaces and assign data sources or update and assign data sources.
Enter workspace update page or use find API to find that data source have been assigned to specific workspace.
http://localhost:6601/w/${OSD_ID}/api/saved_objects/_find?fields=id&fields=title&fields=workspaces&per_page=10000&type=data-source&workspaces=${WORKSPACE_ID}
Changelog
Check List
yarn test:jest
yarn test:jest_integration