-
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
[MDS] Install Vega sample data #6218
[MDS] Install Vega sample data #6218
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6218 +/- ##
=======================================
Coverage 67.46% 67.46%
=======================================
Files 3368 3368
Lines 65422 65431 +9
Branches 10557 10560 +3
=======================================
+ Hits 44134 44145 +11
+ Misses 18717 18714 -3
- Partials 2571 2572 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM
} | ||
|
||
export const updateDataSourceNameInVegaSpec = ( | ||
props: UpdateDataSourceNameInVegaSpecProps | ||
): string => { | ||
const { spec } = props; | ||
const { spec, spacing } = props; | ||
const stringifiedSpacing = !!spacing ? spacing : 2; |
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.
a shorter way to write this const stringifiedSpacing = spacing || 2;
import { SavedObject, updateDataSourceNameInVegaSpec } from '../../../../../../core/server'; | ||
|
||
describe('getSavedObjectsWithDataSource()', () => { | ||
const getVisualizationSavedObjects = (): Array<SavedObject<any>> => { |
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.
nonblocking: you can move this large json object to a data.json
file and then load it with one line
import data from './data.json'
CHANGELOG.md
Outdated
@@ -59,6 +59,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
- [Discover] Options button to configure legacy mode and remove the top navigation option ([#6170](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6170)) | |||
- [Multiple Datasource] Add default functionality for customer to choose default datasource ([#6058](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/6058)) | |||
- [Multiple Datasource] Add import support for Vega when specifying a datasource ([#6123](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6123)) | |||
- [Workspace] Validate if workspace exists when setup inside a workspace ([#6154](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6154)) |
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.
@huyaboo would you rebase from main, so others change won't showup in your PR
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 CHANGELOG.md
change was because my commit adds a new line at the end of the file
f006109
to
94fcc10
Compare
CHANGELOG.md
Outdated
- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322)) |
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.
Did we change this line?
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 believe it was a newline added to the end of my CHANGELOG.md
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.
@huyaboo let's rebase with main. Other contributor's change should not show in your PR.
} | ||
|
||
export const updateDataSourceNameInVegaSpec = ( | ||
props: UpdateDataSourceNameInVegaSpecProps | ||
): string => { | ||
const { spec } = props; | ||
const { spec, spacing } = props; | ||
const stringifiedSpacing = spacing || 2; |
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.
So the type of spacing is number, why is it called stringifiedSpacing? Also, why it is || 2
but not other number? what if spacing is 0?
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.
stringifiedSpacing
is how much indenting should occur when an HJSON document is stringified. It's set to 2
as that seems to be the default for most Vega visualizations. If it were 0
there would be no indenting between the lines so it would look something like this:
{
data: {
url: {
index: some_index
body: {
...
}
}
}
...
}
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.
Can we add those explanation as comments?
const updatedVegaSpec = updateDataSourceNameInVegaSpec({ | ||
spec: vegaSpec, | ||
newDataSourceName: dataSourceTitle, | ||
spacing: 1, |
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 do we hard code spacing as 1 and what does it do?
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 reason spacing
is 1 is because there is one sample data visualization (sankey visualization) that would cause a URL too long error. This setting limits the spacing so the visualization will still be rendered/accessed.
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.
Can we add those explanation as comments?
LGTM, thanks |
CHANGELOG.md
Outdated
@@ -71,7 +71,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
- [Workspace] Add create workspace page ([#6179](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6179)) | |||
- [Multiple Datasource] Make sure customer always have a default datasource ([#6237](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6237)) | |||
- [Workspace] Add workspace list page ([#6182](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6182)) | |||
|
|||
- [Multiple Datasource] Append `data_source_name` to the Vega spec and add datasource reference when installing sample data ([#6218](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6218)) |
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.
nit: can we add simpler text instead of adding details so that it would be easy to understand by anyone? maybe [Multiple Datasource] Add multi data source support to sample vega visualizations
@@ -6,15 +6,24 @@ | |||
import { parse, stringify } from 'hjson'; | |||
import { SavedObject, SavedObjectsClientContract } from '../types'; | |||
|
|||
/** | |||
* Given a Vega spec, the new datasource (by name), and spacing, |
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's not clear from this line, what exactly this function is doing.
Can you update with rebase before merge? |
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
bca768d
to
c000459
Compare
Signed-off-by: Huy Nguyen <[email protected]>
The backport to
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-6218-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8810f085164243532bf8d87f3952c30172ff8d22
# Push it to GitHub
git push --set-upstream origin backport/backport-6218-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 |
* Add data_source_name to sample data install Signed-off-by: Huy Nguyen <[email protected]> * Add unit test coverage Signed-off-by: Huy Nguyen <[email protected]> * Resolve CHANGELOG.md conflicts Signed-off-by: Huy Nguyen <[email protected]> * Refactor logic Signed-off-by: Huy Nguyen <[email protected]> * Remove newline Signed-off-by: Huy Nguyen <[email protected]> * Remove newline Signed-off-by: Huy Nguyen <[email protected]> * Add comments for spacing Signed-off-by: Huy Nguyen <[email protected]> * Update jsdoc Signed-off-by: Huy Nguyen <[email protected]> --------- Signed-off-by: Huy Nguyen <[email protected]> (cherry picked from commit 8810f08)
* Add data_source_name to sample data install Signed-off-by: Huy Nguyen <[email protected]> * Add unit test coverage Signed-off-by: Huy Nguyen <[email protected]> * Resolve CHANGELOG.md conflicts Signed-off-by: Huy Nguyen <[email protected]> * Refactor logic Signed-off-by: Huy Nguyen <[email protected]> * Remove newline Signed-off-by: Huy Nguyen <[email protected]> * Remove newline Signed-off-by: Huy Nguyen <[email protected]> * Add comments for spacing Signed-off-by: Huy Nguyen <[email protected]> * Update jsdoc Signed-off-by: Huy Nguyen <[email protected]> --------- Signed-off-by: Huy Nguyen <[email protected]> (cherry picked from commit 8810f08) Signed-off-by: Huy Nguyen <[email protected]>
Description
With #6123 and #5975 , MDS now fully supports Vega and importing. This PR will fix Vega visualizations when installing sample data. Specifically, the
data_source_name
field will be appended to eachurl
body of the Vega spec and the datasource reference added to the saved object metadata.Issues Resolved
Closes #5927
Screenshot
Screen.Recording.2024-03-19.at.4.47.35.PM.mov
Testing the changes
Data Source Sample
data_source_name: Data Source Sample
was added to eachurl
body of the Vega spec and the datasource reference is included in the saved object metadata.Check List
yarn test:jest
yarn test:jest_integration