Skip to content
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

Remove Angular from courier request handler #20032

Merged
merged 6 commits into from Jun 26, 2018

Conversation

Projects
None yet
5 participants
@ppisljar
Copy link
Member

ppisljar commented Jun 19, 2018

removes all angular dependencies from courier request handler

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 19, 2018

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from cbc33f4 to 36e2bc1 Jun 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 20, 2018

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from 36e2bc1 to 0d8e145 Jun 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 20, 2018

@ppisljar

This comment has been minimized.

Copy link
Member Author

ppisljar commented Jun 20, 2018

jenkins, test this

@ppisljar ppisljar added review chore and removed WIP labels Jun 20, 2018

@ppisljar ppisljar requested review from timroes and markov00 Jun 20, 2018

@ppisljar ppisljar changed the title decoupling courier request handler from vis removes angular from courier request handler Jun 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 21, 2018

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from 22be864 to 3a8a2c7 Jun 21, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 21, 2018

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from 3a8a2c7 to baa38a5 Jun 21, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 21, 2018

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from 1a3f0be to 4fe22b6 Jun 22, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 22, 2018

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from 4fe22b6 to 60090ba Jun 22, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 22, 2018

@timroes

This comment has been minimized.

Copy link
Contributor

timroes commented Jun 25, 2018

Jenkins, test this

if (_.has(agg, 'type.postFlightRequest')) {
const nestedSearchSource = new SearchSource().inherits(requestSearchSource);
resp = await agg.type.postFlightRequest(resp, vis.aggs, agg, nestedSearchSource);
const nestedSearchSource = requestSearchSource.makeChild();

This comment has been minimized.

Copy link
@timroes

timroes Jun 25, 2018

Contributor

Just to note that down: this will change the behavior slightly, since we now call all parents start handler (i.e. the one of requestSearchSource in case this nestedSearchSource will receive the requestIsStarting. I am not yet sure if that actually will ever happen of what that influences, but wanted to point it out for now.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 25, 2018

@timroes

This comment has been minimized.

Copy link
Contributor

timroes commented Jun 25, 2018

Jenkins, test this

@@ -184,7 +184,7 @@ export function CoordinateMapsVisualizationProvider(Notifier, Private) {
async getGeohashBounds() {
const agg = this._getGeoHashAgg();
if (agg) {
const searchSource = this.vis.API.createInheritedSearchSource(this.vis.searchSource);
const searchSource = this.vis.searchSource.makeChild();

This comment has been minimized.

Copy link
@timroes

timroes Jun 25, 2018

Contributor

The same here as below (yeah sorry for the mixed up order). We'll change the behavior here slightly, by calling the parent searchsource start handler when calling this. The handler will eventually call the modifyAggConfigOnSearchRequestStart of the individual aggregations. I am not sure if we want this to happen in this place already (and the one below). It seems to work fine, but I am wondering about some weird situations we might end up with, where we should not call that method "out of place". Maybe we could give the makeChild method an options, parameter to pass to the inherit method, so we can only enable callParentStartHandlers in the one place in courier request handler where it was also enabled beforehand?

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 26, 2018

@timroes

This comment has been minimized.

Copy link
Contributor

timroes commented Jun 26, 2018

Jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 26, 2018

@ppisljar

This comment has been minimized.

Copy link
Member Author

ppisljar commented Jun 26, 2018

jenkins, test this

@ppisljar

This comment has been minimized.

Copy link
Member Author

ppisljar commented Jun 26, 2018

jenkins test this, x-pack random test fail

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 26, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 26, 2018

💚 Build Succeeded

@timroes timroes changed the title removes angular from courier request handler Remove Angular from courier request handler Jun 26, 2018

@timroes
Copy link
Contributor

timroes left a comment

LGTM, tested (Chrome, Linux) and it seems not to break anything

ppisljar added some commits Jun 19, 2018

remove angular dependency from courier request handler
docouple courier request handler from vis
ups

@ppisljar ppisljar force-pushed the ppisljar:fix/variuousVis branch from d5f9413 to 6188a4b Jun 26, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 26, 2018

@ppisljar ppisljar merged commit 28402d3 into elastic:master Jun 26, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

ppisljar added a commit to ppisljar/kibana that referenced this pull request Jun 26, 2018

@ppisljar ppisljar deleted the ppisljar:fix/variuousVis branch Jun 26, 2018

ppisljar added a commit that referenced this pull request Jun 27, 2018

@ppisljar ppisljar restored the ppisljar:fix/variuousVis branch Sep 26, 2018

@@ -88,7 +87,6 @@ export function VisProvider(Private, Promise, indexPatterns, getAppState) {
indexPatterns: indexPatterns,
timeFilter: timefilter,
queryFilter: queryFilter,
queryManager: queryManagerFactory(getAppState),

This comment has been minimized.

Copy link
@tsullivan

tsullivan Oct 9, 2018

Contributor

Whoops! vis.API.queryManager is a documented Vis API feature: https://www.elastic.co/guide/en/kibana/current/development-vis-object.html Removing this broke integration for plugin authors.

Should this be re-added?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.