-
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
Install, delete, list integrations #40355
Install, delete, list integrations #40355
Conversation
``` > curl --user elastic:changeme http://localhost:5601/api/saved_objects/_find?type=integrations-manager {"page":1,"per_page":20,"total":0,"saved_objects":[]} ``` ``` > curl --user elastic:changeme http://localhost:5601/api/integrations_manager/install/apache-1.0.1/dashboard { "type": "integrations-manager", "id": "apache-1.0.1", "attributes": { "installed": [ { "id": "0c610510-5cbd-11e9-8477-077ec9664dbd", "type": "dashboard" }, { "id": "36f872a0-5c03-11e9-85b4-19d0072eb4f2", "type": "visualization" }, { "id": "filebeat-*", "type": "index-pattern" }, { "id": "80844540-5c97-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "38f96190-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "7e4084e0-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "0a994af0-5c9d-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "ab48c3f0-5ca6-11e9-8477-077ec9664dbd", "type": "visualization" } ] }, "references": [], "updated_at": "2019-07-04T00:59:57.231Z", "version": "WzMwNiwxXQ==" } ``` ``` > curl --user elastic:changeme http://localhost:5601/api/saved_objects/_find?type=integrations-manager {"page":1,"per_page":20,"total":1,"saved_objects":[{"type":"integrations-manager","id":"apache-1.0.1","attributes":{"installed":[{"id":"0c610510-5cbd-11e9-8477-077ec9664dbd","type":"dashboard"},{"id":"36f872a0-5c03-11e9-85b4-19d0072eb4f2","type":"visualization"},{"id":"filebeat-*","type":"index-pattern"},{"id":"80844540-5c97-11e9-8477-077ec9664dbd","type":"visualization"},{"id":"38f96190-5c99-11e9-8477-077ec9664dbd","type":"visualization"},{"id":"7e4084e0-5c99-11e9-8477-077ec9664dbd","type":"visualization"},{"id":"0a994af0-5c9d-11e9-8477-077ec9664dbd","type":"visualization"},{"id":"ab48c3f0-5ca6-11e9-8477-077ec9664dbd","type":"visualization"}]},"references":[],"updated_at":"2019-07-04T00:59:57.231Z","version":"WzMwNiwxXQ=="}]} ``` ``` > curl --user elastic:changeme -X DELETE -H 'kbn-xsrf: true' http://localhost:5601/api/saved_objects/integrations-manager/apache-1.0.1 {} ``` ``` > curl --user elastic:changeme http://localhost:5601/api/saved_objects/_find?type=integrations-manager {"page":1,"per_page":20,"total":0,"saved_objects":[]} ``` ``` > curl --user elastic:changeme http://localhost:5601/api/integrations_manager/install/apache-1.0.1/dashboard { "type": "integrations-manager", "id": "apache-1.0.1", "attributes": { "installed": [ { "id": "0c610510-5cbd-11e9-8477-077ec9664dbd", "type": "dashboard" }, { "id": "36f872a0-5c03-11e9-85b4-19d0072eb4f2", "type": "visualization" }, { "id": "filebeat-*", "type": "index-pattern" }, { "id": "80844540-5c97-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "38f96190-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "7e4084e0-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "0a994af0-5c9d-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "ab48c3f0-5ca6-11e9-8477-077ec9664dbd", "type": "visualization" } ] }, "references": [], "updated_at": "2019-07-04T01:00:39.373Z", "version": "WzMxNiwxXQ==" } ``` ``` > curl --user elastic:changeme http://localhost:5601/api/integrations_manager/install/haproxy-1.3.1/dashboard { "type": "integrations-manager", "id": "haproxy-1.3.1", "attributes": { "installed": [ { "id": "0c610510-5cbd-11e9-8477-077ec9664dbd", "type": "dashboard" }, { "id": "36f872a0-5c03-11e9-85b4-19d0072eb4f2", "type": "visualization" }, { "id": "filebeat-*", "type": "index-pattern" }, { "id": "80844540-5c97-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "38f96190-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "7e4084e0-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "0a994af0-5c9d-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "ab48c3f0-5ca6-11e9-8477-077ec9664dbd", "type": "visualization" } ] }, "references": [], "updated_at": "2019-07-04T01:00:53.429Z", "version": "WzMyNSwxXQ==" } ``` ``` > curl --user elastic:changeme http://localhost:5601/api/saved_objects/_find?type=integrations-manager {"page":1,"per_page":20,"total":2,"saved_objects":[ ... {"id":"ab48c3f0-5ca6-11e9-8477-077ec9664dbd","type":"visualization"}]},"references":[],"updated_at":"2019-07-04T01:00:53.429Z","version":"WzMyNSwxXQ=="}]} ```
``` curl --user elastic:changeme http://localhost:5601/api/integrations_manager/list [ { "id": "docker-1.2.3", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "haproxy-1.3.1", "type": "integrations-manager", "updated_at": "2019-07-04T12:07:14.115Z", "version": "WzM0OSwxXQ==", "attributes": { "installed": [ { "id": "0c610510-5cbd-11e9-8477-077ec9664dbd", "type": "dashboard" }, { "id": "36f872a0-5c03-11e9-85b4-19d0072eb4f2", "type": "visualization" }, { "id": "filebeat-*", "type": "index-pattern" }, { "id": "80844540-5c97-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "38f96190-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "7e4084e0-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "0a994af0-5c9d-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "ab48c3f0-5ca6-11e9-8477-077ec9664dbd", "type": "visualization" } ] }, "references": [] }, { "id": "rabbitmq-1.0.2", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "traefik-1.0.2", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "apache-1.0.1", "type": "integrations-manager", "updated_at": "2019-07-04T12:06:26.993Z", "version": "WzM0MCwxXQ==", "attributes": { "installed": [ { "id": "0c610510-5cbd-11e9-8477-077ec9664dbd", "type": "dashboard" }, { "id": "36f872a0-5c03-11e9-85b4-19d0072eb4f2", "type": "visualization" }, { "id": "filebeat-*", "type": "index-pattern" }, { "id": "80844540-5c97-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "38f96190-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "7e4084e0-5c99-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "0a994af0-5c9d-11e9-8477-077ec9664dbd", "type": "visualization" }, { "id": "ab48c3f0-5ca6-11e9-8477-077ec9664dbd", "type": "visualization" } ] }, "references": [] }, { "id": "kafka-1.0.1", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "kibana-1.3.2", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "mongodb-1.0.1", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "mysql-1.0.2", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "postgresql-1.0.2", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "system-2.0.1", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } }, { "id": "elasticsearch-1.3.1", "type": "integrations-manager", "error": { "statusCode": 404, "message": "Not found" } } ] ```
💔 Build Failed |
Jenkins, test this |
💔 Build Failed |
💚 Build Succeeded |
Probably shouldn't be in registry.js either, but Works For Now.
💚 Build Succeeded |
💚 Build Succeeded |
Won't pass CI b/c of type stuff, but fixes the views.
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
// the public HTTP response types | ||
export type IntegrationList = IntegrationListItem[]; | ||
|
||
export type IntegrationListItem = Installed<RegistryListItem> | NotInstalled<RegistryListItem>; |
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.
The idea here is the same as in:
kibana/x-pack/legacy/plugins/integrations_manager/server/integrations.ts
Lines 204 to 214 in d116949
return savedObject | |
? { | |
...obj, | |
status: 'installed', | |
savedObject, | |
} | |
: { | |
...obj, | |
status: 'not_installed', | |
}; | |
} |
LMK if there’s a better/idiomatic/whatever way of doing this.
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.
I actually would not have written this like you did... but I like how you did it and might adopt it in my own code 😊
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.
I think this looks pretty good already. For improved inference and readability it might be beneficial to store the installed type as a property, as in
interface Installed<T> {
status: 'installed';
value: T;
savedObject: InstallationSavedObject;
}
interface NotInstalled<T> {
status: 'not_installed';
value: T;
}
type MaybeInstalled<T> = Installed<T> | NotInstalled<T>;
export type IntegrationInfo = MaybeInstalled<RegistryPackage>;
That way the set of properties of the value is preserved and cleanly separated from the installation metadata (e.g. for iteration or serialization). As a pattern it also has the advantage that it's agnostic to the value's type.
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.
@weltenwort Thanks for the feedback. I considered something just like that. I think it’s probably the better/cleaner pattern.
I’ll keep thinking but it seems like a change worth making.
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.
A few questions and small changes to look at doing, overall things are looking good/moving.
export async function handleRequestInstall(req: InstallFeatureRequest) { | ||
const { pkgkey, feature } = req.params; | ||
|
||
if (feature === 'dashboard') { |
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.
Can you explain to me how this is meant to work? Requesting the "dashboard" feature just installs everything in the integration package right now, right?
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.
Requesting the "dashboard" feature just installs everything in the integration package right now, right?
No. There’s also an ingest pipeline in the example that isn’t installed.
Can you explain to me how this is meant to work?
feature
asset
specifies the entry point for the dependency/reference tree.
In this specific example, dashboard
leads to references
with visualizations
, and those reference index-pattern
s.
This also allows for the case where we don’t install an asset for one reason or another initially (e.g. no ML b/c license) but need to install it later.
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.
Got it -- and will there eventually be an endpoint to install everything the user has permission to install?
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.
Yes. We could have install/foo-1.2.3
default to installing all (my current thinking) or add something more explicit.
return installation; | ||
} | ||
|
||
export async function handleRequestInstall(req: InstallFeatureRequest) { |
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.
If "feature" is identical to what we've been calling "assets", can we rename all instances of "feature" -> "asset"?
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.
Mind if I do this in a separate PR? I opened #40548
} | ||
} | ||
|
||
function collectReferences( |
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.
Broad question about collectReferences
: are the items in asset.references
also represented in the overall paths
list? If so, would we avoid needing this if we defaulted to "install all the paths" for now?
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.
We need to support optional assets (e.g. no ML jobs, or only install data files) and reading the saved objects (from the archive) is the best (only?) way to get an asset’s dependency tree so we know what to skip.
At least that’s how I imagine it and how I avoided installing the ingest pipeline while installing all the dashboard-related objects.
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.
OK, makes sense for the most part. Can you adjust collectReferences
to return a Map
rather than mutate a passed in one? I think that might be a little easier to follow.
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.
Updated in d6873c1
); | ||
|
||
const integration = createInstallationObject(item, installedObject); | ||
integrationList.push(integration); |
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.
This for loop + Array#push
set up made me think there was some conditional thing about whether an item gets pushed. Since it looks like they all make it in, could we replace the for loop with const integrationList = registryItems.map()
?
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.
Updated in 8ffd5da
...obj, | ||
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.
I like to avoid specifying the return value of a function as much as possible, so that the actual return value dictates the type through TS inference. That way the real return value and the stated return value can't accidentally get out of sync. If we need to specify the types IntegrationInfo
and IntegrationListItem
elsewhere, you could try something like this:
where you just declare a return type that is export type InstallationObject = ReturnType<typeof createInstallationObject>
which would be a union type. Not sure if that will work for all our cases here, so just suggesting.
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.
I’m aiming to have the API (HTTP, JS, etc) types specified in common/types
so that client and server, or others, can share the typing for docs, validation, etc. Like the work in #40041. I just learned that Beats is doing something similar https:/elastic/kibana/blob/master/x-pack/legacy/plugins/beats_management/common/return_types.ts and heard they worked really well.
IMO, the client and server should consume/implement the spec; the spec shouldn’t fall out from their return values. If the response shape changes that should be explicitly changed in the interface/type in common
.
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.
Yeah, in APM we did the same thing around defining common/shared return types, but used a ReturnType
to guarantee that the client consumed exactly what the server returned: https:/elastic/kibana/blob/master/x-pack/legacy/plugins/apm/server/lib/transactions/get_top_transactions/index.ts#L23-L25
As long as the contract is secure it probably doesn't matter too much, but it would be nice to at least make clear which types are "Response" types with some kind of naming convention, perhaps?
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.
Sure. Names are definitely in flux.
Can we address those in later PRs as they come up vs iterating in this one? It’s got a lot of other useful stuff I’d like to land.
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.
I’ll log an issue later tonight. I’m worried about letting the confusion grow and become worse.
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.
Filed #40558 around naming
I want to keep types in common, distinct from client/server
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.
@jfsiii just to be clear, you mean the types for the HTTP API responses, right? Not all types for the project? Just clarifying -- makes sense to me to keep the API response types there.
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.
Not all types, no. Just those that are common/shared between public
and server
or other top-level directories.
Won't pass CI b/c of some TS issues around implicit any but will fix that shortly.
💔 Build Failed |
const integration = createInstallationObject(item, installedObject); | ||
integrationList.push(integration); | ||
} | ||
const integrationList: IntegrationList = registryItems.map(item => |
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.
I'm hoping we don't need this IntegrationList
type here, because createInstallationObject
already returns this type by nature of all of the registryItems
being the right type, right?
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.
Yeah. Seems fine without it ac8c985
💚 Build Succeeded |
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
Summary
Add API endpoints to
List integrations (added `status` field)
Install an integration (response includes references to saved objects)
List integrations includes `savedObject` field if installed
Delete an integration (and other objects we installed)
c79a5c8 Move integration-related functions from registry.ts to integrations.ts
c3d0fb6 Move all integration handlers to integrations.js as handle* functions.
fb4e1d0 Define separate types for registry vs public http responses (also related to Add JSON Schema files & Swagger endpoint #40041)
refs #40044
closes #40468