-
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
[Uptime] Create state store at plugin setup time #66054
Conversation
Pinging @elastic/uptime (Team:uptime) |
It would be really nice if someone from @elastic/kibana-platform could let me know if this is an anti-pattern or otherwise evil hack that violates the principles of Kibana platform design. If so, an alternative suggestion would be highly appreciated. |
06c8a0f
to
c2ffa34
Compare
c2ffa34
to
6bb5519
Compare
|
||
alertTypeInitializers.forEach(init => { | ||
const alertInitializer = init({ | ||
autocomplete: this._data!.autocomplete, |
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 may not work correctly in a future release. We plan to lock down the setup
APIs to only be callable during the setup
phase. If you just need data.autocomplete.getQuerySuggestions
, that is also available on start
.
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.
Yeah I guess in general, I'm not sure why these initializers are called in start
rather than setup? Seems like it all depends on setup APIs?
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 can perhaps be moved inside component by using useKibana hook.
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.
@joshdover can you have a look at e5a1eb3 when you've got a chance? I've moved the registration back to setup
and removed instance fields for setup APIs.
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, that looks better 👍
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.
@joshdover I'm actually actively working on resolving more issues with this branch, but they're related to alerting. One thing we need is core.services.application.getUrlForApp
from CoreStart
. I've moved this back to start
, but triggers_actions_ui
is available on the start plugins param, and autocomplete
is available as well (like you've mentioned).
I'm not referencing anything from setup
now though. Can you verify this is still ok? I think once we've ironed that out it should be the end of platform-specific concerns. bf16f6a
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 wonder if we should reconsider how this whole thing is designed. Why is that the Redux store needs to exist at all? Is it possible we could decouple the UI from the store to make it more portable?
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/plugins/uptime/public/lib/alert_types/__tests__.monitor status alert type validate doesn't throw on empty setStandard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack Endpoint Functional Tests.x-pack/test/functional_endpoint/apps/endpoint/policy_list·ts.endpoint When on the Endpoint Policy List and policies exists "before all" hook for "should show policy on the list"Standard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack Endpoint Functional Tests.x-pack/test/functional_endpoint/apps/endpoint/policy_list·ts.endpoint When on the Endpoint Policy List and policies exists "before all" hook for "should show policy on the list"Standard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
this is not needed anymore |
Summary
I am hoping this change resolves elastic/uptime#179.
This patch moves the creation of our Redux store to the
setup
function of our plugin. We'd pass our UI alert types to the Alerting UI from ourstart
function, wrapping the components we supply in the necessary context providers to interact with the store.Checklist
Delete any items that are not applicable to this PR.
For maintainers