[ML] Management: fix license unsubscribe#59365
[ML] Management: fix license unsubscribe#59365alvarezmelissa87 merged 2 commits intoelastic:masterfrom
Conversation
alvarezmelissa87
added
:ml
release_note:skip
|
Pinging @elastic/ml-ui (:ml) |
There was a problem hiding this comment.
Is it a valid expectation for the licensingSubscription observable to end up being undefined? If it's undefined because of some other error higher up, we log the error where it's happening. It feels like this is a side-effect of something else, but we don't know what.
There was a problem hiding this comment.
From looking at observable docs - looks like unsubscribing within a subscribe callback in an observable is not the recommended way of doing it. I'd imagine that it is possible since the subscription wouldn't have returned yet when this is first hit - so it could be undefined.
I've confirmed that this works so it should unblock us. I'm working on a more rxjs-ey solution but didn't want to keep everyone waiting while I tested.
There was a problem hiding this comment.
it's also worth noting that this is legacy code that will be rewritten soon for NP.
License checks where added here to enable ML's management plugin for the time being.
There was a problem hiding this comment.
Thanks for the confirmation that there will be future work done to improve this part of the code. I'm OK with the changes as they are to get folks unblocked.
alvarezmelissa87
requested review from
joelgriffith and
wylieconlon
and removed request for
wylieconlon
alvarezmelissa87
force-pushed
the
ml-fix-licensing-issue
branch
from
91e4167 to
30696a2
Compare
|
Updated this as it needed an unrelated fix in master - I've updated to use |
| const plugins = npSetup.plugins as PluginsSetupExtended; | ||
| const licencingSubscription = plugins.licensing.license$.subscribe(license => { | ||
| // only need to register once | ||
| const licensing = plugins.licensing.license$.pipe(take(1)); |
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
Assuming tests pass, this LGTM
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* check for undefined before unsubscribe.remove skip from test * use take for observable
…re/files-and-filetree * 'master' of github.com:elastic/kibana: (254 commits) Convert discover_page to ts, remove redundunt methods (elastic#59312) [Fix for Vis Editor] Revert setting time field to empty string when it's undefined (elastic#58873) Delete legacy search endpoint (elastic#59341) [Uptime] Improve duration chart (elastic#58404) [Snapshot & Restore] NP migration (elastic#59109) [ML] Add support for date_nanos time field in anomaly job wizard (elastic#59017) Revert "Makes alerting and actions optional properties for interface RequestH… (elastic#59264)" Change remote_clusters ID to remoteClusters (elastic#59246) Makes alerting and actions optional properties for interface RequestH… (elastic#59264) Clean up date histogram agg type. (elastic#58805) [ML] Management: fix license unsubscribe (elastic#59365) Remove documentation for server.cors settings (elastic#59096) Edit alert flyout (elastic#58964) [SIEM] Fix rule delete/duplicate actions (elastic#59306) move mouse to close obstructing tooltip (elastic#59214) Reset page after deleting (elastic#59310) Make sure phrases input filter triggers autosuggestons (elastic#59299) Add loading count source for http requests (elastic#59245) Revert "[ML] Transforms: Deprecate custom KibanaContext. (elastic#59133)" Expose metrics service to public API (elastic#59294) ... # Conflicts: # src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
7 participants
Summary
Fixes error #59229 (comment)
Uses
taketo limit subscription to first value. Doesn't require call to unsubscribe.Removes 'skip' from test previously thought to be flaky