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

[Multiple Datasource][Version Decoupling] Support Version Decoupling in Index Patterns Dashboards Plugin #7100

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/7100.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [MDS][Version Decoupling] Add support of Version Decoupling in Index Patterns Dashboards Plugin ([#7100](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7100))
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ test('set defaults for all missing optional fields', async () => {
requiredBundles: [],
server: true,
ui: false,
supportedOSDataSourceVersions: undefined,
});
});

Expand All @@ -434,6 +435,7 @@ test('return all set optional fields as they are in manifest', async () => {
'test-opensearch-plugin-2': '>=1.0.0',
},
ui: true,
supportedOSDataSourceVersions: '>=1.0.0',
})
)
);
Expand All @@ -452,6 +454,7 @@ test('return all set optional fields as they are in manifest', async () => {
},
server: false,
ui: true,
supportedOSDataSourceVersions: '>=1.0.0',
});
});

Expand Down
2 changes: 2 additions & 0 deletions src/core/server/plugins/discovery/plugin_manifest_parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ const KNOWN_MANIFEST_FIELDS = (() => {
server: true,
extraPublicDirs: true,
requiredBundles: true,
supportedOSDataSourceVersions: true,
};

return new Set(Object.keys(manifestFields));
Expand Down Expand Up @@ -245,6 +246,7 @@ export async function parseManifest(
ui: includesUiPlugin,
server: includesServerPlugin,
extraPublicDirs: manifest.extraPublicDirs,
supportedOSDataSourceVersions: manifest.supportedOSDataSourceVersions,
};
}

Expand Down
2 changes: 2 additions & 0 deletions src/core/server/plugins/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): Plug
requiredBundles: [],
server: true,
ui: true,
supportedOSDataSourceVersions: '>=1.0.0',
...manifestProps,
};
}
Expand Down Expand Up @@ -125,6 +126,7 @@ test('`constructor` correctly initializes plugin instance', () => {
expect(plugin.path).toBe('some-plugin-path');
expect(plugin.requiredPlugins).toEqual(['some-required-dep']);
expect(plugin.optionalPlugins).toEqual(['some-optional-dep']);
expect(plugin.supportedOSDataSourceVersions).toEqual('>=1.0.0');
});

test('`setup` fails if `plugin` initializer is not exported', async () => {
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/plugins/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export class PluginWrapper<
public readonly requiredBundles: PluginManifest['requiredBundles'];
public readonly includesServerPlugin: PluginManifest['server'];
public readonly includesUiPlugin: PluginManifest['ui'];
public readonly supportedOSDataSourceVersions: PluginManifest['supportedOSDataSourceVersions'];

private readonly log: Logger;
private readonly initializerContext: PluginInitializerContext;
Expand Down Expand Up @@ -100,6 +101,7 @@ export class PluginWrapper<
this.requiredBundles = params.manifest.requiredBundles;
this.includesServerPlugin = params.manifest.server;
this.includesUiPlugin = params.manifest.ui;
this.supportedOSDataSourceVersions = params.manifest.supportedOSDataSourceVersions;
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/core/server/plugins/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ export interface PluginManifest {
* @deprecated
*/
readonly extraPublicDirs?: string[];

/**
* Specifies the supported version range when adding OpenSearch cluster as data sources.
* Value will be following semver as a range for example ">= 1.3.0"
*/
readonly supportedOSDataSourceVersions?: string;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data_source/common/data_sources/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export interface DataSourceAttributes extends SavedObjectAttributes {
title: string;
description?: string;
endpoint: string;
dataSourceVersion?: string;
dataSourceVersion: string;
dataSourceEngineType?: DataSourceEngineType;
installedPlugins?: string[];
auth: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
"ui": true,
"optionalPlugins": ["dataSource"],
"requiredPlugins": ["management", "data", "urlForwarding"],
"requiredBundles": ["opensearchDashboardsReact", "opensearchDashboardsUtils"]
"requiredBundles": ["opensearchDashboardsReact", "opensearchDashboardsUtils"],
"supportedOSDataSourceVersions": ">=1.0.0"
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@
DataSourceRef,
IndexPatternManagmentContext,
} from 'src/plugins/index_pattern_management/public/types';
import semver from 'semver';
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';
Copy link
Member

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?

Copy link
Collaborator Author

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


interface HeaderProps {
onDataSourceSelected: (id: string, type: string, title: string) => void;
Expand Down Expand Up @@ -68,6 +70,12 @@
.then((fetchedDataSources: DataSourceTableItem[]) => {
setIsLoading(false);
if (fetchedDataSources?.length) {
fetchedDataSources = fetchedDataSources.filter((dataSource) =>
semver.satisfies(

Check warning on line 74 in src/plugins/index_pattern_management/public/components/create_index_pattern_wizard/components/step_data_source/components/header/header.tsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/index_pattern_management/public/components/create_index_pattern_wizard/components/step_data_source/components/header/header.tsx#L73-L74

Added lines #L73 - L74 were not covered by tests
dataSource.datasourceversion,
Copy link
Member

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?

Copy link
Collaborator Author

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

pluginManifest.supportedOSDataSourceVersions
)
);
setDataSources(fetchedDataSources);
}
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,5 @@ export interface DataSourceTableItem {
sort: string;
checked?: 'on' | 'off';
label: string;
datasourceversion: string;
Copy link
Member

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?

Copy link
Collaborator Author

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@
savedObjectsClient
.find<DataSourceAttributes>({
type: 'data-source',
fields: ['title', 'type'],
fields: ['title', 'type', 'dataSourceVersion'],
perPage: 10000,
})
.then((response) =>
Expand All @@ -99,13 +99,15 @@
const id = dataSource.id;
const type = dataSource.type;
const title = dataSource.get('title');
const datasourceversion = dataSource.get('dataSourceVersion');

Check warning on line 102 in src/plugins/index_pattern_management/public/components/utils.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/index_pattern_management/public/components/utils.ts#L102

Added line #L102 was not covered by tests
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.


return {
id,
title,
type,
label: title,
sort: `${title}`,
datasourceversion,
};
})
.sort((a, b) => a.sort.localeCompare(b.sort))
Expand Down
Loading