[Index management] Treat indices beginning with a period as regular indices #112990

sebelga · 2021-09-23T15:03:14Z

In 8.x indices starting with a dot (.) will be considered as normal indices. Only if they are created with the setting { hidden: true } they will be considered hidden.

This PR updates Index management to treat indices beginning with a period as regular indices.

Note on implementation

This PR will be backported to 7.x. Once the backport is done I will add a commit to change the hardcoded version "8.0.0" to "7.16.0" and I will run the test updating the jest snapshots. We will then be able to test in 7.x and make sure there are no regression.

Deleting or closing an index

In 7.x, indices starting with a dot are considered system indices and there is a warning inside the modal that indicates that. In 8.x we won't surface system indices in Index management so we don't show any warning.

In 8.x

In 7.x

Fixes #79527

sebelga · 2021-09-23T15:30:50Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

-    health: i % 2 === 0 ? 'green' : 'yellow',
-    status: i % 2 === 0 ? 'open' : 'closed',
+let isOpen = true;
+const getBaseFakeIndex = () => {


I had to make this change to make sure alternate indices are open and closed.

I think this will be easier to follow if this function accepts isOpen instead of relying on a side effect:

const getBaseFakeIndex = (isOpen) => { /* ... */ };

And then the consumer is responsible for the alternation logic:

// Indices will alternate between being open and closed for (let i = 0; i < 105; i++) { indices.push({ ...getBaseFakeIndex(true), name: `testy${i}`, }); indices.push({ ...getBaseFakeIndex(false), name: `.admin${i}`, // Add 2 hidden indices in the list in position 3 & 7 // note: for each loop iteration we add 2 indices hidden: i === 1 || i === 3 ? true : false, // ".admin1" and ".admin3" are the only hidden in 8.x }); }

Kind of the same, but ok if you prefer it like that I will change it 👍

sebelga · 2021-09-23T15:31:33Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

    const rendered = mountWithIntl(component);
    await runAllPromises();
    rendered.update();

-    snapshot(rendered.find('.euiPagination li').map((item) => item.text()));
-    const switchControl = rendered.find('.euiSwitch__button');
+    snapshot(namesText(rendered));


Not sure why we were snapshotting the eui pagination list. I changed it to snapshot the list of indices names.

@cuff-links Looks like there is some overlap here with the work you're doing in #112010

sebelga · 2021-09-23T15:32:02Z

x-pack/plugins/index_management/public/application/lib/indices.ts

+import SemVer from 'semver/classes/semver';
+import { Index } from '../../../common';
+
+const version = '8.0.0';


In 7.x this will be 7.16.0

Does this need to be hardcoded? I think you can read the Kibana version from core via PluginInitializerContext.env.packageInfo.version. See #85969 (comment).

I agree with Alison. Would we have to remember to update these strings for each patch release (7.16.1, 8.0.1) and minor (8.1.0)?

I started trying to inject dynamically the version with PluginInitializerContext.env.packageInfo.version. Then I realised it was going to change way too many files for very little benefit. I then weighted the pros and cons and thought it was much more surgical with less impact in the code base (less risk of regression) to have 2 hardcoded strings.

Would we have to remember to update these strings for each patch release

No we won't, the code only looks for the major version.

sebelga · 2021-09-23T15:32:27Z

x-pack/plugins/index_management/public/application/store/selectors/indices_filter.test.ts

 import { ExtensionsService } from '../../../services';
 import { getFilteredIndices } from '.';
 // @ts-ignore
 import { defaultTableState } from '../reducers/table_state';
 import { setExtensionsService } from './extension_service';

+const version = '8.0.0';


In 7.x this will be 7.16.0

elasticmachine · 2021-09-23T15:34:13Z

Pinging @elastic/kibana-stack-management (Team:Stack Management)

cjcenizal · 2021-09-23T22:42:25Z

This PR will be backported to 7.x.

Why backport this to 7.x? After feature freeze the only backports we'll make to 7.16 are bug fixes, which have historically been fairly rare for our team. I don't think it's worth dealing with a complex backport when the likelihood of having to deal with merge conflicts is low.

I'm concerned about the loss of this type of UX when users delete indices which affect Kibana's internals:

We'll still want to warn users away from making potentially destructive mistakes. But how do we identify these internal indices now? Is it enough to assume any hidden index is internal? I think it might be. Can you reach out to the Core folks about this and get their input?

cjcenizal

Thanks for working on this, Seb! I tested locally and it works great. I do think we should avoid hardcoding version numbers, but I think we can also avoid having to worry about that entirely by not backporting this to 7.x.

cjcenizal · 2021-09-23T22:46:18Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

-    health: i % 2 === 0 ? 'green' : 'yellow',
-    status: i % 2 === 0 ? 'open' : 'closed',
+let isOpen = true;
+const getBaseFakeIndex = () => {


I think this will be easier to follow if this function accepts isOpen instead of relying on a side effect:

const getBaseFakeIndex = (isOpen) => { /* ... */ };

And then the consumer is responsible for the alternation logic:

// Indices will alternate between being open and closed for (let i = 0; i < 105; i++) { indices.push({ ...getBaseFakeIndex(true), name: `testy${i}`, }); indices.push({ ...getBaseFakeIndex(false), name: `.admin${i}`, // Add 2 hidden indices in the list in position 3 & 7 // note: for each loop iteration we add 2 indices hidden: i === 1 || i === 3 ? true : false, // ".admin1" and ".admin3" are the only hidden in 8.x }); }

cjcenizal · 2021-09-23T22:47:29Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

@@ -110,6 +119,7 @@ const testAction = (rendered, buttonIndex, rowIndex = 0) => {
  // so we "time" our assertion based on how many Redux actions we observe. This is brittle because it
  // depends upon how our UI is architected, which will affect how many actions are dispatched.
  // Expect this to break when we rearchitect the UI.
+  // Update: Expect this to be removed when we rearchitect the UI :)


Could you help me understand what this means? For clarity on my part, my original comment about rearchitecting wasn't a reference to removing Redux. It was a reference more broadly to any change to architecture which could break the the test. It was an attempt to explain the brittleness behind the test.

And I went one step further. We are testing implementation detail and we don't want that. Once we rearchitect the UI this test will have to go away. Actually it should even go away (and replaced by CIT) before doing any change of architecture to be sure we don't have any regression.

cjcenizal · 2021-09-23T22:52:27Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

    const rendered = mountWithIntl(component);
    await runAllPromises();
    rendered.update();

-    snapshot(rendered.find('.euiPagination li').map((item) => item.text()));
-    const switchControl = rendered.find('.euiSwitch__button');
+    snapshot(namesText(rendered));


@cuff-links Looks like there is some overlap here with the work you're doing in #112010

cjcenizal · 2021-09-23T22:56:52Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

    switchControl.simulate('click');
-    snapshot(rendered.find('.euiPagination li').map((item) => item.text()));
+
+    snapshot(namesText(rendered));


I'm having a hard time understanding what to expect from this. Some questions on my mind are:

What's the initial state of the toggle? On or off?

Are we expecting to see or not to see hidden indices in the first snapshot?

And all of the same questions apply again after we click the toggle again: is it off now, or on? Should we see hidden indices or not?

I think we can improve this by creating two separate tests: one to assert that the table shows hidden indices when the toggle is on, and one to assert the opposite. Within each test it would also be helpful to add an assertion about the state of the toggle (on or off). This will make it easier to verify the tests are working as expected based. We could take these even farther by inlining the expected content of the table instead of using a snapshot:

exports[`index table should show hidden indices only when the switch is turned on 1`] = ` Array [ "testy0", ".admin0", "testy1", "testy2", ".admin2", "testy3", "testy4", ".admin4", "testy5", ".admin5", ] `; exports[`index table should show hidden indices only when the switch is turned on 2`] = ` Array [ "testy0", ".admin0", "testy1", ".admin1", "testy2", ".admin2", "testy3", ".admin3", "testy4", ".admin4", ] `;

Let me see what can be done but let's not spend too much time on this (now) as time is scarce. I already improved on a previous bad test to snapshot the list of indices instead of the pagination.
We do have a tech debt to write proper CIT test and get rid of this file.

But I agree with you that it is a terrible test, I just try to do with what's in place to avoid breaking other tests and fixing the whole file.

cjcenizal · 2021-09-23T23:00:55Z

x-pack/plugins/index_management/public/application/lib/indices.ts

+import SemVer from 'semver/classes/semver';
+import { Index } from '../../../common';
+
+const version = '8.0.0';


I agree with Alison. Would we have to remember to update these strings for each patch release (7.16.1, 8.0.1) and minor (8.1.0)?

cjcenizal · 2021-09-23T23:03:41Z

x-pack/plugins/index_management/public/application/store/selectors/index.js

@@ -39,14 +40,14 @@ export const getIndexStatusByIndexName = (state, indexName) => {
  const { status } = indices[indexName] || {};
  return status;
 };
-export const getIsSystemIndexByName = (indexNames) => {
+export const getIsSystemIndexByName = (indexNames, allIndices) => {


Per my comment we might want to consider all hidden indices to be system indices.

I hear you and at the same time I'd like to understand why we can't have a simple naming convention and stick to it everywhere in the stack. system indices are system, and hidden indices are simply hidden. Why the UI has to diverge from that and mention to the user that one of his hidden index is in fact system index?

sebelga · 2021-09-27T08:27:11Z

Why backport this to 7.x? After feature freeze the only backports we'll make to 7.16 are bug fixes, which have historically been fairly rare for our team. I don't think it's worth dealing with a complex backport when the likelihood of having to deal with merge conflicts is low.

I don't think we have to look at it like that. I'd like to put in place a system where we don't have to think what differences exist between majors. This PR could have been created 2 releases ago and we we would have wanted to backport to 7.x

I like to keep things very simple. Are we ever going to backport a PR to a 7.16.x branch? Yes. Then let's have both majors aligned and don't ever worry about remembering the difference between them. The code base is the source of truth (that's why I added comments to explain the nuances).
When we will be in 9.x. If we find in the code if (kibanaVersion.major < 8) { ... } we know for sure that we can delete that branch of the condition.

"dealing with a complex backport" might be a bit an overstatement 😊 I would have preferred to be able to inject the kibana version dynamically but as I said in my comment that was going to be way too big of a change for very little gain.

I'm concerned about the loss of this type of UX when users delete indices which affect Kibana's internals

I will rich out to the @elastic/kibana-core but my assumption is that there were going to mark .kibana as a system index which would not be surfaced in the UI. We need to clarify that indeed but if .kibana is not a system index I don't know what is 😊
The issue I am solving has the title "Treat indices beginning with a period as regular indices" so that's the new behaviour starting in 8.x. Let's see what the core team thinks.

…system-hidden-indices

sebelga · 2021-09-27T13:06:37Z

@cjcenizal PR is ready for another round of review, thanks! 👍

cjcenizal

Code LGTM, didn't test locally. Just want to note that my comment regarding warning users against deleting system indices is moot (#112990 (comment)). I had assumed that ES had already implemented functionality to hide system indices, and that Kibana wasn't going to migrate over to define its internal indices as ES system indices, but that turns out not to be the case.

cjcenizal · 2021-09-27T18:36:30Z

x-pack/plugins/index_management/__jest__/components/index_table.test.js

+    // We **don't** expect them to be in this list as by default we don't show hidden indices
+    let indicesInTable = namesText(rendered);
+    expect(indicesInTable).not.toContain('.admin1');
+    expect(indicesInTable).not.toContain('.admin3');


This test makes sense for 8.0, but does it make sense for 7.16? In 7.16 wouldn't we want to assert that all dot-prefixed indices are hidden here, and revealed on lines 282-283?

That's what the snapshots should give us. But you are right it is not very explicit (that's why I never use snapshots). I added a test to make it explicit (dc292d5)

…system-hidden-indices

kibanamachine · 2021-09-28T13:16:31Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`indexManagement`	502	508	+6

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`indexManagement`	160	163	+3

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`indexManagement`	12	9	-3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`indexManagement`	527.0KB	534.3KB	+7.2KB

Unknown metric groups

API count

id	before	after	diff
`indexManagement`	165	168	+3

History

💚 Build #155703 succeeded 4dae1f5
💔 Build #155392 failed 0867002
💔 Build #155366 failed b72d566

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

…ndices (elastic#112990) # Conflicts: # x-pack/plugins/index_management/__jest__/client_integration/helpers/constants.ts # x-pack/plugins/index_management/__jest__/client_integration/index_template_wizard/template_edit.test.tsx

…ndices (#112990) (#113286) # Conflicts: # x-pack/plugins/index_management/__jest__/client_integration/helpers/constants.ts # x-pack/plugins/index_management/__jest__/client_integration/index_template_wizard/template_edit.test.tsx

sebelga added 3 commits September 23, 2021 13:00

Move logic to detect hidden & system indices to single place

8ebe741

Add logic to detect hidden and system indices according to major version

7082580

Update tests

b72d566

sebelga commented Sep 23, 2021

View reviewed changes

sebelga marked this pull request as ready for review September 23, 2021 15:33

sebelga requested a review from a team as a code owner September 23, 2021 15:33

sebelga added backport-v7.16.0 release_note:skip Skip the PR/issue when compiling release notes v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Sep 23, 2021

sebelga requested a review from cjcenizal September 23, 2021 15:34

Fix TS issue

0867002

cjcenizal mentioned this pull request Sep 23, 2021

Convert Kibana system indices to be hidden #113056

Closed

cjcenizal suggested changes Sep 23, 2021

View reviewed changes

sebelga added 3 commits September 27, 2021 10:09

Improve test readability

dd68b60

Merge remote-tracking branch 'upstream/master' into index-management/…

8143dc1

…system-hidden-indices

Update jest snapshot

4dae1f5

sebelga requested a review from cjcenizal September 27, 2021 13:06

sebelga mentioned this pull request Sep 27, 2021

[Mappings editor] Remove boost parameter from field types #113142

Merged

cjcenizal approved these changes Sep 27, 2021

View reviewed changes

sebelga added 2 commits September 28, 2021 12:13

Add test to explicitly check for indices starting with dot in 7.x

dc292d5

Merge remote-tracking branch 'upstream/master' into index-management/…

c145a85

…system-hidden-indices

sebelga merged commit e28f429 into elastic:master Sep 28, 2021

sebelga deleted the index-management/system-hidden-indices branch September 28, 2021 14:29

sebelga mentioned this pull request Sep 28, 2021

[7.x] [Index management] Treat indices beginning with a period as regular indices (#112990) #113286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Index management] Treat indices beginning with a period as regular indices #112990

[Index management] Treat indices beginning with a period as regular indices #112990

sebelga commented Sep 23, 2021 •

edited

Loading

sebelga Sep 23, 2021

cjcenizal Sep 23, 2021

sebelga Sep 27, 2021

sebelga Sep 23, 2021

cjcenizal Sep 23, 2021

sebelga Sep 23, 2021

alisonelizabeth Sep 23, 2021

cjcenizal Sep 23, 2021

sebelga Sep 27, 2021

sebelga Sep 23, 2021

elasticmachine commented Sep 23, 2021

cjcenizal commented Sep 23, 2021 •

edited

Loading

cjcenizal left a comment

cjcenizal Sep 23, 2021

cjcenizal Sep 23, 2021

sebelga Sep 27, 2021 •

edited

Loading

cjcenizal Sep 23, 2021

cjcenizal Sep 23, 2021

sebelga Sep 27, 2021

sebelga Sep 27, 2021

cjcenizal Sep 23, 2021

cjcenizal Sep 23, 2021

sebelga Sep 27, 2021

sebelga commented Sep 27, 2021 •

edited

Loading

sebelga commented Sep 27, 2021

cjcenizal left a comment •

edited

Loading

cjcenizal Sep 27, 2021

sebelga Sep 28, 2021

kibanamachine commented Sep 28, 2021

API count

[Index management] Treat indices beginning with a period as regular indices #112990

[Index management] Treat indices beginning with a period as regular indices #112990

Conversation

sebelga commented Sep 23, 2021 • edited Loading

Note on implementation

Deleting or closing an index

In 8.x

In 7.x

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 23, 2021

cjcenizal commented Sep 23, 2021 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Sep 27, 2021 • edited Loading

sebelga commented Sep 27, 2021

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Sep 28, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Any counts in public APIs

Async chunks

API count

History

sebelga commented Sep 23, 2021 •

edited

Loading

cjcenizal commented Sep 23, 2021 •

edited

Loading

sebelga Sep 27, 2021 •

edited

Loading

sebelga commented Sep 27, 2021 •

edited

Loading

cjcenizal left a comment •

edited

Loading