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

[MD] Expose picker using function in data source management plugin setup #6030

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

BionIT
Copy link
Collaborator

@BionIT BionIT commented Mar 5, 2024

Description

This change exposes data source picker as a function in the data source management plugin.

Issues Resolved

Fixes #6018

Screenshot

exposepicker.mp4

Testing the changes

The following was performed in the recording above:

  1. enable data source plugin
  2. go to add sample data page and add data from a data source is successful, note that the picker here is the same picker as used in alerting, query workbench plugins
  3. go to query workbench plugin page, and query the indices from selected data source is successful, note that query workbench has been changed to use the function exposed by the plugin to render the picker
  4. go to alerting plugin page, and query monitors from selected data source is successful, note that alerting has been changed to use the function exposed by the plugin to render the picker
  5. run node scripts/build_opensearch_dashboards_platform_plugins.js is successful
  6. run yarn build for alerting plugin is successful

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: Lu Yu <nluyu@amazon.com>
@BionIT BionIT changed the title Expose picker using function in data source management plugin setup [MD] Expose picker using function in data source management plugin setup Mar 5, 2024
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.13%. Comparing base (54c36fe) to head (0aa33c4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6030   +/-   ##
=======================================
  Coverage   67.13%   67.13%           
=======================================
  Files        3316     3317    +1     
  Lines       64015    64016    +1     
  Branches    10256    10256           
=======================================
+ Hits        42975    42977    +2     
+ Misses      18547    18546    -1     
  Partials     2493     2493           
Flag Coverage Δ
Linux_1 31.63% <ø> (ø)
Linux_2 55.20% <ø> (ø)
Linux_3 44.58% <100.00%> (-0.01%) ⬇️
Linux_4 35.16% <0.00%> (-0.01%) ⬇️
Windows_1 31.65% <ø> (ø)
Windows_2 55.17% <ø> (ø)
Windows_3 44.60% <100.00%> (+<0.01%) ⬆️
Windows_4 35.16% <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.

Copy link
Contributor

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this fix!

return { registerAuthenticationMethod };
return {
registerAuthenticationMethod,
getDataSourcePicker: createClusterSelector(),
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I am understanding correctly, this will expose a function getDataSourcePicker, that plugins should be able to consume via optionalPlugins?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly!

@BionIT BionIT added the multiple datasource multiple datasource project label Mar 5, 2024
@@ -16,7 +16,7 @@ export const LocalCluster: ClusterOption = {
id: '',
};

interface ClusterSelectorProps {
export interface ClusterSelectorProps {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export interface ClusterSelectorProps {
export interface DataSourceSelectorProps {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cut another issue to address the naming #6038

@@ -16,7 +16,7 @@ export const LocalCluster: ClusterOption = {
id: '',
};

interface ClusterSelectorProps {
export interface ClusterSelectorProps {
savedObjectsClient: SavedObjectsClientContract;
notifications: ToastsStart;
onSelectedDataSource: (clusterOption: ClusterOption[]) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onSelectedDataSource: (clusterOption: ClusterOption[]) => void;
onSelectedDataSource: (dataSourceOption: DataSourceOption[]) => void;

@@ -16,7 +16,7 @@ export const LocalCluster: ClusterOption = {
id: '',
};

interface ClusterSelectorProps {
export interface ClusterSelectorProps {
savedObjectsClient: SavedObjectsClientContract;
notifications: ToastsStart;
onSelectedDataSource: (clusterOption: ClusterOption[]) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onSelectedDataSource: (clusterOption: ClusterOption[]) => void;
onSelectedDataSource: (dataSourceOption: DataSourceOption[]) => void;

@@ -16,7 +16,7 @@ export const LocalCluster: ClusterOption = {
id: '',
};

Copy link
Member

Choose a reason for hiding this comment

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

could we rename this file to DataSourceSelector.tsx

* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
import { createClusterSelector } from './create_cluster_selector';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { createClusterSelector } from './create_cluster_selector';
import { dataSourceSelector } from './data_source_selector';

return { registerAuthenticationMethod };
return {
registerAuthenticationMethod,
getDataSourcePicker: createClusterSelector(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getDataSourcePicker: createClusterSelector(),
getDataSourcePicker: createDataSourceSelector(),

import React from 'react';
import { ClusterSelector, ClusterSelectorProps } from './cluster_selector';

export function createClusterSelector() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export function createClusterSelector() {
export function createDataSourceSelector() {

@@ -7,6 +7,7 @@ import { DataSourcePluginSetup } from 'src/plugins/data_source/public';
import { CoreSetup, CoreStart, Plugin } from '../../../core/public';

import { PLUGIN_NAME } from '../common';
import { createClusterSelector } from './components/cluster_selector/create_cluster_selector';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { createClusterSelector } from './components/cluster_selector/create_cluster_selector';
import { createDataSourceSelector } from './components/datasource_selector/create_datasource_selector';

@@ -17,6 +18,7 @@ import {
AuthenticationMethodRegistery,
} from './auth_registry';
import { noAuthCredentialAuthMethod, sigV4AuthMethod, usernamePasswordAuthMethod } from './types';
import { ClusterSelectorProps } from './components/cluster_selector/cluster_selector';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { ClusterSelectorProps } from './components/cluster_selector/cluster_selector';
import { DataSourceSelectorProps } from './components/datasource_selector/datasource_selector';

@@ -26,6 +28,7 @@ export interface DataSourceManagementSetupDependencies {

export interface DataSourceManagementPluginSetup {
registerAuthenticationMethod: (authMethodValues: AuthenticationMethod) => void;
getDataSourcePicker: React.ComponentType<ClusterSelectorProps>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
getDataSourcePicker: React.ComponentType<ClusterSelectorProps>;
getDataSourcePicker: React.ComponentType<DataSourceSelectorProps>;

@@ -19,6 +19,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Discover] Fix table cell content overflowing in Safari ([#5948](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5948))
- [BUG][MD]Fix schema for test connection to separate validation based on auth type ([#5997](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5997))
- [Discover] Enable 'Back to Top' Feature in Discover for scrolling to top ([#6008](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6008))
- [BUG][MD]Expose picker using function in data source management plugin setup([#6030](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6030))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [BUG][MD]Expose picker using function in data source management plugin setup([#6030](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6030))
- [BUG][MD]Expose DataSourceSelector using function in data source management plugin setup([#6030](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6030))

@@ -16,7 +16,7 @@ export const LocalCluster: ClusterOption = {
id: '',
};

interface ClusterSelectorProps {
export interface ClusterSelectorProps {
savedObjectsClient: SavedObjectsClientContract;
notifications: ToastsStart;
onSelectedDataSource: (clusterOption: ClusterOption[]) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onSelectedDataSource: (clusterOption: ClusterOption[]) => void;
onSelectedDataSource: (dataSourceOption: DataSourceOption[]) => void;

import React from 'react';
import { render } from '@testing-library/react';

describe('create cluster selector', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('create cluster selector', () => {
describe('create datasource selector', () => {

hideLocalCluster: false,
fullWidth: false,
};
const TestComponent = createClusterSelector();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const TestComponent = createClusterSelector();
const TestComponent = createDataSourceSelector();

*/

import React from 'react';
import { ClusterSelector, ClusterSelectorProps } from './cluster_selector';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { ClusterSelector, ClusterSelectorProps } from './cluster_selector';
import { DataSourceSelector, DataSourceSelectorProps } from './datasource_selector';

import { ClusterSelector, ClusterSelectorProps } from './cluster_selector';

export function createClusterSelector() {
return (props: ClusterSelectorProps) => <ClusterSelector {...props} />;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return (props: ClusterSelectorProps) => <ClusterSelector {...props} />;
return (props: DataSourceSelectorProps) => <DataSourceSelector {...props} />;

@BionIT BionIT merged commit 49d1649 into opensearch-project:main Mar 5, 2024
67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 5, 2024
…tup (#6030)

* expose picker using function in plugin setup

Signed-off-by: Lu Yu <nluyu@amazon.com>

* add changelog and test

Signed-off-by: Lu Yu <nluyu@amazon.com>

---------

Signed-off-by: Lu Yu <nluyu@amazon.com>
(cherry picked from commit 49d1649)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
BionIT pushed a commit that referenced this pull request Mar 5, 2024
…tup (#6030) (#6037)

* expose picker using function in plugin setup

Signed-off-by: Lu Yu <nluyu@amazon.com>

* add changelog and test

Signed-off-by: Lu Yu <nluyu@amazon.com>

---------

Signed-off-by: Lu Yu <nluyu@amazon.com>
(cherry picked from commit 49d1649)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] adding a "required bundle" throws an error during build even if its used
6 participants