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

[Archive Migration] xpack..reporting-api-integ/reporting-security #112432

Merged

Conversation

wayneseymour
Copy link
Member

@wayneseymour wayneseymour commented Sep 16, 2021

Found another spot where the es archive
for discover is still in use.

Use the kbn archive instead.

Helps with: #102552

Partially unblocks: #110445

Found another spot where the es archive
for discover is still in use.

Use the kbn archive instead.
@wayneseymour wayneseymour added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 16, 2021
@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@wayneseymour wayneseymour marked this pull request as ready for review September 19, 2021 21:21
@wayneseymour wayneseymour requested review from a team as code owners September 19, 2021 21:21
Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

I think this test should unload in the after method

@@ -42,7 +44,7 @@ export default function ({ getService }: FtrProviderContext) {

after('remove index alias', async () => {
await esArchiver.load('test/functional/fixtures/es_archiver/logstash_functional');
await esArchiver.load('test/functional/fixtures/es_archiver/discover');
await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover');
Copy link

Choose a reason for hiding this comment

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

I think both of these lines should be unload?

Also, I don't understand what the call to cleanupIndexAlias does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, they were originally .load()'s: https://github.com/elastic/kibana/blob/master/x-pack/test/reporting_api_integration/reporting_and_security/bwc_existing_indexes.ts#L44

As for the kbn archiver, yes...that should be an unload. Great catch.

For cleanupIndexAlias, its a Higher Order Function. When it's called, it adds and index alias, and then it returns a fn, that when called, removes the same index alias...that it created.

It's first applied here:

cleanupIndexAlias = await reportingAPI.coerceReportsIntoExistingIndex(

this is where it creates.

Later on, it's called in the after method, cleaning up what it created, that index.

Anyway, so Ill change the kbn archiver call to be an unload. Same for the logstash call boss?

Copy link

Choose a reason for hiding this comment

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

Yes, please unload logstash_functional as well.

In the parent index file there's 10 tests. 4 of those load logstash_functional. So another option would be to split those 10 tests into 2 describes and manage the load/unload of logstash_functional in the before/after there for those 4 tests. That would save loading it 3 times. But you don't need to do that on this PR unless you want to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'd love to that right after this is merged.

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

@wayneseymour
Copy link
Member Author

@tsullivan thanks mate!

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - code and Jenkins results reviewed

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@LeeDr LeeDr merged commit 2c4abee into master Sep 20, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 20, 2021
…astic#112432)

* [Archive Migration] xpack..reporting-api-integ/reporting-security

Found another spot where the es archive
for discover is still in use.

Use the kbn archive instead.

* Switch to unload, per cr.

* unload logstash

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 21, 2021
…12432) (#112620)

* [Archive Migration] xpack..reporting-api-integ/reporting-security

Found another spot where the es archive
for discover is still in use.

Use the kbn archive instead.

* Switch to unload, per cr.

* unload logstash

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Tre <[email protected]>
@spalger spalger deleted the archive-migration/xpack-reporting-api-integ-reporting-security branch May 8, 2022 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants