-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
don't register any features in LP. #65611
don't register any features in LP. #65611
Conversation
Pinging @elastic/kibana-gis (Team:Geo) |
Pinging @elastic/kibana-platform (Team:Platform) |
} | ||
|
||
class NamespaceAgnosticTypePlugin implements Plugin { | ||
setup(core: CoreSetup, plugins: SetupDeps) { |
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.
@elastic/kibana-platform that's another big thing to get rid of LP: to make all tests KP-compatible. we should have done it as a part of the plugin migration effort 😅
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 agree. Even if they are small, there is a lot of test legacy plugins we will still need to migrate to get rid of legacy...
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
} | ||
|
||
class NamespaceAgnosticTypePlugin implements Plugin { | ||
setup(core: CoreSetup, plugins: SetupDeps) { |
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 agree. Even if they are small, there is a lot of test legacy plugins we will still need to migrate to get rid of legacy...
type: string; | ||
ignore_above?: number; |
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 seems to be only used in a test plugin and could probably be removed from the mapping instead (even if I did not try to launch the test suite with the mapping modified). But I guess it doesn't hurt to still add it.
const { registerFeature, getFeatures } = server.newPlatform.setup.plugins.features; | ||
server.expose('registerFeature', registerFeature); | ||
const { getFeatures } = server.newPlatform.setup.plugins.features; | ||
server.expose('getFeatures', getFeatures); |
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.
Are 3rd party plugins meant to be used with a licensed version of Kibana supposed to be able to use x-pack features/apis?
If so, should we add a dev doc or anything to warn that registerFeature
is no longer usable from legacy plugins?
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.
ok, I will do. just in case
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, thanks @restrry!
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
maps changes lgtm
code review, tested in chrome
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* don't register any features in LP. breaks features value reading in KP * move test plugin to NP * fix mappings * update docs * migrate another test * use contstants file for BWC with original code Co-authored-by: Elastic Machine <[email protected]>
* don't register any features in LP. breaks features value reading in KP * move test plugin to NP * fix mappings * update docs * migrate another test * use contstants file for BWC with original code Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Fixes #65461
It blocks security plugin migration. A list of features is locked on the first reading, which means that maps LP plugin cannot register a feature after the Security plugin reads the value in KP.