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

[Ingest Manager] Do not await in start. Return a Promise #69505

Merged
merged 18 commits into from
Jun 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export * from './fleet_setup';
export * from './epm';
export * from './enrollment_api_key';
export * from './install_script';
export * from './ingest_setup';
export * from './output';
export * from './settings';
export * from './app';
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export interface PostIngestSetupResponse {
isInitialized: boolean;
}
35 changes: 20 additions & 15 deletions x-pack/plugins/ingest_manager/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { i18n } from '@kbn/i18n';
import { DEFAULT_APP_CATEGORIES } from '../../../../src/core/public';
import { DataPublicPluginSetup, DataPublicPluginStart } from '../../../../src/plugins/data/public';
import { LicensingPluginSetup } from '../../licensing/public';
import { PLUGIN_ID } from '../common/constants';
import { PLUGIN_ID, CheckPermissionsResponse, PostIngestSetupResponse } from '../common';

import { IngestManagerConfigType } from '../common/types';
import { setupRouteService, appRoutesService } from '../common';
Expand All @@ -28,10 +28,7 @@ export type IngestManagerSetup = void;
*/
export interface IngestManagerStart {
registerDatasource: typeof registerDatasource;
success: boolean;
error?: {
message: string;
};
success: Promise<true>;
Copy link
Member

Choose a reason for hiding this comment

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

I found the naming a little confusing, what do you thing to have something like initializationPromise ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. I agree that the name could be improved. Perhaps we could use the isInitialized or isReady names we use in the setup responses[1]?

I was initially trying to make smallest external change but we already made it a breaking change by making it a promise so I guess we might as well name it what we want.

[1] I should file a ticket to make these consistent

}

export interface IngestManagerSetupDeps {
Expand Down Expand Up @@ -78,21 +75,29 @@ export class IngestManagerPlugin
}

public async start(core: CoreStart): Promise<IngestManagerStart> {
let successPromise: IngestManagerStart['success'];
try {
const permissionsResponse = await core.http.get(appRoutesService.getCheckPermissionsPath());
if (permissionsResponse.success) {
const { isInitialized: success } = await core.http.post(setupRouteService.getSetupPath());
return { success, registerDatasource };
const permissionsResponse = await core.http.get<CheckPermissionsResponse>(
appRoutesService.getCheckPermissionsPath()
);

if (permissionsResponse?.success) {
successPromise = core.http
.post<PostIngestSetupResponse>(setupRouteService.getSetupPath())
.then(({ isInitialized }) =>
isInitialized ? Promise.resolve(true) : Promise.reject(new Error('Unknown setup error'))
);
} else {
throw new Error(permissionsResponse.error);
throw new Error(permissionsResponse?.error || 'Unknown permissions error');
}
} catch (error) {
return {
success: false,
error: { message: error.body?.message || 'Unknown error' },
registerDatasource,
};
successPromise = Promise.reject(error);
}

return {
success: successPromise,
registerDatasource,
};
}

public stop() {}
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/ingest_manager/server/routes/setup/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { RequestHandler } from 'src/core/server';
import { TypeOf } from '@kbn/config-schema';
import { outputService, appContextService } from '../../services';
import { GetFleetStatusResponse } from '../../../common';
import { GetFleetStatusResponse, PostIngestSetupResponse } from '../../../common';
import { setupIngestManager, setupFleet } from '../../services/setup';
import { PostFleetSetupRequestSchema } from '../../types';
import { IngestManagerError, getHTTPResponseCode } from '../../errors';
Expand Down Expand Up @@ -83,9 +83,10 @@ export const ingestManagerSetupHandler: RequestHandler = async (context, request
const callCluster = context.core.elasticsearch.legacy.client.callAsCurrentUser;
const logger = appContextService.getLogger();
try {
const body: PostIngestSetupResponse = { isInitialized: true };
await setupIngestManager(soClient, callCluster);
return response.ok({
body: { isInitialized: true },
body,
});
} catch (e) {
if (e instanceof IngestManagerError) {
Expand Down
15 changes: 1 addition & 14 deletions x-pack/plugins/security_solution/public/app/home/setup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,7 @@ export const Setup: React.FunctionComponent<{
});
};

const displayToast = () => {
notifications.toasts.addDanger({
title,
text: defaultText,
});
};

if (!ingestManager.success) {
if (ingestManager.error) {
displayToastWithModal(ingestManager.error.message);
} else {
displayToast();
}
}
ingestManager.success.catch((error: Error) => displayToastWithModal(error.message));
}, [ingestManager, notifications.toasts]);

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,6 @@ export const depsStartMock: () => DepsStartMock = () => {

return {
data: dataMock,
ingestManager: { success: true, registerDatasource },
ingestManager: { success: Promise.resolve(true), registerDatasource },
};
};
7 changes: 5 additions & 2 deletions x-pack/test/security_solution_endpoint/apps/endpoint/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@
*/
import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ loadTestFile }: FtrProviderContext) {
export default function ({ loadTestFile, getService }: FtrProviderContext) {
describe('endpoint', function () {
this.tags('ciGroup7');

const ingestManager = getService('ingestManager');
before(async () => {
await ingestManager.setup();
});
Comment on lines +11 to +14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

loadTestFile(require.resolve('./endpoint_list'));
loadTestFile(require.resolve('./policy_list'));
loadTestFile(require.resolve('./policy_details'));
Expand Down
2 changes: 2 additions & 0 deletions x-pack/test/security_solution_endpoint/services/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { services as apiIntegrationServices } from '../../api_integration/services';
import { services as xPackFunctionalServices } from '../../functional/services';
import { EndpointPolicyTestResourcesProvider } from './endpoint_policy';

export const services = {
...xPackFunctionalServices,
ingestManager: apiIntegrationServices.ingestManager,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice

policyTestResources: EndpointPolicyTestResourcesProvider,
};