-
Notifications
You must be signed in to change notification settings - Fork 867
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] feat: update workspace server to support data connection type #8200
[Workspace] feat: update workspace server to support data connection type #8200
Conversation
Signed-off-by: tygao <tygao@amazon.com>
5421ae6
to
aec3dec
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8200 +/- ##
=======================================
Coverage 64.04% 64.05%
=======================================
Files 3740 3741 +1
Lines 88608 88635 +27
Branches 13799 13803 +4
=======================================
+ Hits 56746 56772 +26
- Misses 31264 31265 +1
Partials 598 598
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: tygao <tygao@amazon.com>
@@ -6,5 +6,5 @@ | |||
"ui": true, | |||
"requiredPlugins": ["opensearchDashboardsUtils"], | |||
"optionalPlugins": [], | |||
"extraPublicDirs": ["common/data_sources", "common/util"] | |||
"extraPublicDirs": ["common","common/data_sources", "common/util"] |
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.
Seems like a new pattern to me, so it will bypass the lint check?
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.
I suppose would, I checked some other usage before modified.
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/opensearch_dashboards.json
https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/expressions/opensearch_dashboards.json
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.
LGTM, so the permission on the data connections will be raised in another PR?
Yes, this will be addressed and included in later PRs. |
if (newDataConnections) { | ||
const originalSelectedDataConnectionIds = originalSelectedDataSourcesAndConnections | ||
.filter((item) => item.type === DATA_CONNECTION_SAVED_OBJECT_TYPE) | ||
.map((ds) => ds.id); | ||
const dataConnectionsToBeRemoved = originalSelectedDataConnectionIds.filter( | ||
(ds) => !newDataConnections.find((item) => item === ds) | ||
); | ||
const dataConnectionsToBeAdded = newDataConnections.filter( | ||
(ds) => !originalSelectedDataConnectionIds.find((item) => item === ds) | ||
); | ||
if (dataConnectionsToBeRemoved.length > 0) { | ||
for (const dataConnectionId of dataConnectionsToBeRemoved) { | ||
promises.push( | ||
client.deleteFromWorkspaces(DATA_CONNECTION_SAVED_OBJECT_TYPE, dataConnectionId, [id]) | ||
); | ||
} | ||
} | ||
if (dataConnectionsToBeAdded.length > 0) { | ||
for (const dataConnectionId of dataConnectionsToBeAdded) { | ||
promises.push( | ||
client.addToWorkspaces(DATA_CONNECTION_SAVED_OBJECT_TYPE, dataConnectionId, [id]) | ||
); | ||
} | ||
} | ||
} |
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: seems like the logic to handle the data connection are the same as data source, would it be possible to make it into a function like addOperationsToPromisesList(type: string)
?
So the code here will be like
const promises = [];
const addOperationsToPromisesList = (type: string) => { xxx }
// Add data sources operations into promise
addOperationsToPromisesList(DATA_SOURCE_SAVED_OBJECT_TYPE);
// Add data connections operations into promise
addOperationsToPromisesList(DATA_CONNECTION_SAVED_OBJECT_TYPE);
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.
Sounds good, will optimize this part in #8013.
@@ -204,6 +204,7 @@ export class WorkspaceClient implements IWorkspaceClient { | |||
settings: { | |||
dataSources?: string[]; | |||
permissions?: SavedObjectPermissions; | |||
dataConnections?: string[]; |
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.
Why not reuse dataSources
? It doesn't seem necessary to add an extra parameter dataConnections
. Because for workspace, they are all data sources, I personally don't want it to be implemented in a way that every time adding new type of data source, it requires to change the API interface.
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.
what's passed in here is just saved object ids, we could handle different type of data source internally.
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, if we reuse dataSources field, we need to update this field from [‘id1’,‘id2’] to [{type:‘data-source’,id:‘id1’},{type:‘data-connection’,id:‘id2’], which would make a breaking change to existing API.
…type (#8200) * feat: update worksapce server to support data connection type Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR #8200 created/updated * test: add tests for wrapper 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 f338aff) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…type (#8200) (#8203) * feat: update worksapce server to support data connection type * Changeset file for PR #8200 created/updated * test: add tests for wrapper --------- (cherry picked from commit f338aff) 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
Update workspace server to support data connection type
This is a PR picked from #8013, as we want to separate frontend and server changes and merge server changes first for testing, so we extract server changes from #8013 and create this PR.
Testing the changes
Create workspace
post http://localhost:6601/api/workspaces
Result
Update workspace
put http://localhost:6601/api/_IlxNR
Result
Verify
get data-connection objects with workspace field
http://localhost:6601/api/saved_objects/_find?fields=id&fields=title&fields=workspaces&per_page=10000&type=data-connection&workspaces=*
Result
Changelog
Check List
yarn test:jest
yarn test:jest_integration