[Reporting] Use ES plugin from NP #56209
Conversation
tsullivan
added
0sp
zDeprecated Feature:Reporting
|
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
|
@elasticmachine merge upstream |
|
Ready for review :) |
|
Ack: will review tomorrow |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
joelgriffith
left a comment
joelgriffith
left a comment
There was a problem hiding this comment.
LGTM, reviewed code only
pgayvallet
left a comment
pgayvallet
left a comment
There was a problem hiding this comment.
Some NITs regarding test mocks, overall LGTM
| const mockElasticsearch = { | ||
| dataClient: { | ||
| asScoped: () => ({ callAsCurrentUser: jest.fn() }), | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
It's recommended to use the provided mocks when mocking core services. (multiple occurences, will comment only once)
import { elasticsearchServiceMock } from '[..]/src/core/server/mocks';
const mockElasticsearch = elasticsearchServiceMock.createSetupContractMock()| import * as Rx from 'rxjs'; | ||
| import { ElasticsearchServiceSetup } from 'kibana/server'; | ||
| import { catchError, map, mergeMap, takeUntil } from 'rxjs/operators'; |
There was a problem hiding this comment.
NIT: kibana/server import should probably be after node modules imports
| await plugin.setup(coreSetup, { | ||
| elasticsearch: coreSetup.elasticsearch, | ||
| security: server.newPlatform.setup.plugins.security as SecurityPluginSetup, | ||
| usageCollection: server.newPlatform.setup.plugins.usageCollection, | ||
| }); |
There was a problem hiding this comment.
Redundant parameters: elasticsearch is already provided by coreSetup, elasticsearch: coreSetup.elasticsearch, seems unnecessary.
There was a problem hiding this comment.
The params are forced to fit the ReportingStartDeps interface, which is used all over the place to pull the ES service. Unfortunately it seems to make better sense to keep it this way, but I could see it changing later on.
| getMockServer(), | ||
| {} as ElasticsearchServiceSetup, | ||
| getMockLogger(), |
There was a problem hiding this comment.
NIT: cf. previous comment, using core mocks is preferable to force-casting
|
@pgayvallet regarding the need for using mocks from core: I agree ultimately that would be a good refactoring improvement to the tests. This particular area of code feels pretty short-lived though, soon @joelgriffith and I will be heavily changing the queuing systems to replace with ESQueue, and further on I really think we can remove the abstraction of "export types" from Reporting if we're able to make the Reporting plugin have complete focus on capturing screenshots. Right now it juggles both screenshotting a Kibana URL and exporting CSV from a saved search. |
* [Reporting] Use ES plugin from NP * fix elasticsearchErrors reference * fix mocha test * convert to jest * fix the code and tests * cosmetics * fix mocha tests * fix imports * fix mocha tests * fix jest * simplify Co-authored-by: Elastic Machine <[email protected]>
* [Reporting] Use ES plugin from NP (#56209) * [Reporting] Use ES plugin from NP * fix elasticsearchErrors reference * fix mocha test * convert to jest * fix the code and tests * cosmetics * fix mocha tests * fix imports * fix mocha tests * fix jest * simplify Co-authored-by: Elastic Machine <[email protected]> * fix eslint * fix missing import * fix the backport * fix mocha test Co-authored-by: Elastic Machine <[email protected]>
5 participants
Summary
#53898
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibility