-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Index management] Treat indices beginning with a period as regular indices #112990
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
Changes from all commits
8ebe741
7082580
b72d566
0867002
dd68b60
8143dc1
4dae1f5
dc292d5
c145a85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,4 +5,4 @@ | |
| * 2.0. | ||
| */ | ||
|
|
||
| export const BRANCH = '8.x'; | ||
| export const MAJOR_VERSION = '8.0.0'; | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import { setUiMetricService } from '../../public/application/services/api'; | |
| import { indexManagementStore } from '../../public/application/store'; | ||
| import { setExtensionsService } from '../../public/application/store/selectors/extension_service'; | ||
| import { ExtensionsService } from '../../public/services'; | ||
| import { kibanaVersion } from '../client_integration/helpers'; | ||
|
|
||
| /* eslint-disable @kbn/eslint/no-restricted-paths */ | ||
| import { notificationServiceMock } from '../../../../../src/core/public/notifications/notifications_service.mock'; | ||
|
|
@@ -43,24 +44,30 @@ let server = null; | |
| let store = null; | ||
| const indices = []; | ||
|
|
||
| for (let i = 0; i < 105; i++) { | ||
| const baseFake = { | ||
| health: i % 2 === 0 ? 'green' : 'yellow', | ||
| status: i % 2 === 0 ? 'open' : 'closed', | ||
| const getBaseFakeIndex = (isOpen) => { | ||
| return { | ||
| health: isOpen ? 'green' : 'yellow', | ||
| status: isOpen ? 'open' : 'closed', | ||
| primary: 1, | ||
| replica: 1, | ||
| documents: 10000, | ||
| documents_deleted: 100, | ||
| size: '156kb', | ||
| primary_size: '156kb', | ||
| }; | ||
| }; | ||
|
|
||
| for (let i = 0; i < 105; i++) { | ||
| indices.push({ | ||
| ...baseFake, | ||
| ...getBaseFakeIndex(true), | ||
| name: `testy${i}`, | ||
| }); | ||
| indices.push({ | ||
| ...baseFake, | ||
| ...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 | ||
| }); | ||
| } | ||
|
|
||
|
|
@@ -110,6 +117,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 :) | ||
| let dispatchedActionsCount = 0; | ||
| store.subscribe(() => { | ||
| if (dispatchedActionsCount === 1) { | ||
|
|
@@ -254,15 +262,44 @@ describe('index table', () => { | |
| expect(button.text()).toEqual('Manage 2 indices'); | ||
| }); | ||
|
|
||
| test('should show system indices only when the switch is turned on', async () => { | ||
| test('should show hidden indices only when the switch is turned on', async () => { | ||
| const rendered = mountWithIntl(component); | ||
| await runAllPromises(); | ||
| rendered.update(); | ||
|
|
||
| snapshot(rendered.find('.euiPagination li').map((item) => item.text())); | ||
| const switchControl = rendered.find('.euiSwitch__button'); | ||
| // We have manually set `.admin1` and `.admin3` as hidden indices | ||
| // 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'); | ||
|
Contributor
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. 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?
Contributor
Author
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. 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) |
||
|
|
||
| if (kibanaVersion.major >= 8) { | ||
| // From 8.x indices starting with a period are treated as normal indices | ||
| expect(indicesInTable).toContain('.admin0'); | ||
| expect(indicesInTable).toContain('.admin2'); | ||
| } else if (kibanaVersion.major < 8) { | ||
| // In 7.x those are treated as system and are thus hidden | ||
| expect(indicesInTable).not.toContain('.admin0'); | ||
| expect(indicesInTable).not.toContain('.admin2'); | ||
| } | ||
|
|
||
| snapshot(indicesInTable); | ||
|
|
||
| // Enable "Show hidden indices" | ||
| const switchControl = findTestSubject(rendered, 'indexTableIncludeHiddenIndicesToggle'); | ||
| switchControl.simulate('click'); | ||
| snapshot(rendered.find('.euiPagination li').map((item) => item.text())); | ||
|
|
||
| // We do expect now the `.admin1` and `.admin3` indices to be in the list | ||
| indicesInTable = namesText(rendered); | ||
| expect(indicesInTable).toContain('.admin1'); | ||
| expect(indicesInTable).toContain('.admin3'); | ||
|
|
||
| if (kibanaVersion.major < 8) { | ||
| expect(indicesInTable).toContain('.admin0'); | ||
| expect(indicesInTable).toContain('.admin2'); | ||
| } | ||
|
|
||
| snapshot(indicesInTable); | ||
| }); | ||
|
|
||
| test('should filter based on content of search input', async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
| import SemVer from 'semver/classes/semver'; | ||
| import { Index } from '../../../common'; | ||
|
|
||
| const version = '8.0.0'; | ||
|
Contributor
Author
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. In
Contributor
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. Does this need to be hardcoded? I think you can read the Kibana version from core via
Contributor
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 agree with Alison. Would we have to remember to update these strings for each patch release (
Contributor
Author
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 started trying to inject dynamically the version with
No we won't, the code only looks for the major version. |
||
| const kibanaVersion = new SemVer(version); | ||
|
|
||
| export const isHiddenIndex = (index: Index): boolean => { | ||
| if (kibanaVersion.major < 8) { | ||
| // In 7.x we consider hidden index all indices whose name start with a dot | ||
| return (index.name ?? '').startsWith('.') || index.hidden === true; | ||
| } | ||
| return index.hidden === true; | ||
| }; | ||
|
|
||
| export const isSystemIndex = (index: Index): boolean => { | ||
| if (kibanaVersion.major < 8) { | ||
| return (index.name ?? '').startsWith('.'); | ||
| } | ||
| // From 8.0 we won't surface system indices in Index management | ||
| return false; | ||
| }; | ||
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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.