-
Notifications
You must be signed in to change notification settings - Fork 159
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
Remove AOSS options from the data source selector #2016
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2016 +/- ##
==========================================
- Coverage 70.61% 70.58% -0.03%
==========================================
Files 97 97
Lines 2600 2601 +1
Branches 380 387 +7
==========================================
Hits 1836 1836
- Misses 668 669 +1
Partials 96 96 ☔ View full report in Codecov by Sentry. |
Blocked on: opensearch-project/OpenSearch-Dashboards#7154 |
@@ -59,6 +59,7 @@ export const SecurityPluginTopNavMenu = React.memo( | |||
: undefined, | |||
onSelectedDataSources: wrapSetDataSourceWithUpdateUrl, | |||
fullWidth: true, | |||
dataSourceFilter: (ds) => ds.attributes.auth.credentials?.service !== 'aoss', |
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.
Adding a test is overkill for this plugin. OSD core has a test: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/b422791347c22029f0cf155ff24492d0ea9e3c8d/src/plugins/data_source_management/public/components/data_source_selectable/data_source_selectable.test.tsx#L86, and we should not bog ourselves down with internal implementations.
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 are the possible values for service?
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.
The values are 'aoss', or 'es', but good call out here. I pulled it from core as a constant since it is available. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/a2ee3629040c71bcf1d5d02a0c110703b4e5ca44/src/plugins/data_source/common/data_sources/types.ts#L50
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.
Should we flip this around and only show datasource that is OpenSearch? Will there be other types of datasources in the future?
@RyanL1997 perhaps you know if other datasource types will be added in a future release?
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.
We actually cannot import the constant as core does not export it. I will create an issue to do that, but reverted the change for now.
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.
This PR is blocked on: opensearch-project/OpenSearch-Dashboards#7261 |
Closing for now until core supports it |
Description
This change removes AOSS data sources from the data source selector. Security is not supported for these data sources, so they should not be shown in the security plugin.
Category
Enhancement
Why these changes are required?
Fix: #2011
What is the old behavior before changes and new behavior after changes?
AOSS data sources used to show up in the selector, now they don't.
Issues Resolved
Fix: #2011
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.