-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Ingest setup upgrade #78081
Changes from 13 commits
76df74d
6dea606
268c526
04a39bd
f111406
2846b76
20357fd
9b29802
3d2a686
7cb80e9
6e333ab
99f6aee
8a41396
219cfa9
ba00737
267931b
037ba98
a76006f
5c259d1
06cfadc
a3c77ce
25c3cd0
202b7a2
ee4c774
c906aea
11b677d
4afbb56
3f8295d
7f02b5f
0427b9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* 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 expect from '@kbn/expect'; | ||
import { FtrProviderContext } from '../../../api_integration/ftr_provider_context'; | ||
import { skipIfNoDockerRegistry } from '../../helpers'; | ||
import { GetInfoResponse, Installed } from '../../../../plugins/ingest_manager/common'; | ||
|
||
export default function (providerContext: FtrProviderContext) { | ||
const { getService } = providerContext; | ||
const supertest = getService('supertest'); | ||
const log = getService('log'); | ||
|
||
describe('setup api', async () => { | ||
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. Can you add some tests that show the failure case(s)? I'm unclear what we intend to be returned from a 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. Yeah so adding the failure case might be tricky. Any ideas on how to trigger a failure? This is what comes to mind:
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. If we can map out the desired behavior I am happy to help with testing. I did a few different types recently working on Registry and /setup changes. If this is run on every setup I really want to understand/document what it should/does do. 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'll have to do some digging to figure out what should be returned in each failure scenario. After thinking about this more, the code changes will currently swallow the error :( because of this:
That's probably not what we want but in the current state if an error occurs it will always 500 and the response body will look like this:
The success case will always return:
For the error case we could probably create a new error class that is a child of to extract out the passed in status code. That way we can preserve the status code and message that occurred when the upgrade/install failed for a package and return that as a response for 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. @skh @neptunian any ideas on how to force a failure scenario in |
||
skipIfNoDockerRegistry(providerContext); | ||
describe('setup performs upgrades', async () => { | ||
const oldEndpointVersion = '0.13.0'; | ||
beforeEach(async () => { | ||
await supertest | ||
.post(`/api/ingest_manager/epm/packages/endpoint-${oldEndpointVersion}`) | ||
.set('kbn-xsrf', 'xxxx') | ||
.send({ force: true }) | ||
.expect(200); | ||
}); | ||
it('upgrades the endpoint package from 0.13.0 to the latest version available', async function () { | ||
let { body }: { body: GetInfoResponse } = await supertest | ||
.get(`/api/ingest_manager/epm/packages/endpoint-${oldEndpointVersion}`) | ||
.expect(200); | ||
const latestEndpointVersion = body.response.latestVersion; | ||
log.info(`Endpoint package latest version: ${latestEndpointVersion}`); | ||
// make sure we're actually doing an upgrade | ||
expect(latestEndpointVersion).not.eql(oldEndpointVersion); | ||
await supertest.post(`/api/ingest_manager/setup`).set('kbn-xsrf', 'xxxx').expect(200); | ||
|
||
({ body } = await supertest | ||
.get(`/api/ingest_manager/epm/packages/endpoint-${latestEndpointVersion}`) | ||
.expect(200)); | ||
expect(body.response).to.have.property('savedObject'); | ||
expect((body.response as Installed).savedObject.attributes.install_version).to.eql( | ||
latestEndpointVersion | ||
); | ||
}); | ||
}); | ||
}); | ||
} |
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.
Would it be better to
await
thegetInstallation
call here? If we did then I wouldn't have to change the return value and would just check if the result wasundefined
and throw the error here instead of insetup.ts
.