-
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]Import sample data to current workspace #6105
[Workspace]Import sample data to current workspace #6105
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6105 +/- ##
==========================================
- Coverage 67.55% 67.37% -0.19%
==========================================
Files 3428 3443 +15
Lines 67344 67762 +418
Branches 10996 11024 +28
==========================================
+ Hits 45497 45653 +156
- Misses 19177 19439 +262
Partials 2670 2670
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: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
dec0efb
to
54adb59
Compare
@@ -81,7 +81,10 @@ export class SampleDataSetCards extends React.Component { | |||
loadSampleDataSets = async (dataSourceId) => { |
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.
It seems we are adding workspaceId into the parameters of listSampleDataSets/install/uninstall but not in loadSampleDataSets, any reason for such difference?
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.
The loadSampleDataSets
function call listSampleDataSets
below. It will read the current workspace id from the workspace service directly. So we don't add the parameter for workspace id.
getDataSourceIntegratedSavedObjects: (dataSourceId?: string, dataSourceTitle?: string) => | ||
getSavedObjectsWithDataSource(getSavedObjects(), dataSourceId, dataSourceTitle), |
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.
Is it save to delete this field?
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.
This method has been moved to src/plugins/home/server/services/sample_data/data_sets/util.ts
, searching all OSD, no other places use it.
@@ -5,59 +5,74 @@ | |||
|
|||
import { SavedObject } from 'opensearch-dashboards/server'; | |||
|
|||
export const appendDataSourceId = (id: 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.
Seems this logic has gone?
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. This function will be refactor to addPrefixTo
in below.
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.
Any updates on this comment?
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.
The appendDataSourceId
has been added back. To support workspace, the workspaceId has been added to the appendDataSourceId
result function. It will add workspace id prefix at the return value of getDataSourceIntegratedDashboard
and getDataSourceIntegratedDefaultIndex
functions.
…ta-to-workspace Signed-off-by: Lin Wang <wonglam@amazon.com>
</div> | ||
`); | ||
}); | ||
it('should call unlisten history after destroy', async () => { |
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.
maybe like "should clean up history listeners after unmount" ?
getFinalSavedObjects({ | ||
dataset, | ||
}) | ||
).toBe(dataset.savedObjects); |
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.
should be "toEqual" here too, so can performs a deep equality 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.
Use toBe here means we just want to use the original saved objects here. I think toBe would be more strict than toEqual. toBe is for compare object reference, they are the same object if reference is same.
Is it
|
@wanglam do you have a chance to verify the all different cases for import sample data. And how to work with MDS to add sample data too
Thanks |
Signed-off-by: Lin Wang <wonglam@amazon.com>
Sure. I think these three cases would work fine. I will test them. For add user to created workspace, you can follow the test steps of this PR. First of all we need to add For update existing workspace, we can go to the workspace list page. Then click the pencil icon, we will navigate to the workspace update page. Click the |
// dispatch synthetic hash change event to update hash history objects | ||
// this is necessary because hash updates triggered by using popState won't trigger this event naturally. | ||
// This must be called before the app is mounted to avoid call this after the redirect to default app logic kicks in | ||
const unlisten = history.listen((location) => { |
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.
Do we need this for import sample data app? It seems it's home page app specific.
Signed-off-by: Lin Wang <wonglam@amazon.com>
Hi @Flyingliuhub , I've tested these three scenarios in my local. Here are all the findings:
|
<Router> | ||
<Switch> | ||
<Route | ||
path="*" |
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.
Q: why does it need a Route with path *
here? Seems we don't even need Router
here
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.
The renderTutorialDirectory
need props.match.params.tab
data and pass it to TutorialDirectory component. But all the tabs has been deleted in this PR. I think we can remove the Route here and pass a fixed SAMPLE_DATA_TAB_ID
here.
@@ -5,59 +5,74 @@ | |||
|
|||
import { SavedObject } from 'opensearch-dashboards/server'; | |||
|
|||
export const appendDataSourceId = (id: 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.
Any updates on this comment?
Signed-off-by: Lin Wang <wonglam@amazon.com>
* Import sample data to workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * Enable workspace ui plugin Signed-off-by: Lin Wang <wonglam@amazon.com> * Add changelog for import sample data to current workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * feat: register sample data as standalone app (#8) * feat: register sample data as standalone app Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: use props to pass homeLink Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Retrieve workspace id from request Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove workspace id in query Signed-off-by: Lin Wang <wonglam@amazon.com> * Move changelog to fragments Signed-off-by: Lin Wang <wonglam@amazon.com> * Fix sample data list unit tests Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove no need workspaces deps Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove manual created changelogs Signed-off-by: Lin Wang <wonglam@amazon.com> * Changeset file for PR #6105 created/updated * Enable sample data in workspace overview page (#9) * enable sample data in workspace overview page Signed-off-by: Hailong Cui <ihailong@amazon.com> * add comments for empty id Signed-off-by: Hailong Cui <ihailong@amazon.com> --------- Signed-off-by: Hailong Cui <ihailong@amazon.com> * Add unit tests for getFinalSavedObjects in data sets util file Signed-off-by: Lin Wang <wonglam@amazon.com> * Add unit tests for renderImportSampleDataApp destroy Signed-off-by: Lin Wang <wonglam@amazon.com> * Address PR comments Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove history listen in renderImportSampleDataApp Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove Route for workspace import sample data entry point Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Hailong Cui <ihailong@amazon.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Hailong Cui <ihailong@amazon.com> (cherry picked from commit 3e9a159) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Import sample data to workspace * Enable workspace ui plugin * Add changelog for import sample data to current workspace * feat: register sample data as standalone app (#8) * feat: register sample data as standalone app * feat: optimize code * feat: add comment * feat: use props to pass homeLink * feat: add unit test --------- * Retrieve workspace id from request * Remove workspace id in query * Move changelog to fragments * Fix sample data list unit tests * Remove no need workspaces deps * Remove manual created changelogs * Changeset file for PR #6105 created/updated * Enable sample data in workspace overview page (#9) * enable sample data in workspace overview page * add comments for empty id --------- * Add unit tests for getFinalSavedObjects in data sets util file * Add unit tests for renderImportSampleDataApp destroy * Address PR comments * Remove history listen in renderImportSampleDataApp * Remove Route for workspace import sample data entry point --------- (cherry picked from commit 3e9a159) Signed-off-by: Lin Wang <wonglam@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Hailong Cui <ihailong@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: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Hailong Cui <ihailong@amazon.com>
} | ||
|
||
// update reference | ||
if (saveObject.type === 'index-pattern') { |
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 removing the data source reference for index pattern?
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.
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 think that's removed by accident. Will add them back and add some unit tests.
…t#6105) * Import sample data to workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * Enable workspace ui plugin Signed-off-by: Lin Wang <wonglam@amazon.com> * Add changelog for import sample data to current workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * feat: register sample data as standalone app (opensearch-project#8) * feat: register sample data as standalone app Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: optimize code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add comment Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: use props to pass homeLink Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add unit test Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * Retrieve workspace id from request Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove workspace id in query Signed-off-by: Lin Wang <wonglam@amazon.com> * Move changelog to fragments Signed-off-by: Lin Wang <wonglam@amazon.com> * Fix sample data list unit tests Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove no need workspaces deps Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove manual created changelogs Signed-off-by: Lin Wang <wonglam@amazon.com> * Changeset file for PR opensearch-project#6105 created/updated * Enable sample data in workspace overview page (opensearch-project#9) * enable sample data in workspace overview page Signed-off-by: Hailong Cui <ihailong@amazon.com> * add comments for empty id Signed-off-by: Hailong Cui <ihailong@amazon.com> --------- Signed-off-by: Hailong Cui <ihailong@amazon.com> * Add unit tests for getFinalSavedObjects in data sets util file Signed-off-by: Lin Wang <wonglam@amazon.com> * Add unit tests for renderImportSampleDataApp destroy Signed-off-by: Lin Wang <wonglam@amazon.com> * Address PR comments Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove history listen in renderImportSampleDataApp Signed-off-by: Lin Wang <wonglam@amazon.com> * Remove Route for workspace import sample data entry point Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Signed-off-by: SuZhou-Joe <suzhou@amazon.com> Signed-off-by: Hailong Cui <ihailong@amazon.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: Hailong Cui <ihailong@amazon.com>
Description
For now, the import sample data page is a sub page under home application. The home application is not visible inside specific workspace. This PR separate import sample data page and register it as a new standalone application. Then import sample data page can be used inside workspace. This PR added a new entry point for import sample data in workspace overview page. User can visit import sample data page by this link quickly.
Issues Resolved
#6106
Screenshot
The sample data can be imported to specific workspace.
Testing the changes
Since switch workspace by manually wasn't support in current main branch. We need to apply some patch to testing all the changes.
yarn osd bootstrap
workspace.enabled: true
andsavedObjects.permission.enabled: true
to theconfig/opensearch_dashboards.yml
to enable workspace feature flagyarn start --no-base-path
start OSD server to make sure no random base path will be appendedNavigate to workspace creator page and create a test workspace
Click left dropdown switch to created workspace
Click "Discover pre-loaded datasets before adding your own." to visit workspace's Add sample data page
Add data
button, all sample data will be imported like below imageThere will be four dashboards in current workspace.
Click
View data
to view imported dashboard like below imageCreate another workspace, and visit another workspace's add sample data page like step 4 & 5.
The sample data was not import in the new workspace.
The
[Flights] Global Flight Dashboard
will be removed in this workspace like above image.Changelog
Check List
yarn test:jest
yarn test:jest_integration