Management API - simpler interface, remove app context usage #71144
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattkime
changed the title
mattkime
changed the title
3 tasks
mattkime
added
v8.0.0
v7.9.0
Feature:Kibana Management
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
mattkime
added
the
release_note:plugin_api_changes
cjcenizal
approved these changes
Jul 9, 2020
There was a problem hiding this comment.
Nice improvement, @mattkime! I found a few typos and things and I explained some of my concerns around the change to the Security section definition. I'd like to hear your perspective on that. None of my comments are terribly controversial though, so I'd like to see them addressed but I think I can still approve to unblock you.
src/plugins/management/public/components/management_sections.tsx
Outdated
Show resolved
Hide resolved
src/plugins/management/public/components/management_sections.tsx
Outdated
Show resolved
Hide resolved
src/plugins/management/public/components/management_sections.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/management/management_service.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security/public/management/management_service.ts
Outdated
Show resolved
Hide resolved
pgayvallet
approved these changes
Jul 13, 2020
legrego
approved these changes
Jul 13, 2020
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
YulNaumenko
approved these changes
Jul 13, 2020
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
lizozom
reviewed
Jul 14, 2020
| // eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
| export interface ManagementStart {} | ||
|
|
||
| export interface ManagementSectionsStartPrivate { |
There was a problem hiding this comment.
Haven't seen this pattern before (Private interface), why is it used?
Also, it seems to be the same as SectionsServiceStart? Is it future proofing?
There was a problem hiding this comment.
One is for the service and one is for the plugin, you're right that its a little redundant but I think its okay.
There was a problem hiding this comment.
Not sure why we need this, but okay
| return { | ||
| sections: this.managementSections.start({ capabilities: core.application.capabilities }), | ||
| }; | ||
| this.managementSections.start({ capabilities: core.application.capabilities }); |
There was a problem hiding this comment.
Why aren't we returning sections on the start contract anymore?
There was a problem hiding this comment.
It was never used externally, only internally.
| private getSection = (sectionId: ManagementSectionId | string) => | ||
| this.sections.get(sectionId) as ManagementSection; | ||
| constructor() { | ||
| // Note on adding sections - sections can be defined in a plugin and exported as a contract |
There was a problem hiding this comment.
Can you provide a short example for what you mean?
There was a problem hiding this comment.
Here's a previous version of the code that was changed due to feedback - 291bf7a
There was a problem hiding this comment.
I mean an example in a comment
|
|
||
| export class ManagementSectionsService { | ||
| private sections: Map<ManagementSectionId | string, ManagementSection> = new Map(); | ||
| definedSections: DefinedSections; |
There was a problem hiding this comment.
//Major nit
why not just sections? (both for the variable and the class)
There was a problem hiding this comment.
its not necessarily the full set of sections, its just the set of sections that have been defined within this plugin. Sections could be defined and exposed via other plugins.
There was a problem hiding this comment.
I would then add a comment at least.
| } from './types'; | ||
| import { createGetterSetter } from '../../kibana_utils/public'; | ||
|
|
||
| const [getSectionsServiceStartPrivate, setSectionsServiceStartPrivate] = createGetterSetter< |
There was a problem hiding this comment.
Would you mind explaining this bit?
Why is the getter setter called private?
There was a problem hiding this comment.
Its only used internally.
There was a problem hiding this comment.
I think all getter-setters are used within the bundle they are defined in. Not sure about this.
There was a problem hiding this comment.
I'm not sure I understand, is there a change you'd recommend? This is used within the bundle.
There was a problem hiding this comment.
Just found the "Private" part a bit unclear. Not critical though.
| this.licenseFeaturesSubscription = this.license.features$.subscribe(async (features) => { | ||
| const securitySection = management.sections.getSection(ManagementSectionId.Security); | ||
| const securitySection = this.securitySection as ManagementSection; |
There was a problem hiding this comment.
Why did you have to cast here?
There was a problem hiding this comment.
We know its been set in the setup method but typescript isn't aware of this. It thinks it might be undefined.
There was a problem hiding this comment.
Won't this.securitySection! work as well. I find it clearer.
lizozom
approved these changes
Jul 14, 2020
mattkime
added a commit
to mattkime/kibana
that referenced
this pull request
Jul 14, 2020
…#71144) Management API - simpler interface, remove app context usage, consolidate rendeing # Conflicts: # src/core/MIGRATION.md # x-pack/plugins/upgrade_assistant/kibana.json
mattkime
added a commit
that referenced
this pull request
Jul 14, 2020
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/context/_date_nanos·js.context app context view for date_nanos displays predessors - anchor - successors in right orderStandard OutStack TraceKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/context/_date_nanos·js.context app context view for date_nanos displays predessors - anchor - successors in right orderStandard OutStack TraceKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/dashboard/dashboard_filter_bar·js.dashboard app using current data dashboard filter bar filter editor field list shows index pattern of vis when one is addedStandard OutStack Traceand 6 more failures, only showing the first 3. Build metricsSaved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Dev Docs
Management API changes - Public setup contract has been reduced to just
registerand a new interface,sectionswhich is a map of management sections provided by the plugin and replacesgetSection. Public start interfaces have been removed as all registration should occur in the setup lifecycle.