-
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
[D&D] Save index pattern using proper saved object structure #2218
[D&D] Save index pattern using proper saved object structure #2218
Conversation
4fca6c9
to
200708f
Compare
Codecov Report
@@ Coverage Diff @@
## main #2218 +/- ##
==========================================
- Coverage 66.78% 66.78% -0.01%
==========================================
Files 3131 3133 +2
Lines 60057 60079 +22
Branches 9150 9153 +3
==========================================
+ Hits 40109 40123 +14
- Misses 17759 17766 +7
- Partials 2189 2190 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it 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.
Love the PR as a whole. Very surgical. Just a few high level callouts:
- We should be removign the index pattern from the visualization state
- we should be writing migrations for existing saved objects that have the older version of the saved object
- We should have backward compatibility tests/migration tests e.g. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/2.2/src/plugins/visualizations/server/saved_objects/visualization_migrations.ts#L35 and its associated test do something identical to this.
A guide to saved object migrations: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/2.2/src/core/server/saved_objects/migrations/README.md
src/plugins/wizard/public/saved_visualizations/saved_visualization_references.ts
Show resolved
Hide resolved
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 is awesome, a really clean implementation! Did you verify that now the references render correctly in the Saved Object Management UI (feature parity with other visualization types)?
src/plugins/wizard/public/application/utils/use/use_saved_wizard_vis.ts
Outdated
Show resolved
Hide resolved
src/plugins/wizard/public/saved_visualizations/saved_visualization_references.ts
Show resolved
Hide resolved
Removed index pattern from the visualizatonState when we save. Added migration for existing wizard saved object and unit tests. |
…it is only saved as an id in the visualizationState. Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
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.
Love the changes, nice job Abby! Left a few more comments. Also
- Can we update the default version of the doc to 2 in
wizard/public/saved_visualizations/_saved_vis.ts
- Have you verified that the migration works with an actual doc with did not have the references? Since this is the first migration we have since we forked, I just want to make sure that migrations still work for us.
cf927fe
to
5dbbd1e
Compare
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.
Some general comments, but all the functionality looks great, and a big improvement!
src/plugins/wizard/public/saved_visualizations/saved_visualization_references.ts
Show resolved
Hide resolved
Signed-off-by: abbyhu2000 <[email protected]>
c5089e6
to
00b1812
Compare
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.
Ship it! 🚢
Migration tested: Checked out the main branch and created some wizards that only saves index pattern id in the visualization attribute; checked out the new branch and restart opensearch dashboard without restarting opensearch. The migration will trigger. There are no problems with viewing and editing those old wizards and their saved objects have been altered to the new proper way as well (refname has been saved in kibanaSavedObjectMeta and index pattern object has been pushed to the reference array) |
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.
Awesome!
* Save index pattern as a proper saved object relationship, previously it is only saved as an id in the visualizationState. Signed-off-by: abbyhu2000 <[email protected]> * remove index id from visualization when saving & add comments Signed-off-by: abbyhu2000 <[email protected]> * add migration for existing wizard Signed-off-by: abbyhu2000 <[email protected]> * add migration unit test Signed-off-by: abbyhu2000 <[email protected]> * change wizard doc version to 2; change migration version to 2.3.0 Signed-off-by: abbyhu2000 <[email protected]> Signed-off-by: abbyhu2000 <[email protected]> (cherry picked from commit 428e832)
* Save index pattern as a proper saved object relationship, previously it is only saved as an id in the visualizationState. Signed-off-by: abbyhu2000 <[email protected]> * remove index id from visualization when saving & add comments Signed-off-by: abbyhu2000 <[email protected]> * add migration for existing wizard Signed-off-by: abbyhu2000 <[email protected]> * add migration unit test Signed-off-by: abbyhu2000 <[email protected]> * change wizard doc version to 2; change migration version to 2.3.0 Signed-off-by: abbyhu2000 <[email protected]> Signed-off-by: abbyhu2000 <[email protected]> (cherry picked from commit 428e832)
…2271) * Save index pattern as a proper saved object relationship, previously it is only saved as an id in the visualizationState. Signed-off-by: abbyhu2000 <[email protected]> * remove index id from visualization when saving & add comments Signed-off-by: abbyhu2000 <[email protected]> * add migration for existing wizard Signed-off-by: abbyhu2000 <[email protected]> * add migration unit test Signed-off-by: abbyhu2000 <[email protected]> * change wizard doc version to 2; change migration version to 2.3.0 Signed-off-by: abbyhu2000 <[email protected]> Signed-off-by: abbyhu2000 <[email protected]> (cherry picked from commit 428e832) Co-authored-by: Qingyang(Abby) Hu <[email protected]>
Description
Previously the index pattern is only saved as an id in the visualization state. This PR saves the index pattern using proper saved object structure: by saving a indexRefName in the searchSourceJSON in kibanaSavedObjectMeta and also saving the name, type and id of the index pattern as a reference object.
Before:
Should be:
Issues Resolved
resolves #1878
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr