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

filter out data source object when export #6124

Closed

Conversation

yujin-emma
Copy link
Contributor

Description

Issues Resolved

Screenshot

Testing the changes

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: yujin-emma <yujin.emma.work@gmail.com>
Signed-off-by: yujin-emma <yujin.emma.work@gmail.com>
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.15%. Comparing base (45096bb) to head (e3a767e).
Report is 147 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6124      +/-   ##
==========================================
- Coverage   67.15%   67.15%   -0.01%     
==========================================
  Files        3326     3326              
  Lines       64400    64401       +1     
  Branches    10361    10361              
==========================================
- Hits        43251    43250       -1     
- Misses      18619    18620       +1     
- Partials     2530     2531       +1     
Flag Coverage Δ
Linux_1 31.73% <0.00%> (-0.01%) ⬇️
Linux_2 55.23% <100.00%> (+<0.01%) ⬆️
Linux_3 44.75% <0.00%> (-0.01%) ⬇️
Linux_4 35.08% <0.00%> (-0.01%) ⬇️
Windows_1 31.75% <0.00%> (-0.01%) ⬇️
Windows_2 55.20% <100.00%> (+<0.01%) ⬆️
Windows_3 44.75% <0.00%> (-0.01%) ⬇️
Windows_4 35.08% <0.00%> (-0.01%) ⬇️

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.

@@ -180,6 +180,7 @@ export async function exportSavedObjectsToStream({
exportedObjects = sortObjects(rootObjects);
}

exportedObjects = exportedObjects.filter((obj) => obj.type !== 'data-source');
Copy link
Member

@ruanyl ruanyl Mar 13, 2024

Choose a reason for hiding this comment

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

Instead of filtering the final results, shall we filter the types passed in before sending it to savedObjectsClient.find? Or maybe even throw error if export data-source is not allowed, like in fetchObjectsToExport we do validations for the export parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, I assume we are using the saved object find API and we can specify the object types there

Copy link
Contributor Author

@yujin-emma yujin-emma Mar 19, 2024

Choose a reason for hiding this comment

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

Sure, + 1 on using saved object find API and specifying the types; as for

Or maybe even throw error if export data-source is not allowed
partially agree, error can be some interruption on UX, not quite sure about the user experience, but we can have warning first, more discussion is here

@@ -180,6 +180,7 @@ export async function exportSavedObjectsToStream({
exportedObjects = sortObjects(rootObjects);
}

exportedObjects = exportedObjects.filter((obj) => obj.type !== 'data-source');
// redact attributes that should not be exported
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please add tests for this change as well?

@BionIT
Copy link
Collaborator

BionIT commented Mar 13, 2024

Can we add context for this PR as well as what manual tests have been done?

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Mar 14, 2024

Maybe it is not a good time to ask such question but I'm thinking why don't we make data-source as a hidden type? Hidden type won't be export as allowed types in saved objects management and can not be performed CRUD operations on by a normal saved objects client. That meets what data source needs I suppose.

@BionIT BionIT added v2.14.0 multiple datasource multiple datasource project labels Mar 19, 2024
@BionIT BionIT marked this pull request as draft March 19, 2024 23:11
@yujin-emma
Copy link
Contributor Author

Maybe it is not a good time to ask such question but I'm thinking why don't we make data-source as a hidden type? Hidden type won't be export as allowed types in saved objects management and can not be performed CRUD operations on by a normal saved objects client. That meets what data source needs I suppose.

We still need to list the data source object, but since it contains the auth information, we cannot import/export, more discussion is here

@Hailong-am
Copy link
Contributor

If an index pattern depends on datasource, and we don't export datasource will that break the reference? If yes, the user experience is same, all dashboards/visualization will be broken.

we might need remove the reference in index pattern as well when export.

@kavilla
Copy link
Member

kavilla commented May 2, 2024

@yujin-emma will remove the 2.14 label as this in draft still. And have added the 2.15 label or if it needs to be closed feel free to do so.

@kavilla kavilla added v2.15.0 and removed v2.14.0 labels May 2, 2024
@BionIT
Copy link
Collaborator

BionIT commented Jun 3, 2024

@yujin-emma Is this PR still being worked on, are we plan to fix in 2.15?

@yujin-emma yujin-emma closed this Jun 3, 2024
@yujin-emma
Copy link
Contributor Author

Close this pull request since we do not plan to work on filter data source object when export, we are going to stop importing data source objects, and the code change already merged in main : #6395

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.

7 participants