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

[Discover - datasource selector] Add extension group title and modal #5815

Merged

Conversation

mengweieric
Copy link
Collaborator

@mengweieric mengweieric commented Feb 6, 2024

Description

Add extension group title to non-index data source groups and present a modal to users upon selecting non-index pattern data sources.

Issues Resolved

Screenshot

Screenshot 2024-02-06 at 11 35 45 AM

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

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

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

Project coverage is 67.27%. Comparing base (a9e874f) to head (78a1a32).
Report is 29 commits behind head on main.

Files Patch % Lines
...rces/datasource_selector/datasource_selectable.tsx 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5815      +/-   ##
==========================================
+ Coverage   67.21%   67.27%   +0.05%     
==========================================
  Files        3344     3345       +1     
  Lines       64792    64803      +11     
  Branches    10427    10428       +1     
==========================================
+ Hits        43552    43598      +46     
+ Misses      18697    18653      -44     
- Partials     2543     2552       +9     
Flag Coverage Δ
Linux_2 55.45% <ø> (ø)
Linux_3 ?
Linux_4 ?
Windows_1 31.66% <0.00%> (-0.01%) ⬇️
Windows_2 55.41% <ø> (?)
Windows_3 44.79% <92.30%> (+0.09%) ⬆️
Windows_4 35.05% <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.

@mengweieric mengweieric marked this pull request as ready for review February 6, 2024 19:34
@mengweieric mengweieric changed the title Feature/redirection modal [Discover - datasource selector] Add extension group title and modal Feb 6, 2024
ashwin-pc
ashwin-pc previously approved these changes Feb 6, 2024
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Spoke to @mengweieric offline. The change in here is not ideal and is mostly a result of the workaround we have to redirect users to different apps. Since the component is already marked as experimental and has to be mostly rewritten post 2.12, these changes are okay for now since they do not prevent us from changing them in another minor release.

@mengweieric mengweieric force-pushed the feature/redirection-modal branch from 54fc2ef to 8be8a1c Compare March 15, 2024 21:34
ashwin-pc
ashwin-pc previously approved these changes Mar 18, 2024
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Save for that test this looks good for the 2.13 temporary redirection.

const redirectToLogExplorer = useCallback(
(dsName: string, dsType: string) => {
return application.navigateToUrl(
`../observability-logs#/explorer?datasourceName=${dsName}&datasourceType=${dsType}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this assume that the plugin handling observability-logs is always installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The s3 datasource with this redirection is for now only be able to be registered through observability plugin. Therefore it can only see s3 datasources when observability (log explorer as part of it) presented.

Copy link
Collaborator

@AMoo-Miki AMoo-Miki Mar 25, 2024

Choose a reason for hiding this comment

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

What happens if I have an S3 DS configured and then connect to the cluster using an OSD that doesn't have the observability plugin? On the surface, I think it will show the DS and attempt to redirect. This should check if the plugin is available and if not, fail to redirect, and also disable selecting the DS.

PS, I think due to rareness of this coming up, this can be filed as an issue for enhancing this; it is not a blocker.

Copy link
Collaborator Author

@mengweieric mengweieric Mar 25, 2024

Choose a reason for hiding this comment

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

yes, currently, there is no way to register s3 datasources outside of Observability plugin. Soon in next release probably, discover will be the only data exploration interface for s3 datasources where this temporary redirection will be removed. I feel it should also make sense to users that whatever datasource showed up in discover datasource selector is supported and available through using discover to avoid confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make sure we have an issue to track this @mengweieric

@kavilla kavilla added v2.14.0 and removed v2.13.0 labels Apr 1, 2024
@ashwin-pc ashwin-pc merged commit a0eaf84 into opensearch-project:main Apr 5, 2024
76 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 5, 2024
…5815)

* add log explorer re-directon modal

Signed-off-by: Eric <[email protected]>

* adjustments to comments

Signed-off-by: Eric <[email protected]>

* add one missing i18n

Signed-off-by: Eric <[email protected]>

* add redirection text to group title

Signed-off-by: Eric <[email protected]>

* include changes in changelog

Signed-off-by: Eric <[email protected]>

* remove redundent title addition and unnecessary modal toggle functions

Signed-off-by: Eric <[email protected]>

* remove one comment

Signed-off-by: Eric <[email protected]>

* add i18n

Signed-off-by: Eric <[email protected]>

* add unit tests for modal

Signed-off-by: Eric <[email protected]>

* test id change

Signed-off-by: Eric <[email protected]>

* add devDependencies for tests

Signed-off-by: Eric <[email protected]>

* use open confirm api and move mock file to discover mock folder

Signed-off-by: Eric <[email protected]>

* remove unused type

Signed-off-by: Eric <[email protected]>

* remove modal for log explorer redirection

Signed-off-by: Eric <[email protected]>

* modify changelog

Signed-off-by: Eric <[email protected]>

* remove modal test

Signed-off-by: Eric <[email protected]>

* remove one modal related test

Signed-off-by: Eric <[email protected]>

---------

Signed-off-by: Eric <[email protected]>
(cherry picked from commit a0eaf84)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
SuZhou-Joe pushed a commit to SuZhou-Joe/OpenSearch-Dashboards that referenced this pull request Apr 7, 2024
…pensearch-project#5815)

* add log explorer re-directon modal

Signed-off-by: Eric <[email protected]>

* adjustments to comments

Signed-off-by: Eric <[email protected]>

* add one missing i18n

Signed-off-by: Eric <[email protected]>

* add redirection text to group title

Signed-off-by: Eric <[email protected]>

* include changes in changelog

Signed-off-by: Eric <[email protected]>

* remove redundent title addition and unnecessary modal toggle functions

Signed-off-by: Eric <[email protected]>

* remove one comment

Signed-off-by: Eric <[email protected]>

* add i18n

Signed-off-by: Eric <[email protected]>

* add unit tests for modal

Signed-off-by: Eric <[email protected]>

* test id change

Signed-off-by: Eric <[email protected]>

* add devDependencies for tests

Signed-off-by: Eric <[email protected]>

* use open confirm api and move mock file to discover mock folder

Signed-off-by: Eric <[email protected]>

* remove unused type

Signed-off-by: Eric <[email protected]>

* remove modal for log explorer redirection

Signed-off-by: Eric <[email protected]>

* modify changelog

Signed-off-by: Eric <[email protected]>

* remove modal test

Signed-off-by: Eric <[email protected]>

* remove one modal related test

Signed-off-by: Eric <[email protected]>

---------

Signed-off-by: Eric <[email protected]>
SuZhou-Joe pushed a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Apr 7, 2024
…pensearch-project#5815)

* add log explorer re-directon modal

Signed-off-by: Eric <[email protected]>

* adjustments to comments

Signed-off-by: Eric <[email protected]>

* add one missing i18n

Signed-off-by: Eric <[email protected]>

* add redirection text to group title

Signed-off-by: Eric <[email protected]>

* include changes in changelog

Signed-off-by: Eric <[email protected]>

* remove redundent title addition and unnecessary modal toggle functions

Signed-off-by: Eric <[email protected]>

* remove one comment

Signed-off-by: Eric <[email protected]>

* add i18n

Signed-off-by: Eric <[email protected]>

* add unit tests for modal

Signed-off-by: Eric <[email protected]>

* test id change

Signed-off-by: Eric <[email protected]>

* add devDependencies for tests

Signed-off-by: Eric <[email protected]>

* use open confirm api and move mock file to discover mock folder

Signed-off-by: Eric <[email protected]>

* remove unused type

Signed-off-by: Eric <[email protected]>

* remove modal for log explorer redirection

Signed-off-by: Eric <[email protected]>

* modify changelog

Signed-off-by: Eric <[email protected]>

* remove modal test

Signed-off-by: Eric <[email protected]>

* remove one modal related test

Signed-off-by: Eric <[email protected]>

---------

Signed-off-by: Eric <[email protected]>
SuZhou-Joe pushed a commit that referenced this pull request Apr 9, 2024
…5815) (#6359)

* add log explorer re-directon modal

Signed-off-by: Eric <[email protected]>

* adjustments to comments

Signed-off-by: Eric <[email protected]>

* add one missing i18n

Signed-off-by: Eric <[email protected]>

* add redirection text to group title

Signed-off-by: Eric <[email protected]>

* include changes in changelog

Signed-off-by: Eric <[email protected]>

* remove redundent title addition and unnecessary modal toggle functions

Signed-off-by: Eric <[email protected]>

* remove one comment

Signed-off-by: Eric <[email protected]>

* add i18n

Signed-off-by: Eric <[email protected]>

* add unit tests for modal

Signed-off-by: Eric <[email protected]>

* test id change

Signed-off-by: Eric <[email protected]>

* add devDependencies for tests

Signed-off-by: Eric <[email protected]>

* use open confirm api and move mock file to discover mock folder

Signed-off-by: Eric <[email protected]>

* remove unused type

Signed-off-by: Eric <[email protected]>

* remove modal for log explorer redirection

Signed-off-by: Eric <[email protected]>

* modify changelog

Signed-off-by: Eric <[email protected]>

* remove modal test

Signed-off-by: Eric <[email protected]>

* remove one modal related test

Signed-off-by: Eric <[email protected]>

---------

Signed-off-by: Eric <[email protected]>
(cherry picked from commit a0eaf84)
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.

5 participants