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

Add set default datasource #6186

Merged
merged 10 commits into from
Mar 19, 2024

Conversation

zhyuanqi
Copy link
Collaborator

@zhyuanqi zhyuanqi commented Mar 18, 2024

Description

Add the default datasource. So after customer sets the datasource, this will store in the Kibana config
For overview default datasource, it should have feature below:

  1. [Done]Customer can set default datasource and it would be saved in the Kibana file under config.
  2. [Done]Once customer sets a datasource as default, they cannot undefault the datasource

Issues Resolved

#6058

Screenshot

Screen.Recording.2024-03-18.at.2.17.02.PM.mov

Testing the changes

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

@bandinib-amzn
Copy link
Member

Looks like there are some lint issues (see Build and test / Build and Verify on Linux CI). Can you fix those please?

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 67.25%. Comparing base (daccae7) to head (df11696).

Files Patch % Lines
...rce/components/edit_form/edit_data_source_form.tsx 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6186   +/-   ##
=======================================
  Coverage   67.24%   67.25%           
=======================================
  Files        3345     3345           
  Lines       64826    64840   +14     
  Branches    10432    10435    +3     
=======================================
+ Hits        43595    43606   +11     
- Misses      18689    18690    +1     
- Partials     2542     2544    +2     
Flag Coverage Δ
Linux_1 31.64% <ø> (ø)
Linux_2 55.45% <ø> (ø)
Linux_3 44.74% <92.85%> (+0.02%) ⬆️
Linux_4 35.05% <ø> (ø)
Windows_1 31.67% <ø> (ø)
Windows_2 55.41% <ø> (ø)
Windows_3 44.75% <92.85%> (+0.02%) ⬆️
Windows_4 35.05% <ø> (ø)

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.

Comment on lines 50 to 55
const setDefaultAriaLabel = i18n.translate(
'dataSourcesManagement.editDataSource.setDefaultDataSource',
{
defaultMessage: 'Set as default Data Source.',
}
);
Copy link
Member

Choose a reason for hiding this comment

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

nit: Set as default data source

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@@ -144,6 +174,8 @@ export const Header = ({
{/* Right side buttons */}
<EuiFlexItem grow={false}>
<EuiFlexGroup alignItems="baseline" gutterSize="m" responsive={false}>
{/* Test defaultn button */}
Copy link
Member

Choose a reason for hiding this comment

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

nit: spelling mistake

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will change it

@bandinib-amzn
Copy link
Member

Is this option only going to be on edit screen? What about while creating data source? Can we set default data source while creating data source? Just to avoid multiple clicks and rendering multiple forms to set default data source.

CHANGELOG.md Outdated
@@ -117,6 +117,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Discover] Enhanced the data source selector with added sorting functionality ([#5609](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/5609))
- [Multiple Datasource] Add datasource picker component and use it in devtools and tutorial page when multiple datasource is enabled ([#5756](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5756))
- [Multiple Datasource] Add datasource picker to import saved object flyout when multiple data source is enabled ([#5781](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5781))
- [Multiple Datasource] Add default functionality for customer to choose default datasource ([#6058](https://github.com/opensearch-project/OpenSearch-Dashboards/issues/6058))
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should put the change log in the unreleased enhancement section above, this section is for previous 2.12 release

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

}

isDefault = () => {
return this.props.isDefault;
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this function only exists to return the prop, then do we need it? Can we just use the prop value?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Comment on lines 50 to 55
const setDefaultAriaLabel = i18n.translate(
'dataSourcesManagement.editDataSource.setDefaultDataSource',
{
defaultMessage: 'Set as default Data Source.',
}
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

@@ -128,7 +135,9 @@ export const EditDataSource: React.FunctionComponent<RouteComponentProps<{ id: s
<EditDataSourceForm
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add tests for this component?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added!

CHANGELOG.md Outdated
@@ -12,6 +12,9 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

- Support dynamic CSP rules to mitigate Clickjacking https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5641
- [CVE-2020-36604] Employ a patched version of hoek `6.1.3` ([#6148](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6148))
- [CVE-2023-45857] Bump `axios` from `0.27.2` to `1.6.1` ([#5470](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5470))
Copy link
Member

Choose a reason for hiding this comment

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

changelog updates should not contain other PRs. probably just need a rebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

@seraphjiang
Copy link
Member

  • [P1]The first create datasource should be auto choice as default datasource
  • [P1] If customer deletes a datasource, clients side should check if it is the default datasource, if it is, it would randomly choose a datasource as default datasouce. If no remaining datasource, the defaultDataSource in Config is none.

@zhyuanqi
this should be out of scope for this PR. could we remove from PR's description. we may track in the meta issue

@zhongnansu zhongnansu added the multiple datasource multiple datasource project label Mar 19, 2024
@zhyuanqi
Copy link
Collaborator Author

zhyuanqi commented Mar 19, 2024

Is this option only going to be on edit screen? What about while creating data source? Can we set default data source while creating data source? Just to avoid multiple clicks and rendering multiple forms to set default data source.

Currently, we don't support set default data source while creating data source. I believe it is the same as index pattern as they don't support set as default during creation. Maybe we can add this in the feature

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
@zhyuanqi zhyuanqi force-pushed the zhyuanqi-setdefault branch from 05fb380 to 2e66cf9 Compare March 19, 2024 03:55
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
BionIT
BionIT previously approved these changes Mar 19, 2024
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
zhyuanqi and others added 4 commits March 19, 2024 06:01
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
@ZilongX
Copy link
Collaborator

ZilongX commented Mar 19, 2024

LGTM, just curious - [Done]Once customer sets a datasource as default, they cannot undefault the datasource feels like the set default datasource operation being a one shot thing then if customer can not undefault, so would the default update being out of scope of this PR or that's designed on purpose ?

@ZilongX
Copy link
Collaborator

ZilongX commented Mar 19, 2024

meanwhile picking up the latest from main and re-running all the checks

@zhyuanqi
Copy link
Collaborator Author

LGTM, just curious - [Done]Once customer sets a datasource as default, they cannot undefault the datasource feels like the set default datasource operation being a one shot thing then if customer can not undefault, so would the default update being out of scope of this PR or that's designed on purpose ?

So we always want customer to have a default datasource. Although customer can not undefault datasource, they can default other datasource in order to undefault the previous one.
We will have two new features about default coming up for next release as

  • The first create datasource should be auto choice as default datasource
  • If customer deletes a datasource, clients side should check if it is the default datasource, if it is, it would randomly choose a datasource as default datasouce. If no remaining datasource, the defaultDataSource in Config is none.

@Flyingliuhub Flyingliuhub merged commit d2347ca into opensearch-project:main Mar 19, 2024
71 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 19, 2024
* Add set default datasource

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* Fix typo

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* change on this.props.isDefault

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* add unit test

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* set data_source to false

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* add more unit test

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* fix lint error

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* edit one more unit test

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* Fix another typo

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

---------

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Co-authored-by: ZilongX <[email protected]>
(cherry picked from commit d2347ca)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
ZilongX pushed a commit that referenced this pull request Mar 19, 2024
* Add set default datasource

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* Fix typo

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* change on this.props.isDefault

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* add unit test

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* set data_source to false

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* add more unit test

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* fix lint error

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* edit one more unit test

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

* Fix another typo

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

---------

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Co-authored-by: ZilongX <[email protected]>
(cherry picked from commit d2347ca)
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
Status: No status
Development

Successfully merging this pull request may close these issues.

9 participants