-
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
move oss features registration to KP #66524
move oss features registration to KP #66524
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
e429320
to
c9ebc3a
Compare
const registry = savedObjects.getTypeRegistry(); | ||
const savedObjectTypes = registry | ||
.getAllTypes() | ||
.filter(t => !t.hidden) |
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.
@pgayvallet I wrongly assumed that getAllTypes
should return already filtered types, as savedObjects.type
did in LP. Do we consider it's a problem that a plugin can expose all types to a consumer?
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.
When we use the type registry in the repository we need to know all types, but I think consumers of this API should usually only operate on types that are visible so I think it would make sense to make that the default. If we have a use case where this should be exposed outside of core we could have an option like includedHiddenTypes: true
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 named it getAllTypes
in an attempt to distinguish it from the legacy types
, but it's probably not enough and jsdoc is lacking.
kibana/src/legacy/server/saved_objects/saved_objects_mixin.js
Lines 81 to 82 in 358d139
const service = { | |
types: visibleTypes, |
It shouldn't be a problem for a plugin to access / expose all types to a consumer. AFAIK, hidden types were already consumed in legacy, but for different usages, via the schema
property that contains both visible and hidden types. Also I think some consumers need to access all types, even if I can't remember exactly for which usages, probably security/spaces)
However I agree that 1/ the jsdoc for getAllTypes
should be more explicit, and 2/ the type registry should have a getVisibleTypes
method.
I will create a issue for that improvement
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.
created #66818
@elasticmachine merge upstream |
1 similar comment
@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.
Endpoint change 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.
LGTM
`); | ||
}); | ||
|
||
it('returns OSS + registered features with timielion when available', async () => { |
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.
typo: timielion
const registry = savedObjects.getTypeRegistry(); | ||
const savedObjectTypes = registry | ||
.getAllTypes() | ||
.filter(t => !t.hidden) |
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 named it getAllTypes
in an attempt to distinguish it from the legacy types
, but it's probably not enough and jsdoc is lacking.
kibana/src/legacy/server/saved_objects/saved_objects_mixin.js
Lines 81 to 82 in 358d139
const service = { | |
types: visibleTypes, |
It shouldn't be a problem for a plugin to access / expose all types to a consumer. AFAIK, hidden types were already consumed in legacy, but for different usages, via the schema
property that contains both visible and hidden types. Also I think some consumers need to access all types, even if I can't remember exactly for which usages, probably security/spaces)
However I agree that 1/ the jsdoc for getAllTypes
should be more explicit, and 2/ the type registry should have a getVisibleTypes
method.
I will create a issue for that improvement
e6b497c
to
0b45f68
Compare
@@ -37,7 +35,8 @@ export function defineRoutes({ router, featureRegistry, getLegacyAPI }: RouteDef | |||
request.query.ignoreValidLicenses || | |||
!feature.validLicenses || | |||
!feature.validLicenses.length || | |||
getLegacyAPI().xpackInfo.license.isOneOf(feature.validLicenses) | |||
(context.licensing!.license.type && |
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.
Did you want the !
or the ?
type here? Isn't ?
the safe one where instead of turning off the type assertion it will resolve all of this as null/undefined rather than causing a null pointer exception.
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 want !
here. License is always available, otherwise features
plugin won't start without licensing
plugin. There is no way in our typings to mark a field as required.
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 from a representative of SIEM, just left one comment about the bang operator but I am fine if it runs correctly and tests pass so that is an optional question I left.
* register OSS features with KP SO types only * use Licensing plugin API in features plugin * add plugin tests * filter hidden types out * cleanup tests * rename * add degug logging * add warning for setup contract * fix typo
* master: (191 commits) [Maps] Get number of categories from palette (elastic#66454) move oss features registration to KP (elastic#66524) [kbn/plugin-helpers] typescript-ify (elastic#66513) Add kibana-operations as codeowners for .ci/es-snapshots and vars/ (elastic#66746) FTR: move basic services under common folder (elastic#66563) Migrate Beats Management UI to KP (elastic#65791) [CI] Add 20 minutes to overall build timeout lint import from restricted zones for export exressions (elastic#66588) [SIEM][Detection Engine] Add validation for Rule Actions (elastic#63332) KP plugins shouldn't need package.json (elastic#66654) Replace agent metrics link with the new one (elastic#66632) [CI] Add one retry to setup step (elastic#66638) [CI] Add slack alerts to tracked branch jobs, change default channel, change formatting (elastic#66580) [docLinks] Add docLinks to CoreSetup. (elastic#66631) [DOCS] Rename monitoring collection from internal to legacy (elastic#65781) unskip newsfeed tests (elastic#66562) [NP] Migrate uiSettings owned by Kibana app (elastic#64321) [ML] Functional tests - stabilize typing in DFA mml input (elastic#66706) [Map] return bounding box for static feature collection without joins (elastic#66607) remove trailing slash in graph sample data links (elastic#66358) ...
…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
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-intake-agent / Jest Integration Tests.packages/kbn-optimizer/src/integration_tests.builds expected bundles, saves bundle counts to metadataStandard Out
Stack Trace
Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/uptime/public/components/common/charts/__tests__.PingHistogram component renders the component without errorsStandard Out
Stack Trace
Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/uptime/public/components/overview/monitor_list/__tests__.MonitorList component renders the monitor listStandard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
Summary
Moves OSS feature registration to the KP.
Fix #65461
Checklist
Delete any items that are not applicable to this PR.
For maintainers
Dev Docs
Fixed bug not allowing
Features
plugingetFeatures
method to be invoked within the Start lifecycle.