-
Notifications
You must be signed in to change notification settings - Fork 158
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
Adds Cluster Selector to the Security Plugin if multi-datasources enabled #1810
Adds Cluster Selector to the Security Plugin if multi-datasources enabled #1810
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/supporting-multiple-datasources #1810 +/- ##
===========================================================================
- Coverage 67.41% 67.31% -0.10%
===========================================================================
Files 94 95 +1
Lines 2409 2417 +8
Branches 321 322 +1
===========================================================================
+ Hits 1624 1627 +3
- Misses 707 712 +5
Partials 78 78 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
uses: ./.github/actions/run-cypress-tests | ||
with: | ||
dashboards_config_file: opensearch_dashboards_multidatasources.yml | ||
yarn_command: 'yarn cypress:run --browser chrome --headless --spec "test/cypress/e2e/multi-datasources/multi_datasources_disabled.spec.js"' |
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 default is that its disabled, so it might be cleaner to just add a simple assertion to an existing cypress test that asserts the cluster selector is not present. Decided to break this out for clarity/I can remove this file and move it into another file as part of cleanup.
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -10,7 +10,8 @@ | |||
"savedObjectsManagement" | |||
], | |||
"optionalPlugins": [ |
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.
TODO: building the plugin fails when adding dataSourceManagement as a "required bundle", even though it is suggested to do so by the core team. I opened an issue in core here: opensearch-project/OpenSearch-Dashboards#6018 to get more clarity, but removed it for now from this PR. Will follow up after core responds to the issue.
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.
Reverted it back since without it it a different error is thrown. Will be ok to merge this with build/install workflow failing but everything else passing, until core puts in a fix or suggests a workaround.
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.
Added it as an optional plugin as per suggestion from @BionIT
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.
Optional is not working. Leaving it as requiredBundle, even though this is failing yarn build/install workflow for now.
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Ignoring code cov for now, since changes are in flight to use different components and registration methods. We can do a final codecov check when merging the feature branch into main. |
order: 500, | ||
mount: (element: HTMLElement) => { | ||
ReactDOM.render( | ||
<ClusterSelector |
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 call this datasource selector to be consistent with the naming for multiple data sources?
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.
I can get onboard with that - however, in the next PR I will be removing this entirely so I think it's ok to keep it as is. The next PR will pick up the data source selector that will get built into the TopNavMenu, however that is not in main yet. The PR will get raised once the changes are in main.
Marking as draft first - when I get back next week if things are in OSD core by that point will replace this one in favor of the more complete PR |
Closing this since OSD merged changes into main that I will consume in #1818 |
Description
Adds dataSourceManagement as a required bundle. This is necessary to import the shared cluster selector component that should be used to support multi datasources in the security dashboards plugin. It adds this component to the top nav if multi datasources feature is enabled, and does not do so if it is disabled. It also adds unit and cypress tests verifying this behavior.
Category
Enhancement
Why these changes are required?
This is needed to import and use the shared component coming from OSD core.
What is the old behavior before changes and new behavior after changes?
Very crude UI/UX but this PR simply registers the component to the left of the avatar if datasources are enabled. TODO: right now it is registered globally, we need to figure out how to register it only for the security plugin screens. However, OSD core may expose this in the future, so thus not spending too much time tinkering with it.
Issues Resolved
Closes: #1795
Testing
Plugin should build and existing tests should pass
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.