-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from 6 commits
362555a
e89b5b0
5c37b3a
f81e670
f737949
0f2ba73
04247a4
5437feb
9b978e9
13cef20
98dfac8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,109 @@ | ||||||||||||||||||||||||||||||||||||
/* | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 . There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
kibana/src/core/server/saved_objects/service/saved_objects_client.ts Lines 149 to 164 in 40a1bde
Take a look at 98dfac8. I updated all the |
||||||||||||||||||||||||||||||||||||
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) { | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the single resource version of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, IIUC. It’s what’s used for the detail page. |
||||||||||||||||||||||||||||||||||||
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, | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> { | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
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 withInstallable
overMaybe*
,Either*
, etc.