-
Notifications
You must be signed in to change notification settings - Fork 885
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
[Multiple Datasource][Version Decoupling] Support Version Decoupling in Index Patterns Dashboards Plugin #7100
[Multiple Datasource][Version Decoupling] Support Version Decoupling in Index Patterns Dashboards Plugin #7100
Conversation
…in Index Patterns Dashboards Plugin Signed-off-by: Zilong Xia <zilongx@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7100 +/- ##
==========================================
+ Coverage 67.43% 67.45% +0.01%
==========================================
Files 3448 3448
Lines 67960 67964 +4
Branches 11057 11057
==========================================
+ Hits 45832 45845 +13
+ Misses 19454 19446 -8
+ Partials 2674 2673 -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
Can you please add more UT for uncovered lines so that codecov CI would pass? |
Do we have any test result to support above sentence? Can we attach that here as source of truth? |
import { useOpenSearchDashboards } from '../../../../../../../../../plugins/opensearch_dashboards_react/public'; | ||
import { getDataSources } from '../../../../../../components/utils'; | ||
import { DataSourceTableItem, StepInfo } from '../../../../types'; | ||
import { LoadingState } from '../../../loading_state'; | ||
import * as pluginManifest from '../../../../../../../opensearch_dashboards.json'; |
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 use absolute path here instead of relative path with directory traversal?
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.
yes, this one just to keep the save relative style as existing imports lol
@@ -92,4 +92,5 @@ export interface DataSourceTableItem { | |||
sort: string; | |||
checked?: 'on' | 'off'; | |||
label: string; | |||
datasourceversion: string; |
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.
Is it expected that each plugin needs to have this change in order to onboard version decoupling?
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.
Yes, datasource would be the minimal fields to check in order to decide the version compatibility
@@ -99,13 +99,15 @@ export async function getDataSources(savedObjectsClient: SavedObjectsClientContr | |||
const id = dataSource.id; | |||
const type = dataSource.type; | |||
const title = dataSource.get('title'); | |||
const datasourceversion = dataSource.get('dataSourceVersion'); |
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.
same as above.
@@ -68,6 +70,12 @@ export const Header: React.FC<HeaderProps> = (props: HeaderProps) => { | |||
.then((fetchedDataSources: DataSourceTableItem[]) => { | |||
setIsLoading(false); | |||
if (fetchedDataSources?.length) { | |||
fetchedDataSources = fetchedDataSources.filter((dataSource) => | |||
semver.satisfies( | |||
dataSource.datasourceversion, |
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.
what about for existing/old data sources that doesn't have this field? Does it break backwards compatibility?
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.
for older data sources it won't work yest as they won't support version decoupling, MDS initially just list out all data sources created regardless so there would be a bunch of unexpected behaviors, version decoupling is aiming to improve that user experiences by only allowing consumption against compatible data sources.
besides there is a refresh
flow of data sources planning in progress which would also resolve the concern here, which will refresh the current saved object to bring it up-to-date
Yes I was planned yet later found the whole section was not covered in the first place, I'll try to add coverage for the whole section or maybe in separate PR |
Yes, we have test results indicating latest Index Pattern would only support ES 7.9 + which for OS it would be 1.0 + |
…in Index Patterns Dashboards Plugin (#7100) * [Multiple Datasource][Version Decoupling] Support Version Decoupling in Index Patterns Dashboards Plugin Signed-off-by: Zilong Xia <zilongx@amazon.com> * Changeset file for PR #7100 created/updated --------- Signed-off-by: Zilong Xia <zilongx@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 5ee6f58) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…in Index Patterns Dashboards Plugin (#7100) (#7123) * [Multiple Datasource][Version Decoupling] Support Version Decoupling in Index Patterns Dashboards Plugin * Changeset file for PR #7100 created/updated --------- (cherry picked from commit 5ee6f58) Signed-off-by: Zilong Xia <zilongx@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Version Decoupling
inIndex Patterns Dashboards Plugin
supportedOSDataSourceVersions
in the plugin manifest fileIndex Patterns Plugin
it would support all versions of OS clusters so the supported range would be>=1.0.0
Issues Resolved
#7099
Screenshot
Screen.Recording.2024-06-24.at.1.30.34.PM.mov
Changelog
Check List
yarn test:jest
yarn test:jest_integration