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

Split integrations.ts into handlers & data #40685

Merged
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/integrations_manager/common/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { PLUGIN_ID } from './constants';
export const API_ROOT = `/api/${PLUGIN_ID}`;
export const API_LIST_PATTERN = `${API_ROOT}/list`;
export const API_INFO_PATTERN = `${API_ROOT}/package/{pkgkey}`;
export const API_INSTALL_PATTERN = `${API_ROOT}/install/{pkgkey}/{feature?}`;
export const API_DELETE_PATTERN = `${API_ROOT}/delete/{pkgkey}/{feature?}`;
export const API_INSTALL_PATTERN = `${API_ROOT}/install/{pkgkey}/{asset?}`;
export const API_DELETE_PATTERN = `${API_ROOT}/delete/{pkgkey}/{asset?}`;

export function getListPath() {
return API_LIST_PATTERN;
Expand Down
10 changes: 6 additions & 4 deletions x-pack/legacy/plugins/integrations_manager/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,17 @@ export interface RegistryPackage {
// the public HTTP response types
export type IntegrationList = IntegrationListItem[];

export type IntegrationListItem = Installed<RegistryListItem> | NotInstalled<RegistryListItem>;
export type IntegrationListItem = Installable<RegistryListItem>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you don’t have to do the | each time. Went with Installable over Maybe*, Either*, etc.


export type IntegrationInfo = Installed<RegistryPackage> | NotInstalled<RegistryPackage>;
export type IntegrationInfo = Installable<RegistryPackage>;

type Installed<T = {}> = T & {
export type Installable<T> = Installed<T> | NotInstalled<T>;

export type Installed<T = {}> = T & {
status: 'installed';
savedObject: InstallationSavedObject;
};

type NotInstalled<T = {}> = T & {
export type NotInstalled<T = {}> = T & {
status: 'not_installed';
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,9 @@ import { IntegrationInfo } from '../../common/types';

export function Detail(props: { package: string }) {
const [info, setInfo] = useState<IntegrationInfo | null>(null);
useEffect(
() => {
getIntegrationInfoByKey(props.package).then(setInfo);
},
[props.package]
);
useEffect(() => {
getIntegrationInfoByKey(props.package).then(setInfo);
}, [props.package]);

// don't have designs for loading/empty states
if (!info) return null;
Expand Down
230 changes: 0 additions & 230 deletions x-pack/legacy/plugins/integrations_manager/server/integrations.ts

This file was deleted.

109 changes: 109 additions & 0 deletions x-pack/legacy/plugins/integrations_manager/server/integrations/data.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I think a few simple comments on top of each of the exported functions in this file to explain what they do in relationship to each other would go a long way in helping new folks understand the flow here, and then we can massage the actual names of the functions as we better understand how it all fits together or what we need for other cases, etc. Otherwise this is all really clear and clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Breaking this up helped me see some patterns.

If it’s OK with you, I’d like to hold off on that until I add another asset type (e.g. #38609) because I think that might influence things. Things don’t need to be “final” before I add the comments, just looking to get back to making after this refactoring then see how things fit together.

* 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.
*/

import {
SavedObject,
SavedObjectsBulkGetObject,
SavedObjectsClientContract,
} from 'src/core/server/saved_objects';
import { InstallationSavedObject, Installable } from '../../common/types';
import { SAVED_OBJECT_TYPE } from '../../common/constants';
import * as Registry from '../registry';

export async function getIntegrations(client: SavedObjectsClientContract) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions all accept a saved object client. Mainly because we get the client from the request, but it also means we can inject a client. Figure we’ll come back to this over time and when the client lands in core .

Copy link
Member

Choose a reason for hiding this comment

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

So happy to have TS here...

const registryItems = await Registry.fetchList();
const searchObjects: SavedObjectsBulkGetObject[] = registryItems.map(({ name, version }) => ({
type: SAVED_OBJECT_TYPE,
id: `${name}-${version}`,
}));

const results = await client.bulkGet(searchObjects);
const savedObjects: InstallationSavedObject[] = results.saved_objects.filter(o => !o.error); // ignore errors for now
Copy link
Member

@jasonrhodes jasonrhodes Jul 10, 2019

Choose a reason for hiding this comment

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

do you know if the saved object client takes a type generic for what its methods return? I like being able to do:

const results = await client.bulkGet<InstallationSavedObject>(searchObjects);

and not having to explicitly type the return of the filter on the next line, if the client method handles that properly. Not a blocker just curious.

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 haven’t seen that before but, yeah, they do.

Or something quite similar

async bulkGet<T extends SavedObjectAttributes = any>(
&
export type SavedObjectAttribute =
| string
| number
| boolean
| null
| undefined
| SavedObjectAttributes
| SavedObjectAttributes[];
/**
*
* @public
*/
export interface SavedObjectAttributes {
[key: string]: SavedObjectAttribute | SavedObjectAttribute[];
}

Take a look at 98dfac8. I updated all the client.* calls and removed the explicit typing of their return values.

const integrationList = registryItems
.map(item =>
createInstallableFrom(
item,
savedObjects.find(({ id }) => id === `${item.name}-${item.version}`)
)
)
.sort(sortByName);

return integrationList;
}

export async function getIntegrationInfo(client: SavedObjectsClientContract, pkgkey: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this the single resource version of getIntegrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, IIUC. It’s what’s used for the detail page. getIntegrations is what’s used for for the list view.

const [item, savedObject] = await Promise.all([
Registry.fetchInfo(pkgkey),
getInstallationObject(client, pkgkey),
]);
const installation = createInstallableFrom(item, savedObject);

return installation;
}

export async function getInstallationObject(
client: SavedObjectsClientContract,
Copy link
Member

Choose a reason for hiding this comment

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

And this is "get the current install state for this one integration"?

pkgkey: string
): Promise<InstallationSavedObject | undefined> {
return client.get(SAVED_OBJECT_TYPE, pkgkey).catch(e => undefined);
}

export async function installAssets(
client: SavedObjectsClientContract,
pkgkey: string,
filter = (entry: Registry.ArchiveEntry): boolean => true
) {
const toBeSavedObjects = await Registry.getObjects(pkgkey, filter);
const createResults = await client.bulkCreate(toBeSavedObjects, { overwrite: true });
const createdObjects: SavedObject[] = createResults.saved_objects;
const installed = createdObjects.map(({ id, type }) => ({ id, type }));
const results = await client.create(
SAVED_OBJECT_TYPE,
{ installed },
{ id: pkgkey, overwrite: true }
);

return results;
}

export async function removeInstallation(client: SavedObjectsClientContract, pkgkey: string) {
const installation = await getInstallationObject(client, pkgkey);
const installedObjects = (installation && installation.attributes.installed) || [];

// Delete the manager saved object with references to the asset objects
// could also update with [] or some other state
await client.delete(SAVED_OBJECT_TYPE, pkgkey);

// Delete the installed assets
const deletePromises = installedObjects.map(async ({ id, type }) => client.delete(type, id));
await Promise.all(deletePromises);

// successful delete's in SO client return {}. return something more useful
return installedObjects;
}

function sortByName(a: { name: string }, b: { name: string }) {
if (a.name > b.name) {
return 1;
} else if (a.name < b.name) {
return -1;
} else {
return 0;
}
}

function createInstallableFrom<T>(from: T, savedObject?: InstallationSavedObject): Installable<T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped all the overloads

return savedObject
? {
...from,
status: 'installed',
savedObject,
}
: {
...from,
status: 'not_installed',
};
}
Loading