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

Pass scoped context to tutorial providers when building tutorials #22260

Merged
merged 4 commits into from
Sep 5, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 22, 2018

Part of the solution to Support for Kibana Spaces in Beats modules.

Talked through a solution with @elastic/kibana-security. The conversation revolved around providing a way for plugins to decorate tutorials by registering a contextFactory. The contextFactory is a function that is called with the request and returns an object of properties.
tutorialSpecProvider will now be called with an additional parameter, context, which contains the output of all registered contextFactories merged into a single object. tutorialSpecProvider can use the context object and its properties to conditionally modify the generated tutorial spec.

In the cases of spaces, the spaces plugin will decorate tutorials with a contextFactory that returns the spaceId. So when the tutorialSpecProvider is called, if spaces plugin is running and the request is within a space, then the context will include { spaceId: "value"}

@ycombinator @ruflin @elastic/kibana-sharing @alexfrancoeur

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Aug 22, 2018

flaky visualize test

fail: "visualize app data table with index without time filter "after all" hook"
19:08:35              │      [GET http://localhost:9515/session/2f5693819948083ed1f3978b0ad5dd23/url] connect ECONNREFUSED 127.0.0.1:9515

@nreese
Copy link
Contributor Author

nreese commented Aug 22, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ruflin
Copy link
Member

ruflin commented Aug 23, 2018

For my understanding: After this PR is merged the space plugin still needs to be modified to register the contextFactory?

@kobelb
Copy link
Contributor

kobelb commented Aug 23, 2018

For my understanding: After this PR is merged the space plugin still needs to be modified to register the contextFactory?

That's correct, this is focused on the architecture to allow the plugins to register additional information that can be used within the tutorials.

return _.cloneDeep(tutorials);
server.decorate('server', 'getTutorials', (request) => {
const initialContext = {};
const scopedContext = scopedTutorialContextFactoryies.reduce((accumulatedContext, contextFactory) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the plural of factory is factories.

server.decorate('server', 'getTutorials', (request) => {
const initialContext = {};
const scopedContext = scopedTutorialContextFactoryies.reduce((accumulatedContext, contextFactory) => {
return { ...accumulatedContext, ...contextFactory(request) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw an error if we have two context factories try to set the same values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong preference either way, so LGTM!

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ycombinator
Copy link
Contributor

jenkins, test this pretty please with sugar on top

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese merged commit b26e2b4 into elastic:master Sep 5, 2018
nreese added a commit to nreese/kibana that referenced this pull request Sep 5, 2018
…astic#22260)

* Pass scoped context to tutorial providers when building tutorials

* only generated scoped context one time

* spelling
nreese added a commit that referenced this pull request Sep 5, 2018
…2260) (#22743)

* Pass scoped context to tutorial providers when building tutorials

* only generated scoped context one time

* spelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Add Data Add Data and sample data feature on Home v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants