-
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
[Remote clusters] Migrate client-side code out of legacy #57365
[Remote clusters] Migrate client-side code out of legacy #57365
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@@ -77,7 +77,7 @@ describe('Create Remote cluster', () => { | |||
beforeEach(async () => { | |||
({ component, form, actions } = setup()); | |||
|
|||
await nextTick(); | |||
await nextTick(100); |
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 needed to adjust the timeout when switching from axios
to the http service
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.
Was the issue the same in each instance where nextTick was added? Just wondering whether it is worth putting a comment in each time.
Not a big deal though :)
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.
Good point! I may have gotten carried away 😂. It actually is not needed for this specific line, but I updated the code where it was needed with a comment.
} = core; | ||
|
||
// Initialize services | ||
initBreadcrumbs(setBreadcrumbs); |
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.
@jloleysens This is the type of convention I was trying to get to. In my apps I am migrating I am instantiating the service inside the plugin setup()
cycle.
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 guess in this pattern we would also not want the stateful services to be accessed via import in downstream code (as it currently is), e.g.,
let _setBreadcrumbs: (breadcrumbs: Breadcrumb[]) => void; |
More asking 👆🏻 :)
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.
Correct, these are still being accessed via import. I was on the fence to whether it made sense to refactor as part of this migration PR. I think it might be better to open up a separate PR once this is merged and move to context if that is the pattern we want to follow.
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.
These changes look great to me! Always good to see a PR that is "in the red" 😉
Left a question regarding tests, not a blocker.
@@ -77,7 +77,7 @@ describe('Create Remote cluster', () => { | |||
beforeEach(async () => { | |||
({ component, form, actions } = setup()); | |||
|
|||
await nextTick(); | |||
await nextTick(100); |
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.
Was the issue the same in each instance where nextTick was added? Just wondering whether it is worth putting a comment in each time.
Not a big deal though :)
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…re/files-and-filetree * 'master' of github.com:elastic/kibana: (139 commits) Move Ace XJSON lexer-rules, worker and utils to es_ui_shared (elastic#57563) [Upgrade Assistant] Fix filter deprecations search filter (elastic#57541) [ML] New Platform server shim: update indices routes (elastic#57685) Bump redux dependencies (elastic#53348) [Index management] Client-side NP ready (elastic#57295) change id of x-pack event_log plugin to eventLog (elastic#57612) [eventLog] get kibana.index name from config instead of hard-coding it (elastic#57607) revert allow using any path to generate fixes ui titles (elastic#57535) Fix login redirect for expired sessions (elastic#57157) Expose Vis on the contract as it requires visTypes (elastic#56968) [SIEM][Detection Engine] Fixes queries to ignore errors when signals index is not present [Remote clusters] Migrate client-side code out of legacy (elastic#57365) Fix failed test reporter for SIEM Cypress use (elastic#57240) skip flaky suite (elastic#45244) update chromedriver to 80.0.1 (elastic#57602) change slack action to only report on whitelisted host name (elastic#57582) [kbn/optimizer] throw errors into stream on invalid completion (elastic#57735) moving visualize/utils to new platform (elastic#56650) ...
This PR mostly completes the NP migration for the
remote_clusters
plugin.Continuation of #56781.
What's left
How to test
You will need to set up two clusters in order to test remote clusters. For example:
xpack.remote_clusters.enabled
isfalse