[sample data] handle index aliases when installing/uninstalling datasets #122689

pgayvallet · 2022-01-11T18:35:39Z

Summary

Fix #120137

Refactor the install and uninstall sample data routes
Add some unit test coverage
Handle the case where the sample data indices have been reindexed using UA and are now aliases

Checklist

Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2022-01-12T12:28:26Z

src/plugins/home/server/services/sample_data/lib/insert_data_into_index.ts

+} from './translate_timestamp';
+import { loadData } from './load_data';
+
+export const insertDataIntoIndex = ({


This was extracted from src/plugins/home/server/services/sample_data/routes/install.ts

pgayvallet · 2022-01-12T12:32:45Z

test/api_integration/apis/home/sample_data.ts

+        // Failing: See https://github.com/elastic/kibana/issues/121051
+        describe.skip('dates', () => {


Only those 2 tests are flaky so I isolated the skip, as this file is actually the only FTR coverage of the feature.

pgayvallet · 2022-01-12T12:36:26Z

src/plugins/home/server/services/sample_data/sample_data_installer.ts

+    try {
+      // if the sample data was reindexed using UA, the index name is actually an alias pointing to the reindexed
+      // index. In that case, we need to get rid of the alias and to delete the underlying index
+      const { body: response } = await this.esClient.asCurrentUser.indices.getAlias({
+        name: index,
+      });
+      const aliasName = index;
+      index = Object.keys(response)[0];
+      await this.esClient.asCurrentUser.indices.deleteAlias({ name: aliasName, index });
+    } catch (err) {
+      // ignore errors from missing alias
+    }
+
+    try {
+      await this.esClient.asCurrentUser.indices.delete({
+        index,
+      });
+    } catch (err) {
+      // ignore delete errors
+    }


This is the actual fix, and the only functional change of the PR. We're now checking if there's an alias matching the index name, and if there is, we delete both the alias and the underlying index

elasticmachine · 2022-01-12T12:37:11Z

Pinging @elastic/kibana-core (Team:Core)

lukeelmers · 2022-01-13T18:18:16Z

@elasticmachine merge upstream

lukeelmers

Ah, I was going to test this manually with a Cloud CI image, but then realized I'd need to test against the backport since I'd need a 7.17 build to compare.

The code & refactor here LGTM (thanks for taking the time to do the additional cleanup) ❤️

lukeelmers · 2022-01-13T18:11:10Z

src/plugins/home/server/services/sample_data/sample_data_installer.test.ts

+      try {
+        await installer.install('unknown_data_set');
+        expect('should have returned an error').toEqual('but it did not');
+      } catch (e) {
+        expect(e).toBeInstanceOf(SampleDataInstallError);
+        expect((e as SampleDataInstallError).httpCode).toEqual(404);
+      }


nit/optional: What's the benefit of using try/catch vs toThrowError?

expect(async () => { await installer.install('unknown_data_set'); }).rejects.toThrowError(new SampleDataInstallError(...));

If for some reason it fails to throw, the jest error message from toThrowError might be clearer than expected 'should have returned an error' to equal 'but it did not'.

Same question applies to other error tests in this file - are you just aiming to have more granular assertions for each aspect of the error?

Unless I'm wrong, .rejects.toThrowError(new SampleDataInstallError(...)) will assert against the error's msg too, so I would like to recreate a SampleDataInstallError with the same message we're throwing in the actual code, which I wanted to avoid

pgayvallet · 2022-01-17T07:13:13Z

@elasticmachine merge upstream

kibana-ci · 2022-01-17T08:38:25Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`home`	25	26	+1

Total ESLint disabled count

id	before	after	diff
`home`	26	27	+1

History

💚 Build #17330 succeeded fc510d7
💚 Build #16921 succeeded 593aad1
💚 Build #16882 succeeded 3559586
💔 Build #16847 failed 075278d
💔 Build #16775 failed 6e90891

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

…ets (elastic#122689) * refactor install/uninstall routes * only skip failing tests * check for aliases when uninstalling sample datasets * fix return value * add unit tests * factorize installer creation * add tests for the alias scenario Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 683ab10)

kibanamachine · 2022-01-17T09:30:58Z

💔 Some backports could not be created

Status	Branch	Result
✅	8.0
❌	7.17	Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 122689

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ets (#122689) (#123121) * refactor install/uninstall routes * only skip failing tests * check for aliases when uninstalling sample datasets * fix return value * add unit tests * factorize installer creation * add tests for the alias scenario Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 683ab10) Co-authored-by: Pierre Gayvallet <pierre.gayvallet@elastic.co>

…ets (elastic#122689) * refactor install/uninstall routes * only skip failing tests * check for aliases when uninstalling sample datasets * fix return value * add unit tests * factorize installer creation * add tests for the alias scenario Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 683ab10) # Conflicts: # src/plugins/home/server/services/sample_data/routes/install.ts # src/plugins/home/server/services/sample_data/routes/uninstall.ts # src/plugins/home/server/services/sample_data/routes/utils.ts # test/api_integration/apis/home/sample_data.ts

…g datasets (#122689) (#123147) * [sample data] handle index aliases when installing/uninstalling datasets (#122689) * refactor install/uninstall routes * only skip failing tests * check for aliases when uninstalling sample datasets * fix return value * add unit tests * factorize installer creation * add tests for the alias scenario Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 683ab10) # Conflicts: # src/plugins/home/server/services/sample_data/routes/install.ts # src/plugins/home/server/services/sample_data/routes/uninstall.ts # src/plugins/home/server/services/sample_data/routes/utils.ts # test/api_integration/apis/home/sample_data.ts * backport missing lib file * fix again * backporting more changes * remove logger from unmodified route * revert changes on FTR tests * adapt installer for 7.x Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

pgayvallet added 7 commits January 11, 2022 19:34

refactor install/uninstall routes

6e90891

only skip failing tests

53b9b99

check for aliases when uninstalling sample datasets

075278d

fix return value

3559586

add unit tests

35e3167

factorize installer creation

3bdd27f

add tests for the alias scenario

593aad1

pgayvallet commented Jan 12, 2022

View reviewed changes

pgayvallet marked this pull request as ready for review January 12, 2022 12:37

pgayvallet requested a review from a team as a code owner January 12, 2022 12:37

lukeelmers added the ci:deploy-cloud label Jan 13, 2022

Merge branch 'main' into kbn-120137-sample-data-alias

fc510d7

lukeelmers self-requested a review January 13, 2022 18:18

lukeelmers approved these changes Jan 13, 2022

View reviewed changes

Merge branch 'main' into kbn-120137-sample-data-alias

31b0594

pgayvallet added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 17, 2022

pgayvallet merged commit 683ab10 into elastic:main Jan 17, 2022

kibanamachine mentioned this pull request Jan 17, 2022

[8.0] [sample data] handle index aliases when installing/uninstalling datasets (#122689) #123121

Merged

pgayvallet mentioned this pull request Jan 17, 2022

[7.17] [sample data] handle index aliases when installing/uninstalling datasets (#122689) #123147

Merged

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sample data] handle index aliases when installing/uninstalling datasets #122689

[sample data] handle index aliases when installing/uninstalling datasets #122689

pgayvallet commented Jan 11, 2022 •

edited

Loading

pgayvallet Jan 12, 2022

pgayvallet Jan 12, 2022

pgayvallet Jan 12, 2022

elasticmachine commented Jan 12, 2022

lukeelmers commented Jan 13, 2022

lukeelmers left a comment

lukeelmers Jan 13, 2022

pgayvallet Jan 17, 2022

pgayvallet commented Jan 17, 2022

kibana-ci commented Jan 17, 2022

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Jan 17, 2022

		// Failing: See https://github.com/elastic/kibana/issues/121051
		describe.skip('dates', () => {

[sample data] handle index aliases when installing/uninstalling datasets #122689

[sample data] handle index aliases when installing/uninstalling datasets #122689

Conversation

pgayvallet commented Jan 11, 2022 • edited Loading

Summary

Checklist

pgayvallet Jan 12, 2022

Choose a reason for hiding this comment

pgayvallet Jan 12, 2022

Choose a reason for hiding this comment

pgayvallet Jan 12, 2022

Choose a reason for hiding this comment

elasticmachine commented Jan 12, 2022

lukeelmers commented Jan 13, 2022

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Jan 13, 2022

Choose a reason for hiding this comment

pgayvallet Jan 17, 2022

Choose a reason for hiding this comment

pgayvallet commented Jan 17, 2022

kibana-ci commented Jan 17, 2022

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Jan 17, 2022

💔 Some backports could not be created

How to fix

Questions ?

pgayvallet commented Jan 11, 2022 •

edited

Loading