-
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
[Archive Migration][Partial] discover apps-management #110444
[Archive Migration][Partial] discover apps-management #110444
Conversation
a07f3c1
to
05c1277
Compare
05c1277
to
b80083b
Compare
creates. Add a sleep to see if there's a race.
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 minor changes.
await kibanaServer.uiSettings.replace({}); | ||
}); | ||
|
||
after(async function afterAll() { | ||
await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/discover'); | ||
await PageObjects.settings.navigateTo(); | ||
await esArchiver.emptyKibanaIndex(); |
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.
Is await esArchiver.emptyKibanaIndex();
just a wrapper for esArchiver.load('emptyKibana')
?
Or an esArchiver.unload()
?
If so, I don't think it should be necessary. And we really want to avoid using the esArchiver.unload()
on the .kibana index.
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 totally sure about the differences, but esArchiver.load('emptyKibana')
loads this obj:
{
"type": "doc",
"value": {
"id": "config:6.0.0-alpha1",
"index": ".kibana",
"source": {
"config": {
"buildNum": 8467,
"dateFormat:tz": "UTC"
},
"type": "config"
}
}
}
and await esArchiver.emptyKibanaIndex()
looks like this:
export async function emptyKibanaIndexAction({
client,
log,
kbnClient,
}: {
client: KibanaClient;
log: ToolingLog;
kbnClient: KbnClient;
}) {
const stats = createStats('emptyKibanaIndex', log);
await cleanKibanaIndices({ client, stats, log });
await migrateKibanaIndex(kbnClient);
stats.createdIndex('.kibana');
return stats.toJSON();
}
Looks like the latter is more involved.
// delete .kibana index and then wait for Kibana to re-create it | ||
await kibanaServer.uiSettings.replace({}); | ||
await kibanaServer.uiSettings.update({}); | ||
}); | ||
|
||
after(async function afterAll() { | ||
await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/discover'); | ||
await PageObjects.settings.navigateTo(); | ||
await PageObjects.settings.clickKibanaIndexPatterns(); | ||
await PageObjects.settings.removeLogstashIndexPatternIfExist(); |
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 don't think we need these last 3 statements. The logstash index pattern should be loaded by the kibanaServer.importExport.load and should be removed by the kibanaServer.importExport.unload so I think this is just wasting time.
since there's only one test. Add so cleanup, per CR.
drop these three cleanup lines as the unload should handle it.
Should not need this.
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
@@ -125,7 +122,7 @@ export default function ({ getService, getPageObjects }) { | |||
'painless', | |||
'number', | |||
null, | |||
'1', | |||
'100', |
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 change was just to help test if the scripted field existing but was down below the view of the field list. But it doesn't really matter if you change it back or not.
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.
Oh I remember rapping with you about that. It was like "visibility" or something.
I'll leave it for now, but I'm glad we've this comment here, for posterity.
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.
LGTM - just code review and Jenkins results.
* [Archive Migration][Partial] discover apps-management Comes from elastic#102827 Helps with elastic#108503 Only include the changes under the test/functional/apps/management folder. * Remove the index pattern, that the test creates. * Drop beforeEach(), in favour of before(), since there's only one test. * Drop outdated comment, drop these three cleanup lines as the unload should handle it. * Just keep one cleanup. Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
) * [Archive Migration][Partial] discover apps-management Comes from #102827 Helps with #108503 Only include the changes under the test/functional/apps/management folder. * Remove the index pattern, that the test creates. * Drop beforeEach(), in favour of before(), since there's only one test. * Drop outdated comment, drop these three cleanup lines as the unload should handle it. * Just keep one cleanup. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Tre <[email protected]>
Comes from #102827
Helps with #108503
Only include the changes under the
test/functional/apps/management folder.
Also, had to clean the index pattern within a previous test
to prevent duplicates