-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups #65796
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
…Security, Kibana, and Stack groups. - Define these groups in src/plugins/management/public/legacy/sections_register to act as a single source of truth in both OSS and X-Pack.
3015048
to
465d813
Compare
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.
@cjcenizal this is looking great! I think the buckets communicate solutions really well and create a sense of intentionality that was lacking before. Overall happy with ESUI changes, I loaded Kibana and was able to access all apps (also tested linking from CCR -> remote clusters, working as expected)
I am happy for this to be merged once CI is passing 👍
Code
Left some minor comments.
UI
-
The bucketed list takes up a bit more vertical screen space than its predecessor which creates a scrollbar for all management apps. This is not the worst thing but it might be worth thinking about a way for the nav bar to not increase height of apps directly.
-
Beats seems to be cutting off (same with CCR) on my screen:
@@ -20,6 +20,33 @@ | |||
import { LegacyManagementSection } from './section'; | |||
import { i18n } from '@kbn/i18n'; | |||
|
|||
export const sections = [ | |||
{ | |||
id: 'ingest', |
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 really like this.
One suggestion in case we want to refactor ids whatever reason, it could be useful to have an enum that management exports in public (I can imagine search for 'data' may bring up many results 😄 ). Maybe Sections.Ingest
for instance.
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.
Great idea! Will do.
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'm hoping we'll change how sections are referenced in the near future. I'm 👎 on looking items up by id
unless necessary and this case is not necessary. We should be exporting the sections as public APIs of the plugins that create them. The current usage creates an implicit dependency and the potential for runtime errors instead of compile time errors.
display: 'Logstash', | ||
order: 30, | ||
icon: 'logoLogstash', | ||
sections.forEach(({ id, title, icon }, idx) => { |
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.
Does icon
exist in any of these?
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.
It doesn't, great spot. My original line of thought was that we'd need to support icons for these buckets but TBH, I think they're unnecessary in terms of UX. I'll remove this from the code and we can always reimplement if we need to.
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'm surprised to find Index Patterns
grouped under the Data
section. I can see the rationale for putting it here, but I wonder if it would be better suited under the Kibana
section?
The term "Index Pattern" is already pretty overloaded, and having this link outside of the Kibana
section is rather confusing to me. These Index Patterns truly are specific to Kibana, and only live within Kibana.
Index Patterns are also a type of Saved Object, and the management screen for Saved Objects is under the Kibana
section.
const kibanaSection = management.sections.getSection('kibana'); | ||
if (kibanaSection) { | ||
this.registeredSpacesManagementApp = kibanaSection.registerApp( | ||
this.registeredSpacesManagementApp = management.sections |
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.
If we're assuming that the kibana
section will always exist, can we update the type definitions to reflect that, rather than forcing TypeScript into trusting us here?
@jloleysens Thanks for your review! I've incorporated your feedback. In terms of the change in nav height, I think we should solve this in a separate design pass as long as the way apps are grouped and named makes sense here. @legrego Thanks for your input, I agree with you. I'd like to avoid introducing controversial changes in this PR and I think this particular change deserves more consideration at a later time. |
private getSection(sectionId: ManagementSection['id']) { | ||
return this.sections.find(section => section.id === sectionId); | ||
|
||
private getSection(sectionId: ManagementSectionId): ManagementSection { |
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.
Does anybody have any suggestions on what I could do to convince TypeScript that this function will always return a ManagementSection
, and not undefined
? TS check shows this error everywhere this method is invoked:
Object is possibly 'undefined'.
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 could consider throwing if find
returns undefined
. That would give us both transpile-time and runtime guarantees
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.
See my previous comment. I consider this a flaw in the API. I think throw
ing will work in the mean time.
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.
Thanks Matt! I updated the code to throw.
There is one consideration worth being aware of (this is relevant to Larry's comment above and I have mentioned it in a previous iteration. Reminding here as it might have some UX implications): Some of the features are Space specific while others are not. Notably: Alerts and Actions are space aware while Watcher is not. This means that in Alerts and Actions you only see the alerts belonging to the Space you are in (the Default in this mockup) but if you navigate to Watcher, which is within the same bucket, you will see all watches. Not sure how confusing this might be with the two living in the same bucket. Machine Learning jobs are not Space aware yet but will become some time soon (btw, not sure if Insights and alerting is the right bucket for ML, the ML team should review). I believe that this use-case/user-intention perspective is great and makes a lot of sense, but we might want to think through whether we want the design to somehow indicate this difference between space-specific and space-agnostic entities. |
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.
LGTM for stack monitoring
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.
SIEM changes LGTM 👍
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.
No endpoint changes in here, and I believe SIEM already 👍
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.
APM changes look good to me!
…curity, Kibana, and Stack groups (elastic#65796)
…ine-editor * 'master' of github.com:elastic/kibana: (157 commits) [ML] fix url assertion (#66850) Skip failing lens test(s). #66779 [SOM] Preserve saved object references when saving the object (#66584) Use ES API from start contract (#66157) Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups (#65796) [Uptime] Fix flaky navigation to certs page in tests (#66806) [Maps] Do not check count for blended layers when layer is not visible (#66460) [SIEM] Fixes glob patterns from directory changes recently for GraphQL chore(NA): bump static-fs to 1.0.2 (#66775) [Maps] Handle cross cluster index _settings resp (#66797) [SIEM][Lists] Adds 90% of the REST API and client API for exception lists and exception items allow any type for customResponseHeaders config (#66689) [APM] Disable map layout animation (#66763) [ML] Add linking to dataframe from job management tab (#65778) [Maps] Get number of categories from palette (#66454) move oss features registration to KP (#66524) [kbn/plugin-helpers] typescript-ify (#66513) Add kibana-operations as codeowners for .ci/es-snapshots and vars/ (#66746) FTR: move basic services under common folder (#66563) Migrate Beats Management UI to KP (#65791) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_form/pipeline_form_fields.tsx
* master: [ML] fix url assertion (elastic#66850) Skip failing lens test(s). elastic#66779 [SOM] Preserve saved object references when saving the object (elastic#66584) Use ES API from start contract (elastic#66157) Reorganize Management apps into Ingest, Data, Alerts and Insights, Security, Kibana, and Stack groups (elastic#65796) [Uptime] Fix flaky navigation to certs page in tests (elastic#66806) [Maps] Do not check count for blended layers when layer is not visible (elastic#66460) [SIEM] Fixes glob patterns from directory changes recently for GraphQL chore(NA): bump static-fs to 1.0.2 (elastic#66775) [Maps] Handle cross cluster index _settings resp (elastic#66797) [SIEM][Lists] Adds 90% of the REST API and client API for exception lists and exception items allow any type for customResponseHeaders config (elastic#66689) [APM] Disable map layout animation (elastic#66763) [ML] Add linking to dataframe from job management tab (elastic#65778)
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Replaces #60847. See that issue for more background on the intention and reasoning behind this reorganization.
I'd like to get approval from all affected teams:
CC @elastic/kibana-core-ui
I'm also particularly interested in hearing people's thoughts on how this change defines all possible buckets in a single place (src/plugins/management/public/legacy/sections_register.js). I think this makes it clearer which buckets are available to put apps into and how the buckets are ordered. I don't think it's a concern that we're creating some buckets in OSS that currently only contain X-Pack apps -- these buckets disappear from the UI when they're empty and they provide a useful structure for third-party plugin developers to follow.
Note that I've also removed the ability to register new sections, which I am concerned might prove controversial (and I'm happy to revert this if it's the case). @elastic/kibana-platform Could you chime in on this?
To-do
#67044 tracks required changes to docs.
Release note
Management apps are now organized into buckets that support common workflow-oriented use-cases: data ingestion, data management, insights and alerting, security, Kibana management, and Stack management.