Skip to content

Making schema async, and plugin discovery expose raw package jsons#18926

Merged
kobelb merged 5 commits intoelastic:masterfrom
kobelb:native-controllers-plugin-discovery
May 8, 2018
Merged

Making schema async, and plugin discovery expose raw package jsons#18926
kobelb merged 5 commits intoelastic:masterfrom
kobelb:native-controllers-plugin-discovery

Conversation

@kobelb
Copy link
Copy Markdown
Contributor

@kobelb kobelb commented May 8, 2018

Subset of changes introduced by https://github.com/elastic/kibana/pull/17496/files that don't include the introduction of native controllers.

@kobelb kobelb requested a review from spalger May 8, 2018 18:34
// only emitted once it is fully extended by all
extendedConfig$: extendConfig$
.ignoreElements()
.concat([config]),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of .concat() here was to wait until extendConfig$ completes and config$ is fully extended before emitting. If the idea here is to subscribe to config$ before extendConfig$ completes, perhaps .combineLatest(extendConfig$.last(), config$).map(x => x[1]) is more what we want.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we just emit the config from the extendConfig$? I pushed a commit that does just this, lemme know what you think.


export const createPack$ = (packageJson$) => (
packageJson$
.mergeMap(({ error, packageJson }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.map() should be sufficient here, and I suggest making this a bit more resilient by either checking specifically for result.packageJson and either forwarding any result that doesn't have a packageJson property or throwing an informative error when an unexpected result is received.

}

return await defaultConfig(settings);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config$ is subscribed to multiple times but not shared.

const extraPaths = [
...settings.plugins.paths,
...settings.plugins.scanDirs,
...config.get('plugins.paths'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse pluginPaths var?

extendedConfig$: extendConfig$
.ignoreElements()
.concat([config]),
.mergeMap(result => result.config),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably include .filter(Boolean).last() here, and might as well add a test that verifies that a single config instance is emitted and that it has the extensions from all plugins when emitted.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented May 8, 2018

Jenkins, test this

CI failure appears to be the headless browser dying.

Copy link
Copy Markdown
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming tests pass in CI

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@kobelb
Copy link
Copy Markdown
Contributor Author

kobelb commented May 8, 2018

Jenkins, test this

Another unrelated CI failure with the following flaky test:

test/functional/apps/dashboard/_dashboard_filtering·js.dashboard app using current data dashboard filtering nested filtering visualization saved with a query filters data

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@kobelb kobelb merged commit 2682bc5 into elastic:master May 8, 2018
@kobelb kobelb deleted the native-controllers-plugin-discovery branch May 8, 2018 23:14
kobelb added a commit to kobelb/kibana that referenced this pull request May 8, 2018
…lastic#18926)

* Making schema async, and plugin discovery expose raw package jsons

* Addressing some peer review comments

* Modifying the way we emit the extendedConfig

* Removing errant config

* Adding filter and last so we only get the last non-null one
@kobelb kobelb added v7.0.0 v6.4.0 Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels May 8, 2018
kobelb added a commit that referenced this pull request May 9, 2018
…18926) (#18940)

* Making schema async, and plugin discovery expose raw package jsons

* Addressing some peer review comments

* Modifying the way we emit the extendedConfig

* Removing errant config

* Adding filter and last so we only get the last non-null one
@spalger
Copy link
Copy Markdown
Contributor

spalger commented May 9, 2018

6.x/6.4: c9cb810

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v6.4.0 v7.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants