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

[home] Include ability to publish kibana saved objects from add data tutorial #19559

Merged
merged 29 commits into from
Jul 18, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b0f6537
add savedObjects to tutorial schema, add savedObjects to APM, add bul…
nreese May 30, 2018
a63831a
SavedObjectInstaller component
nreese May 30, 2018
6decd9d
bulkCreate fixes
nreese May 30, 2018
6eaeb6a
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jun 19, 2018
c3cd774
fix tutorial jest test
nreese Jun 19, 2018
23928e0
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jun 20, 2018
bc739f5
update from sqren review
nreese Jun 20, 2018
822627c
updated copy
nreese Jun 20, 2018
7f9c845
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jun 25, 2018
c73f6e9
move saved object json into seperate json files
nreese Jun 25, 2018
63db6f9
minor commit clean up
nreese Jun 25, 2018
fc2064d
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jun 28, 2018
560b4a9
ensure isMounted before setting state after async call, allow manifes…
nreese Jun 28, 2018
9ff43d3
remove duplicated logic for getting config xpack.apm.indexPattern
nreese Jun 28, 2018
e5000e3
refactor get index pattern title
nreese Jun 28, 2018
7bc56e1
add functional test that loads APM saved objects
nreese Jun 28, 2018
232a760
remove extra await
nreese Jun 28, 2018
12e29b3
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jul 5, 2018
ef0be0e
merge with master - master now has bulkCreate
nreese Jul 10, 2018
6175825
display overwrite message
nreese Jul 10, 2018
cb3f8de
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jul 10, 2018
8ea193d
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jul 13, 2018
e037c91
use angular free savedObjectClient
nreese Jul 13, 2018
e0cb81b
Merge branch 'master' of https://github.com/elastic/kibana into load_…
nreese Jul 17, 2018
66faeb3
functional test cleanup
nreese Jul 17, 2018
a4ad403
handle bulkRequest exception and add jest tests for SavedObjectsInsta…
nreese Jul 17, 2018
c0e25ad
use Promise.reject instead of throw
nreese Jul 17, 2018
7cd8938
update copy
nreese Jul 17, 2018
7e108c0
merge with master
nreese Jul 18, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/core_plugins/kibana/common/tutorials/tutorial_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,7 @@ export const tutorialSchema = {

// Elastic stack artifacts produced by product when it is setup and run.
artifacts: artifactsSchema,

// saved objects used by data module.
savedObjects: Joi.array().items(),
};
3 changes: 3 additions & 0 deletions src/core_plugins/kibana/public/home/components/home_app.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export function HomeApp({
getConfig,
setConfig,
clearIndexPatternsCache,
bulkCreate,
}) {

const isCloudEnabled = chrome.getInjected('isCloudEnabled', false);
Expand All @@ -66,6 +67,7 @@ export function HomeApp({
getTutorial={getTutorial}
replaceTemplateStrings={replaceTemplateStrings}
tutorialId={props.match.params.id}
bulkCreate={bulkCreate}
/>
);
};
Expand Down Expand Up @@ -119,4 +121,5 @@ HomeApp.propTypes = {
getConfig: PropTypes.func.isRequired,
setConfig: PropTypes.func.isRequired,
clearIndexPatternsCache: PropTypes.func.isRequired,
bulkCreate: PropTypes.func.isRequired,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import React, { Fragment } from 'react';
import PropTypes from 'prop-types';

import {
EuiSteps,
EuiFlexGroup,
EuiFlexItem,
EuiText,
EuiButton,
EuiSpacer,
EuiCallOut,
} from '@elastic/eui';

const INCOMPLETE = 'incomplete';
const COMPLETE = 'complete';

export class SavedObjectsInstaller extends React.Component {
constructor(props) {
super(props);

this.state = {
isInstalling: false,
installStatus: INCOMPLETE,
Copy link
Member

Choose a reason for hiding this comment

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

For consistency installStatus should be a boolean like isInstalling (and named isComplete).

};
}
Copy link
Member

@sorenlouv sorenlouv May 31, 2018

Choose a reason for hiding this comment

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

If you don't need the constructor for anything else, you can set the shorthand:

export class SavedObjectsInstaller extends React.Component {
    state = {
        isInstalling: false,
        installStatus: INCOMPLETE,
    };
}


installSavedObjects = async () => {
this.setState({
isInstalling: true,
});

const resp = await this.props.bulkCreate(this.props.savedObjects, { overwrite: true });
const errors = resp.savedObjects.filter(savedObjectCreateResult => {
return savedObjectCreateResult.hasOwnProperty('error');
});

let statusMsg = `${this.props.savedObjects.length} saved objects successfully added`;
if (errors.length > 0) {
statusMsg = `Unable to load kibana saved objects, Error: ${errors[0]}`;
}
Copy link
Member

@sorenlouv sorenlouv May 31, 2018

Choose a reason for hiding this comment

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

I prefer const since it gives the assurance that this variable won't be reassigned later. If you use a ternary you can avoid let.


this.setState({
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to use the isMounted check as this component may have been unmounted due to the async call:

warning.js:33 Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in SavedObjectsInstaller (created by Tutorial)
    in div (created by EuiPanel)
    in EuiPanel (created by Tutorial)
    in div (created by Tutorial)
    in div (created by EuiPage)
    in EuiPage (created by Tutorial)
    in Tutorial (created by Route)
    in Route (created by HomeApp)
    in Switch (created by HomeApp)
    in Router (created by HashRouter)
    in HashRouter (created by HomeApp)
    in HomeApp

isInstalling: false,
installStatusMsg: statusMsg,
installStatus: errors.length === 0 ? COMPLETE : INCOMPLETE,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having two different checks for error (errors.length === 0 and errors.length > 0) maybe just have one const hasError = errors.length > 0.

This way they won't somehow come out of sync in a future change.

});
}

renderInstallMessage() {
if (!this.state.installStatusMsg) {
return;
}

return (
<EuiCallOut
title={this.state.installStatusMsg}
color={this.state.installStatus === COMPLETE ? 'success' : 'warning'}
/>
);
}

renderInstallStep() {
const installStep = (
<Fragment>
<EuiFlexGroup justifyContent="spaceBetween" alignItems="center">
<EuiFlexItem>
<EuiText>
<p>
Click button to add Saved Searches, Visualizations, and Dashboards for module
</p>
</EuiText>
</EuiFlexItem>

<EuiFlexItem
grow={false}
>
<EuiButton
onClick={this.installSavedObjects}
isLoading={this.state.isInstalling}
>
Add
</EuiButton>
</EuiFlexItem>
</EuiFlexGroup>

<EuiSpacer size="s" />

{this.renderInstallMessage()}
</Fragment>
);

return {
title: 'Load Kibana objects',
status: this.state.installStatus,
Copy link
Member

Choose a reason for hiding this comment

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

If status only has two modes, I think it would be better to make it a boolean.

children: installStep,
key: 'installStep'
};
}

render() {
return (
<EuiSteps
steps={[this.renderInstallStep()]}
/>
);
}
}

const savedObjectShape = PropTypes.shape({
id: PropTypes.string.isRequired,
type: PropTypes.string.isRequired,
attributes: PropTypes.object.isRequired,
});

SavedObjectsInstaller.propTypes = {
bulkCreate: PropTypes.func.isRequired,
savedObjects: PropTypes.arrayOf(savedObjectShape).isRequired,
};
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { Footer } from './footer';
import { Introduction } from './introduction';
import { InstructionSet } from './instruction_set';
import { RadioButtonGroup } from './radio_button_group';
import { SavedObjectsInstaller } from './saved_objects_installer';
import { EuiSpacer, EuiPage, EuiPanel, EuiLink, EuiText } from '@elastic/eui';
import * as StatusCheckStates from './status_check_states';

Expand Down Expand Up @@ -235,6 +236,20 @@ export class Tutorial extends React.Component {
});
};

renderSavedObjectsInstaller = () => {
if (!this.state.tutorial.savedObjects) {
return;
}

return (
<SavedObjectsInstaller
bulkCreate={this.props.bulkCreate}
savedObjects={this.state.tutorial.savedObjects}
/>
);

}

renderFooter = () => {
let label;
let url;
Expand Down Expand Up @@ -305,6 +320,7 @@ export class Tutorial extends React.Component {
<EuiSpacer />
<EuiPanel paddingSize="l">
{this.renderInstructionSets(instructions)}
{this.renderSavedObjectsInstaller()}
{this.renderFooter()}
</EuiPanel>
</div>
Expand All @@ -327,4 +343,5 @@ Tutorial.propTypes = {
getTutorial: PropTypes.func.isRequired,
replaceTemplateStrings: PropTypes.func.isRequired,
tutorialId: PropTypes.string.isRequired,
bulkCreate: PropTypes.func.isRequired,
};
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe('isCloudEnabled is false', () => {
getTutorial={getTutorial}
replaceTemplateStrings={replaceTemplateStrings}
tutorialId={'my_testing_tutorial'}
bulkCreate={() => {}}
/>);
loadTutorialPromise.then(() => {
component.update();
Expand All @@ -95,6 +96,7 @@ describe('isCloudEnabled is false', () => {
getTutorial={getBasicTutorial}
replaceTemplateStrings={replaceTemplateStrings}
tutorialId={'my_testing_tutorial'}
bulkCreate={() => {}}
/>);
loadBasicTutorialPromise.then(() => {
component.update();
Expand All @@ -109,6 +111,7 @@ describe('isCloudEnabled is false', () => {
getTutorial={getTutorial}
replaceTemplateStrings={replaceTemplateStrings}
tutorialId={'my_testing_tutorial'}
bulkCreate={() => {}}
/>);
loadTutorialPromise.then(() => {
component.update();
Expand All @@ -126,6 +129,7 @@ test('should render ELASTIC_CLOUD instructions when isCloudEnabled is true', ()
getTutorial={getTutorial}
replaceTemplateStrings={replaceTemplateStrings}
tutorialId={'my_testing_tutorial'}
bulkCreate={() => {}}
/>);
loadTutorialPromise.then(() => {
component.update();
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kibana/public/home/home_ng_wrapper.html
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@
get-config="getConfig"
set-config="setConfig"
clear-index-patterns-cache="clearIndexPatternsCache"
bulk-create="bulkCreate"
/>
3 changes: 3 additions & 0 deletions src/core_plugins/kibana/public/home/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import chrome from 'ui/chrome';
import routes from 'ui/routes';
import template from './home_ng_wrapper.html';
import { FeatureCatalogueRegistryProvider } from 'ui/registry/feature_catalogue';
import { SavedObjectsClientProvider } from 'ui/saved_objects';
import { uiModules } from 'ui/modules';
import {
HomeApp
Expand All @@ -37,6 +38,7 @@ function getRoute() {
return {
template,
controller($scope, config, indexPatterns, Private) {
const savedObjectsClient = Private(SavedObjectsClientProvider);
$scope.addBasePath = chrome.addBasePath;
$scope.directories = Private(FeatureCatalogueRegistryProvider).inTitleOrder;
$scope.recentlyAccessed = recentlyAccessed.get().map(item => {
Expand All @@ -49,6 +51,7 @@ function getRoute() {
const getter = indexPatterns.getIds;
getter.clearCache();
};
$scope.bulkCreate = savedObjectsClient.bulkCreate;
Copy link
Member

@sorenlouv sorenlouv May 31, 2018

Choose a reason for hiding this comment

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

I think we made a mistake by coupling rest clients to Angular in the past. Instead they should have been framework-agnostic static functions.

If we keep coupling new code (in this case React) to Angular we make it harder to switch away from Angular, and increase technical debt.

It's easier to use an existing client but I don't think the alternative is much more difficult. You don't have to re-implement the entire savedObjectsClient - just create a starting point with the endpoints you need.

Afaict this is all you need:

export function bulkCreate(savedObjects, { overwrite }) {
  return kfetch({
    pathname: `/api/saved_objects/_bulk_create`,
    method: 'POST',
    query: {
      overwrite
    },
    body: JSON.stringify(savedObjects)
  });
}

Copy link
Member

Choose a reason for hiding this comment

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

@epixa Do we already have a strategy for consuming Angular services from outside Angular - and how to migrate them? I've already spent a fair amount of time de-angularizing some services and it would be beneficial if everyone were aligned. If we don't have anything I'm open for doing a write-up of the approach I've used, and how I see us moving forward.

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 do not think this is a problem and is exactly how we are interacting with saved objects in other react components. From react's perspective, it is just an injected function. Eventually, the savedObjectClient will be decoupled from angular but all saved object interaction should go through the savedObjectClient.

Copy link
Member

Choose a reason for hiding this comment

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

but all saved object interaction should go through the savedObjectClient

Maybe I don't know enough about the savedObjectClient. What does it provide wrt bulkCreate, that a static method can't do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The savedObjectClient converts the results into savedObject classes and camel cases the object keys. But more importantly, it puts all calls to the saved object API in a single place so if the API ever changes, only the savedObjectClient has to be updated instead of hunting down lots of segregated fetches.

Copy link
Member

@sorenlouv sorenlouv Jun 2, 2018

Choose a reason for hiding this comment

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

But more importantly, it puts all calls to the saved object API in a single place so if the API ever changes, only the savedObjectClient has to be updated instead of hunting down lots of segregated fetches

Totally agree. IMHO it is okay temporarily to have two "clients": the old Angular client, and a new framework agnostic service. Over time the Angular client could even call the framework agnostic client, and eventually the Angular client would be dropped. That's just my view, but interested to hear what other ideas are floating around (ping @epixa ;) )

Copy link
Member

Choose a reason for hiding this comment

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

Btw. I think it's fine to leave it as is. Just my opinion on how to do it going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not in this PR, but I'd rather just remove the angular dependency from the saved object client we have today. It shouldn't be terribly difficult, we'd just need to update any references to the saved object client from angular code to invoke $scope.$apply() after the call.

}
};
}
Expand Down
2 changes: 2 additions & 0 deletions src/core_plugins/kibana/server/tutorials/apm/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { TUTORIAL_CATEGORY } from '../../../common/tutorials/tutorial_category';
import { onPremInstructions } from './on_prem';
import { ELASTIC_CLOUD_INSTRUCTIONS } from './elastic_cloud';
import { getSavedObjects } from './saved_objects';

const apmIntro = 'Collect in-depth performance metrics and errors from inside your applications.';

Expand Down Expand Up @@ -64,5 +65,6 @@ export function apmSpecProvider(server) {
onPrem: onPremInstructions(server),
elasticCloud: ELASTIC_CLOUD_INSTRUCTIONS,
previewImagePath: '/plugins/kibana/home/tutorial_resources/apm/apm.png',
savedObjects: getSavedObjects(server),
};
}
Loading