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

Update import api to have data source id to allow import saved objects from uploading files to have data source #5777

Merged
merged 40 commits into from
Feb 6, 2024

Conversation

yujin-emma
Copy link
Contributor

@yujin-emma yujin-emma commented Feb 2, 2024

Description

Open Search launched Multiple Data Source at 2.4, this feature enables Dashboards to query data from multiple compatible OpenSearch endpoints. When this feature enabled, users can connect data source when adding sample data, querying data via dev tool. However, there is a limitation when user try to import saved objects by uploading a local file. First, we need to know, when multiple data source feature enabled, if you export objects, the data source information will be attached to each object, when using dashboard to visualize the object, each object already contains the data source info, therefore, the visualization tool knows where the object comes from; However, if user tried to import saved objects when they haven’t enabled multiple data source, or from another dashboards where not have the data source info, then the imported saved objects does not include data source, which can be a problem to be visualized correctly in the dashboards when enabled multiple data source.
When multiple data source feature is enabled, user can add sample data and use the dev tool with one connected data source. Import saved objects accepts uploaded files, but the API does not know the data source. These objects cannot display correctly since the it does not know where to fetch the data.
We want to provide user the ability to specify a data source when importing saved objects. When user import saved object without any data source information, the imported saved objects will attach the specified dataSourceId with the objectId; while user import saved objects different data source information ( e.g. user can connect with dataSource1 and some exported saved objects from dataSource2 ), then the objectId would already contains data source info ( dataSource2 ), for this case, we will replace the dataSourceId. The id schema are exactly same as when user add sample data and dev tools with multiple data source enabled. The behavior will keep consistent for users.

Issues Resolved

#5712

previous behavior

default_import_check_conflict_auto_override_override_ask_action
default_import_check_conflict_auto_override_override_success
default_import_check_conflict_auto_override
default_import_create

test1_createNewCopy.mov
resolveDataSourceConflict.mov

Screenshot

Testing the changes

  1. Request: need to enable Multiple Data Source in the config, code pointer: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/config/opensearch_dashboards.yml#L239
  2. Follow the guide to start the OpenSearch-Dashboards, create data source you want to use to import saved objects
  3. Create new data source : Dashboards Management > Data sources
  4. Go to Dashboards Management > Saved objects > Import
  5. At the flyout, choose one data source, you can choose different mode and upload a local file, for different mode, please refer to the video below:

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
test1_createNewCopy.mov
resolveDataSourceConflict.mov
checkConflictNoDataSourceConflict.mov
checkConflictAndAskForOverride.mov
checkConflictAndAskForOverrideWithDataSourceConflict.mov

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

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

Comparison is base (bbd40e1) 66.97% compared to head (99b6c68) 66.78%.

Files Patch % Lines
...erver/saved_objects/import/create_saved_objects.ts 28.00% 14 Missing and 4 partials ⚠️
src/core/server/saved_objects/routes/import.ts 0.00% 5 Missing ⚠️
...rver/saved_objects/routes/resolve_import_errors.ts 0.00% 5 Missing ⚠️
...d_objects/import/check_conflict_for_data_source.ts 85.18% 1 Missing and 3 partials ⚠️
...core/server/saved_objects/import/regenerate_ids.ts 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5777      +/-   ##
==========================================
- Coverage   66.97%   66.78%   -0.19%     
==========================================
  Files        3301     2591     -710     
  Lines       63437    48749   -14688     
  Branches    10107     8765    -1342     
==========================================
- Hits        42485    32559    -9926     
+ Misses      18499    14053    -4446     
+ Partials     2453     2137     -316     
Flag Coverage Δ
Linux_1 ?
Linux_2 55.11% <55.40%> (-0.01%) ⬇️
Linux_3 43.83% <1.35%> (-0.09%) ⬇️
Linux_4 35.21% <1.35%> (-0.07%) ⬇️
Windows_1 ?
Windows_2 ?
Windows_3 ?
Windows_4 ?

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
Collaborator

@BionIT BionIT left a comment

Choose a reason for hiding this comment

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

Left some comments and please let me know if any question

@bandinib-amzn
Copy link
Member

Can you please take a look at failed checks and try to resolve them? Also it looks like commit are signed.

@bandinib-amzn
Copy link
Member

Couple of suggestions:

  1. Utilize block comments where appropriate.
  2. Instead of utilizing @ts-expect-error to suppress errors, consider addressing them.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@BionIT BionIT left a comment

Choose a reason for hiding this comment

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

Added comments

CHANGELOG.md Outdated Show resolved Hide resolved
yujin-emma and others added 16 commits February 6, 2024 00:33
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
…ed new but display they are overriden

Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Flyingliuhub
Flyingliuhub previously approved these changes Feb 6, 2024
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
@@ -274,4 +274,4 @@
# opensearchDashboards.survey.url: "https://survey.opensearch.org"

# Set the value of this setting to true to enable plugin augmentation on Dashboard
# vis_augmenter.pluginAugmentationEnabled: true
# vis_augmenter.pluginAugmentationEnabled: true
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's revert this file. It is not related to our change.

if (object.type === 'visualization' || object.type === 'search') {
// @ts-expect-error
const searchSourceString = object.attributes?.kibanaSavedObjectMeta?.searchSourceJSON;
// @ts-expect-error
Copy link
Member

Choose a reason for hiding this comment

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

what error we are getting here?

test('with createNewCopies disabled', async () => {
const options = setupOptions();
const options = setupOptions(false, undefined);
Copy link
Member

Choose a reason for hiding this comment

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

why we are changing this? Default values are false and undefined?

@@ -410,7 +475,7 @@ describe('#importSavedObjectsFromStream', () => {

test('with createNewCopies enabled', async () => {
// however, we include it here for posterity
const options = setupOptions(true);
const options = setupOptions(true, undefined);
Copy link
Member

Choose a reason for hiding this comment

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

same as above

});

test('accumulates multiple errors', async () => {
const options = setupOptions();
const options = setupOptions(false, undefined);
Copy link
Member

Choose a reason for hiding this comment

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

don't need to change. They have setupOptions has default value.

@bandinib-amzn
Copy link
Member

Although this PR has scope for improvement, functionality wise it is good to go. Let's please make sure to address all the comments and improve quality of this PR in followup PRs.

@bandinib-amzn bandinib-amzn merged commit 510c759 into opensearch-project:main Feb 6, 2024
70 of 71 checks passed
@opensearch-trigger-bot
Copy link
Contributor

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-5777-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 510c7597e4efad952af8de902d1734e2ee726a9d
# Push it to GitHub
git push --set-upstream origin backport/backport-5777-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-5777-to-2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants