-
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
Alerting plugin migrate to Kibana platform #57635
Alerting plugin migrate to Kibana platform #57635
Conversation
…rate-to-kibana-platform # Conflicts: # .github/CODEOWNERS # x-pack/legacy/plugins/alerting/common/alert.ts # x-pack/legacy/plugins/alerting/common/types.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/update_prepacked_rules.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/update_rules.ts # x-pack/plugins/alerting/common/types.ts # x-pack/plugins/alerting/server/alerts_client.ts # x-pack/plugins/alerting/server/task_runner/alert_task_instance.ts # x-pack/plugins/alerting/server/types.ts
…rate-to-kibana-platform # Conflicts: # x-pack/legacy/plugins/alerting/server/plugin.ts
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
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 for sorting the git mv 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.
Change LGTM! Just one nit.
I would maybe hold off merging this until we merge #57524 (to avoid merge conflicts).
x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts
Outdated
Show resolved
Hide resolved
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, except for (I think) a missing if (!this.isInitialized)
in alerts_client_factory.ts
. Made some other nit-level comments.
Learned a bunch of new NP and TS tricks with this as well!
private readonly spaceIdToNamespace: SpaceIdToNamespaceFunction; | ||
private readonly encryptedSavedObjectsPlugin: EncryptedSavedObjectsPluginStart; | ||
private isInitialized = false; | ||
private logger!: Logger; |
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.
Today I learned about using !
on properties like logger!
! Wonder what @gmmorris thinks of that!
? I'm happy with it for now, we seem to have multiple patterns to create plugin internal and exposed objects during setup/start, long-term perhaps we can settle on one.
const { securityPluginSetup } = this; | ||
const spaceId = this.getSpaceId(legacyRequest); | ||
const spaceId = this.getSpaceId(request); |
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 think we want a if (!this.isInitialized) throw new Error('not initialized')
in here.
}; | ||
} | ||
|
||
private createRouteHandlerContext = (): IContextProvider< |
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.
cool - hadn't seen a real use of this yet. I wonder if we could move more stuff in here over time, like the license check, but that's for another PR :-)
x-pack/plugins/alerting/server/routes/_mock_handler_arguments.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/create.ts
Outdated
Show resolved
Hide resolved
…igrate-to-kibana-platform # Conflicts: # x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/_mock_server.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_existing_prepackaged_rules.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_export_all.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/get_export_by_object_ids.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/read_rules.test.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/types.ts # x-pack/legacy/plugins/siem/server/lib/detection_engine/tags/read_tags.test.ts
…igrate-to-kibana-platform # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…igrate-to-kibana-platform # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
Migrates the Alerting plugin from Legacy on to the Kibana Platform.
Closes #51652
Checklist
Delete any items that are not applicable to this PR.