Skip to content
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

Add datasource picker to import saved object flyout when multiple data source is enabled #5781

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

BionIT
Copy link
Collaborator

@BionIT BionIT commented Feb 2, 2024

Description

When multiple data source plugin is enabled, user can connect to remote data sources in addition to the local cluster, to support importing saved objects from remote data source via file upload(#5712), we need to have an option in the flyout for user to select where the file comes from.

Via discussion in the issue, we are altering the order of the import option, and have the Create new objects with random IDs as the default choice. When multiple data source is enabled, we will have the data source picker under Import options section then the Conflict management section will contain the options to address conflicts.

Issues Resolved

Fixes #5712

Screenshot

5717.mp4

Testing the changes

In the video above, we tested the following scenarios
when multiple data source is disabled:

  • upload same ndjson with Create new objects with unique IDs option is successful when there are duplicate objects exists
  • upload same ndjson with Automatically overwrite conflicts option is successful when there are duplicate objects
  • upload same ndjson with Request action on conflict option will prompt user to handle on the conflict
  • upload same json with Automatically overwrite conflicts option twice is successful when there are duplicate objects exists
  • upload same json with Request action on conflict option will prompt user to handle on the conflict
    when multiple data source is enabled:
  • upload ndjson with Create new objects with unique IDs option is successful
  • upload ndjson with Automatically overwrite conflicts option is successful when there are duplicate objects
  • upload same ndjson with Request action on conflict option will prompt user to handle on the conflict
  • upload same json with Automatically overwrite conflicts option twice is successful when there are duplicate objects exists
  • upload same json with Request action on conflict option will prompt user to handle on the conflict

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

Signed-off-by: Lu Yu <[email protected]>
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (7c0bd9f) 67.01% compared to head (d55ed3e) 66.97%.

Files Patch % Lines
...gement_section/objects_table/components/flyout.tsx 75.00% 3 Missing ⚠️
...c/components/cluster_selector/cluster_selector.tsx 60.00% 2 Missing ⚠️
...saved_objects_management/public/lib/import_file.ts 0.00% 2 Missing ⚠️
...cts_management/public/lib/resolve_import_errors.ts 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5781      +/-   ##
==========================================
- Coverage   67.01%   66.97%   -0.05%     
==========================================
  Files        3296     3301       +5     
  Lines       63370    63434      +64     
  Branches    10093    10104      +11     
==========================================
+ Hits        42465    42482      +17     
- Misses      18456    18501      +45     
- Partials     2449     2451       +2     
Flag Coverage Δ
Linux_1 35.23% <0.00%> (-0.01%) ⬇️
Linux_2 55.12% <ø> (ø)
Linux_3 43.93% <60.00%> (-0.01%) ⬇️
Linux_4 35.28% <60.71%> (-0.06%) ⬇️
Windows_1 35.25% <0.00%> (-0.01%) ⬇️
Windows_2 55.09% <ø> (ø)
Windows_3 43.93% <60.00%> (-0.01%) ⬇️
Windows_4 35.28% <60.71%> (-0.06%) ⬇️

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.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a datasource selector component in the data plugin. We should use that and not create a new picker.

@bandinib-amzn
Copy link
Member

bandinib-amzn commented Feb 2, 2024

I'm coming here after watching video attached in screenshot. Would it be possible to consider adding a checkbox or an alternative option for selecting 'overwrite all,' instead of having to click 'overwrite' 15 times? This could potentially enhance the user experience. cc: @kgcreative

@bandinib-amzn
Copy link
Member

We already have a datasource selector component in the data plugin. We should use that and not create a new picker.

Hi @ashwin-pc
I guess you are talking about this PR - #5167 and wondering why we don't use the same selector.

Let's consider the use case from above PR which is Datasource selector support in OpenSearch Dashboard Discover.
The Discover feature in OpenSearch Dashboards is designed for searching, exploring, and visualizing data stored in OpenSearch indices or Remote cluster indices.

The Dev Tool feature in OpenSearch Dashboard provides an environment for developers and administrators to interact with OpenSearch or Remote cluster with query DSL and API directly.

I took discover and dev tool here as an example to show the difference between two pickers here. They serve different purposes. #5167 has picker for selecting data source for searching, exploring data in the selected data source. Whereas data source picker in this PR is listing the data sources (basically remote cluster) to interact with them directly for fetch sample data, fetch indices to create index patterns etc.

IMO, they both serves different purpose, so I would prefer separate picker. @kgcreative I would like to here your thoughts as well.

@BionIT BionIT closed this Feb 2, 2024
@BionIT
Copy link
Collaborator Author

BionIT commented Feb 2, 2024

I'm coming here after watching video attached in screenshot. Would it be possible to consider adding a checkbox or an alternative option for selecting 'overwrite all,' instead of having to click 'overwrite' 15 times? This could potentially enhance the user experience. cc: @kgcreative

@bandinib-amzn The reason user has to act on the action multiple times is because Request action on conflict is selected. If user selects Automatically overwrite conflicts then all the conflict objects will be overwritten without any additional actions. This is the current behavior of the import function

@BionIT BionIT reopened this Feb 2, 2024
@kgcreative
Copy link
Member

kgcreative commented Feb 2, 2024

I'm coming here after watching video attached in screenshot. Would it be possible to consider adding a checkbox or an alternative option for selecting 'overwrite all,' instead of having to click 'overwrite' 15 times? This could potentially enhance the user experience. cc: @kgcreative

Yes, this is PAINFUL today when there are many objects that have conflicts, however, i'm hesitant to add to the scope unless the dev effort is reasonable. Ideally, we should show a selectable list, with easy "Select all/Deselect all/multiselect" options. Presumably each modal in the current experience triggers only when there is a conflict, so this would require a re-think of the flow.

@BionIT
Copy link
Collaborator Author

BionIT commented Feb 3, 2024

We already have a datasource selector component in the data plugin. We should use that and not create a new picker.

As discussed with @ashwin-pc and @mengweieric , we will switch to use the data source selector in 2.13 when it supports the feature requested in #5790

@BionIT BionIT requested a review from ashwin-pc February 3, 2024 00:10
@ashwin-pc
Copy link
Member

IMO, they both serves different purpose, so I would prefer separate picker. @kgcreative I would like to here your thoughts as well.

Thanks for that example. That was a very useful distinction! Then i think my concern is around the confusion in naming. We are calling this and the other component both datasource pickers but have very different uses for each. This one is used specifically to select an OpenSearch cluster while the other is to select what is in thes clusters but also data from other sources. Can we rename this to be a cluster selector instead to disambiguate the two?

@ashwin-pc ashwin-pc dismissed their stale review February 3, 2024 00:13

Clarified the distinction

@BionIT
Copy link
Collaborator Author

BionIT commented Feb 3, 2024

IMO, they both serves different purpose, so I would prefer separate picker. @kgcreative I would like to here your thoughts as well.

Thanks for that example. That was a very useful distinction! Then i think my concern is around the confusion in naming. We are calling this and the other component both datasource pickers but have very different uses for each. This one is used specifically to select an OpenSearch cluster while the other is to select what is in thes clusters but also data from other sources. Can we rename this to be a cluster selector instead to disambiguate the two?

@ashwin-pc I hear you, I don't have a strong opinion, since I will change to use the new data source selector in 2.13, should we go with this name for now? Happy to change it if we feel necessary

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add some tests for the code added here?

@@ -50,15 +50,22 @@ export function SavedObjectsPageProvider({ getService, getPageObjects }: FtrProv
await this.waitTableIsLoaded();
}

async importFile(path: string, overwriteAll = true) {
async importFile(path: string, overwriteAll = true, isLegacy = false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if we default this to true, you can avoid changes in the test files.

@ashwin-pc
Copy link
Member

ashwin-pc commented Feb 3, 2024

@ashwin-pc I hear you, I don't have a strong opinion, since I will change to use the new data source selector in 2.13, should we go with this name for now? Happy to change it if we feel necessary

based on @bandinib-amzn's reply i'm actually convinced that the two might not need to be the same thing if the only thing you are selecting is the cluster and not the index/indexPattern. But if thats true then we do need to rename this since the two wont be combined in 2.13 or beyond

@bandinib-amzn
Copy link
Member

bandinib-amzn commented Feb 3, 2024

e import function

Yes, I completely agree. It doesn't need to be within the scope of this PR. It's an enhancement that we can address in the future. I have created an issue #5794 to track.

@BionIT
Copy link
Collaborator Author

BionIT commented Feb 5, 2024

Can we also add some tests for the code added here?

Added tests in new commit

@ZilongX ZilongX merged commit 8e93c54 into opensearch-project:main Feb 5, 2024
81 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 5, 2024
…a source is enabled (#5781)

* add datasource picker to saved object management page when multiple data source is enabled

Signed-off-by: Lu Yu <[email protected]>

* add changelog

Signed-off-by: Lu Yu <[email protected]>

* change name to cluster selector and move to higher level

Signed-off-by: Lu Yu <[email protected]>

---------

Signed-off-by: Lu Yu <[email protected]>
(cherry picked from commit 8e93c54)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
yujin-emma pushed a commit to yujin-emma/OpenSearch-Dashboards that referenced this pull request Feb 5, 2024
…a source is enabled (opensearch-project#5781)

* add datasource picker to saved object management page when multiple data source is enabled

Signed-off-by: Lu Yu <[email protected]>

* add changelog

Signed-off-by: Lu Yu <[email protected]>

* change name to cluster selector and move to higher level

Signed-off-by: Lu Yu <[email protected]>

---------

Signed-off-by: Lu Yu <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
bandinib-amzn pushed a commit that referenced this pull request Feb 6, 2024
…a source is enabled (#5781)

* add datasource picker to saved object management page when multiple data source is enabled

Signed-off-by: Lu Yu <[email protected]>

* add changelog

Signed-off-by: Lu Yu <[email protected]>

* change name to cluster selector and move to higher level

Signed-off-by: Lu Yu <[email protected]>

---------

Signed-off-by: Lu Yu <[email protected]>
(cherry picked from commit 8e93c54)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ZilongX pushed a commit that referenced this pull request Feb 6, 2024
…a source is enabled (#5781) (#5807)

* add datasource picker to saved object management page when multiple data source is enabled

Signed-off-by: Lu Yu <[email protected]>

* add changelog

Signed-off-by: Lu Yu <[email protected]>

* change name to cluster selector and move to higher level

Signed-off-by: Lu Yu <[email protected]>

---------

Signed-off-by: Lu Yu <[email protected]>
(cherry picked from commit 8e93c54)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Support Multiple Data Source when Import saved objects from uploading files
6 participants