Don't turn Kibana plugin red if Kibana index is not yet ready #7428

ycombinator · 2016-06-10T19:12:23Z

Of course, if the user has *explicitly* requested the status page, then don't reload the window and cause an infinite loop!

ycombinator · 2016-06-10T19:21:10Z

src/plugins/status_page/public/status_page.js

+      const statusPageUrl = chrome.addBasePath('/status');
+      if ((overall.state === 'green') && ($window.location.pathname !== statusPageUrl)) {
+        return $window.location.reload();
+      }


This bit of code is necessary for the following scenario:

If I tried to load up Kibana in the browser while the server was still starting up, and the Kibana index wasn't yet ready (i.e. the Elasticsearch plugin wasn't yet green), I was taken to the Status page. However, by the time the Status page was actually rendered, the Kibana index was ready and all the plugins had turned green. In such cases, rather than show the user a Status page where everything is green, I think it makes sense to reload the window so they are taken to wherever they meant to go.

Happy to remove this code if folks disagree.

Yeah, I disagree with this approach. The issue here is a race condition caused by legacy code from back when the status page automatically refreshed. I think the right thing to do here is get rid of the race condition and send the status information to the browser with the app code, rather than fetching it on page load.

Thanks @spalger. Are you okay with me filing a separate issue to fix this, rather than adding to the scope of this PR? I will, of course, remove this reloading code block from this PR.

Filed issue for the right fix at #7432 and removed reloading code block in db780bd.

bevacqua · 2016-06-10T19:38:39Z

Looks great. I'd remove the comments since they repeat what the code is already stating, but other than that LGTM!

bevacqua · 2016-06-10T19:39:21Z

Note that there seems to be test failures. Probably intermitent

ycombinator · 2016-06-10T19:42:18Z

@cjcenizal @LeeDr: This PR build failed because of these errors:

TypeError: _this.saveScreenshot(...).finally is not a function

I assume this is being fixed in master. Mind posting a comment here when its fixed so I can ask Jenkins to re-test this PR? Thanks!

spalger · 2016-06-10T20:17:01Z

Commented on an outdated diff because it had other relevant comments: #7428 (comment)

…egacy issue

bevacqua · 2016-06-10T20:30:38Z

src/ui/settings/index.js

-    server.plugins.kibana.status.red(message);
+    let message;
+    if (server.plugins.elasticsearch.status.state === 'green') {
+      server.plugins.kibana.status.red('Could not find user-provided settings for this version of Kibana (' + kibanaVersion + ')');


Can we change the message into a template string? Also the message variable is unused.

Totally. And good catch on the unused message variable — I'm surprised the linter didn't catch that. Fixing both things.

ycombinator · 2016-06-10T21:13:44Z

@bevacqua Made a few changes per comments + tests are passing now. Mind giving this another review, please?

bevacqua · 2016-06-10T21:49:55Z

LGTM 🚀

`v91.0.0-backport.0`⏩`v91.3.1` ⚠️ The largest set of changes in this PR that touch source code (as opposed to test code) are related to several **EuiDataGrid** redesigns, particularly around the toolbar, column cell headers, and cell actions. We **strongly** recommend QAing your EuiDataGrid usages, **especially** if you have custom CSS styling on data grid cells. | Changes | Screencap | |--------|--------| | Cell actions and popover | <img src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0" width="240" alt=""> | | Column headers | <img src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2" alt="" width="360"> | | Toolbar | <img src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3" alt="" width="240"> | --- ## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1) **Bug fixes** - Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton` test subject attribute from to the clickable button, for easier E2E testing ([#7427](elastic/eui#7427)) - Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as disabled when rows are being selected ([#7428](elastic/eui#7428)) ## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0) - Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs. ([#7399](elastic/eui#7399)) - Updated `EuiDataGridSchemaDetector`'s comparator arguments to include entry indexes ([#7406](elastic/eui#7406)) ## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0) - Added `endpoint` glyph to `EuiIcon` ([#7383](elastic/eui#7383)) **Bug fixes** - Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where scrollbar widths were not being accounted for ([#7392](elastic/eui#7392)) ## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0) - Updated `EuiDataGrid` cell actions to display above cells instead of within them, to avoid content clipping issues ([#7343](elastic/eui#7343)) - Updated `EuiDataGrid` cell expansion popovers to sit on top of cells instead of below/next to them ([#7343](elastic/eui#7343)) - Updated `EuiListGroupItem` to render an external icon and screen reader affordance for links with `target` set to to `_blank` ([#7352](elastic/eui#7352)) - Updated `EuiListGroupItem` with a new `external` prop, which allows enabling or disabling the new external link icon ([#7352](elastic/eui#7352)) - Updated `EuiText` to no longer set any opinionated styles on child `<img>` tags - use `EuiImage` for image display within text instead ([#7360](elastic/eui#7360)) - Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom actions ([#7361](elastic/eui#7361)) - Added a new `EuiDataGridToolbarControl` subcomponent, which is useful for rendering your own custom `EuiDataGrid` toolbar buttons while matching the look of the default controls ([#7369](elastic/eui#7369)) - Updated `EuiDataGrid`'s toolbar controls to show active/current counts in badges, and updated the Columns button icon ([#7369](elastic/eui#7369)) - Updated `EuiButtonEmpty` to allow passing `false` to `textProps`, which allows rendering custom button content without an extra text wrapper ([#7369](elastic/eui#7369)) - Updated `EuiDataGrid` column header cells to show the sort arrow after the heading text, instead of before ([#7371](elastic/eui#7371)) - Updated `EuiDataGrid`'s column header actions icon from a chevron to `boxesVertical` ([#7371](elastic/eui#7371)) - Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s. Alongside `name`, the `description`, `href`, and `data-test-subj` properties now also accept an optional callback that the current `item` will be passed to ([#7373](elastic/eui#7373)) - Updated `EuiContextMenuItem` with a new `toolTipProps` prop ([#7373](elastic/eui#7373)) - `EuiSelectable` now allows configurable text truncation via `listProps.truncationProps` ([#7388](elastic/eui#7388)) - `EuiTextTruncate` now supports a new `calculationDelayMs` prop for working around font loading or layout shifting scenarios ([#7388](elastic/eui#7388)) **Bug fixes** - Fixed incorrect `EuiPopover` positioning calculations when `hasArrow` was set to false ([#7343](elastic/eui#7343)) - Fixed `EuiSuperSelect` to render options with falsy values (false, 0, and ''), but not nullish values (undefined or null) ([#7362](elastic/eui#7362)) - Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g., booleans or numbers) ([#7362](elastic/eui#7362)) - Fixed `EuiDataGrid`'s numeric and currency column heading cells to be correctly right-aligned ([#7371](elastic/eui#7371)) - Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing tooltip descriptions when rendered in the all actions popover menu ([#7373](elastic/eui#7373)) - Fixed missing underlines on `EuiContextMenu` link hover ([#7373](elastic/eui#7373)) - Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent` ([#7375](elastic/eui#7375)) - Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off in vertical alignment ([#7380](elastic/eui#7380)) **Deprecations** - Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use `toolTipProps.title` instead ([#7373](elastic/eui#7373)) - Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use `toolTipProps.position` instead ([#7373](elastic/eui#7373)) **Accessibility** - Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested interactive custom actions ([#7361](elastic/eui#7361)) - Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly reading out action descriptions to screen readers ([#7373](elastic/eui#7373)) - Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not visibly appearing on keyboard focus ([#7373](elastic/eui#7373)) --------- Co-authored-by: Julia Rechkunova <[email protected]>

`v91.0.0-backport.0`⏩`v91.3.1` ⚠️ The largest set of changes in this PR that touch source code (as opposed to test code) are related to several **EuiDataGrid** redesigns, particularly around the toolbar, column cell headers, and cell actions. We **strongly** recommend QAing your EuiDataGrid usages, **especially** if you have custom CSS styling on data grid cells. | Changes | Screencap | |--------|--------| | Cell actions and popover | <img src="https://github.com/elastic/kibana/assets/549407/6462d983-307f-4a3c-84b1-36d9b276c9a0" width="240" alt=""> | | Column headers | <img src="https://github.com/elastic/kibana/assets/549407/3fd64a15-829a-48f3-9dba-9dae3c73e6b2" alt="" width="360"> | | Toolbar | <img src="https://github.com/elastic/kibana/assets/549407/f876f6d7-635d-497a-b1e7-9daf4e6fd3e3" alt="" width="240"> | --- ## [`v91.3.1`](https://github.com/elastic/eui/releases/v91.3.1) **Bug fixes** - Moved `EuiDataGrid`'s header cells' `dataGridHeaderCellActionButton` test subject attribute from to the clickable button, for easier E2E testing ([elastic#7427](elastic/eui#7427)) - Fixed `EuiBasicTable`/`EuiInMemoryTable` actions to correctly show as disabled when rows are being selected ([elastic#7428](elastic/eui#7428)) ## [`v91.3.0`](https://github.com/elastic/eui/releases/v91.3.0) - Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs. ([elastic#7399](elastic/eui#7399)) - Updated `EuiDataGridSchemaDetector`'s comparator arguments to include entry indexes ([elastic#7406](elastic/eui#7406)) ## [`v91.2.0`](https://github.com/elastic/eui/releases/v91.2.0) - Added `endpoint` glyph to `EuiIcon` ([elastic#7383](elastic/eui#7383)) **Bug fixes** - Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where scrollbar widths were not being accounted for ([elastic#7392](elastic/eui#7392)) ## [`v91.1.0`](https://github.com/elastic/eui/releases/tag/v91.1.0) - Updated `EuiDataGrid` cell actions to display above cells instead of within them, to avoid content clipping issues ([elastic#7343](elastic/eui#7343)) - Updated `EuiDataGrid` cell expansion popovers to sit on top of cells instead of below/next to them ([elastic#7343](elastic/eui#7343)) - Updated `EuiListGroupItem` to render an external icon and screen reader affordance for links with `target` set to to `_blank` ([elastic#7352](elastic/eui#7352)) - Updated `EuiListGroupItem` with a new `external` prop, which allows enabling or disabling the new external link icon ([elastic#7352](elastic/eui#7352)) - Updated `EuiText` to no longer set any opinionated styles on child `<img>` tags - use `EuiImage` for image display within text instead ([elastic#7360](elastic/eui#7360)) - Improved `EuiBasicTable`/`EuiInMemoryTable`s mobile UI for custom actions ([elastic#7361](elastic/eui#7361)) - Added a new `EuiDataGridToolbarControl` subcomponent, which is useful for rendering your own custom `EuiDataGrid` toolbar buttons while matching the look of the default controls ([elastic#7369](elastic/eui#7369)) - Updated `EuiDataGrid`'s toolbar controls to show active/current counts in badges, and updated the Columns button icon ([elastic#7369](elastic/eui#7369)) - Updated `EuiButtonEmpty` to allow passing `false` to `textProps`, which allows rendering custom button content without an extra text wrapper ([elastic#7369](elastic/eui#7369)) - Updated `EuiDataGrid` column header cells to show the sort arrow after the heading text, instead of before ([elastic#7371](elastic/eui#7371)) - Updated `EuiDataGrid`'s column header actions icon from a chevron to `boxesVertical` ([elastic#7371](elastic/eui#7371)) - Updated the actions column in `EuiBasicTable` and `EuiInMemoryTable`s. Alongside `name`, the `description`, `href`, and `data-test-subj` properties now also accept an optional callback that the current `item` will be passed to ([elastic#7373](elastic/eui#7373)) - Updated `EuiContextMenuItem` with a new `toolTipProps` prop ([elastic#7373](elastic/eui#7373)) - `EuiSelectable` now allows configurable text truncation via `listProps.truncationProps` ([elastic#7388](elastic/eui#7388)) - `EuiTextTruncate` now supports a new `calculationDelayMs` prop for working around font loading or layout shifting scenarios ([elastic#7388](elastic/eui#7388)) **Bug fixes** - Fixed incorrect `EuiPopover` positioning calculations when `hasArrow` was set to false ([elastic#7343](elastic/eui#7343)) - Fixed `EuiSuperSelect` to render options with falsy values (false, 0, and ''), but not nullish values (undefined or null) ([elastic#7362](elastic/eui#7362)) - Fixed `EuiSuperSelect`'s typing to allow non-string values (e.g., booleans or numbers) ([elastic#7362](elastic/eui#7362)) - Fixed `EuiDataGrid`'s numeric and currency column heading cells to be correctly right-aligned ([elastic#7371](elastic/eui#7371)) - Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not showing tooltip descriptions when rendered in the all actions popover menu ([elastic#7373](elastic/eui#7373)) - Fixed missing underlines on `EuiContextMenu` link hover ([elastic#7373](elastic/eui#7373)) - Fixed visual text truncation of `EuiBreadcrumb`s with `popoverContent` ([elastic#7375](elastic/eui#7375)) - Fixed `EuiFormRow`s with `hasEmptyLabelSpace` being very slightly off in vertical alignment ([elastic#7380](elastic/eui#7380)) **Deprecations** - Deprecated `EuiContextMenuItem`'s `toolTipTitle` prop. Use `toolTipProps.title` instead ([elastic#7373](elastic/eui#7373)) - Deprecated `EuiContextMenuItem`'s `toolTipPosition` prop. Use `toolTipProps.position` instead ([elastic#7373](elastic/eui#7373)) **Accessibility** - Fixed custom `EuiBasicTable`/`EuiInMemoryTable` rendering nested interactive custom actions ([elastic#7361](elastic/eui#7361)) - Fixed `EuiBasicTable` and `EuiInMemoryTable` actions not correctly reading out action descriptions to screen readers ([elastic#7373](elastic/eui#7373)) - Fixed `EuiBasicTable` and `EuiInMemoryTable` primary actions not visibly appearing on keyboard focus ([elastic#7373](elastic/eui#7373)) --------- Co-authored-by: Julia Rechkunova <[email protected]>

ycombinator added 2 commits June 10, 2016 12:07

If Elasticsearch plugin is not yet green, log a warning

a2459bb

Reload window if user has been sent to status page but status is green

ab66d5e

Of course, if the user has *explicitly* requested the status page, then don't reload the window and cause an infinite loop!

ycombinator added review v5.0.0 v5.0.0-alpha4 labels Jun 10, 2016

Adding comments

e860fc3

ycombinator reviewed Jun 10, 2016
View reviewed changes

ycombinator assigned bevacqua Jun 10, 2016

bevacqua assigned ycombinator and unassigned bevacqua Jun 10, 2016

ycombinator added 2 commits June 10, 2016 12:46

Removing extra parenthesis because operator precedence is a thing

3ebf365

Removing redundant comments

aa05237

ycombinator mentioned this pull request Jun 10, 2016

Render status page synchronously with page load #7432

Closed

Removing window reload code in favor of the right fix for what is a l…

db780bd

…egacy issue

bevacqua reviewed Jun 10, 2016
View reviewed changes

ycombinator added 2 commits June 10, 2016 13:37

Removing unused variable

58f9843

Using template string instead of concatenation

3425818

ycombinator merged commit 5cf440c into elastic:master Jun 10, 2016

ycombinator deleted the gh-7424 branch June 10, 2016 21:51

epixa added v5.0.0 and removed v5.0.0 labels Jun 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't turn Kibana plugin red if Kibana index is not yet ready #7428

Don't turn Kibana plugin red if Kibana index is not yet ready #7428

ycombinator commented Jun 10, 2016

ycombinator Jun 10, 2016 •

edited

Loading

spalger Jun 10, 2016

ycombinator Jun 10, 2016

ycombinator Jun 10, 2016

bevacqua commented Jun 10, 2016

bevacqua commented Jun 10, 2016 •

edited

Loading

ycombinator commented Jun 10, 2016

spalger commented Jun 10, 2016

bevacqua Jun 10, 2016

ycombinator Jun 10, 2016

ycombinator commented Jun 10, 2016

bevacqua commented Jun 10, 2016

Don't turn Kibana plugin red if Kibana index is not yet ready #7428

Don't turn Kibana plugin red if Kibana index is not yet ready #7428

Conversation

ycombinator commented Jun 10, 2016

ycombinator Jun 10, 2016 • edited Loading

Choose a reason for hiding this comment

spalger Jun 10, 2016

Choose a reason for hiding this comment

ycombinator Jun 10, 2016

Choose a reason for hiding this comment

ycombinator Jun 10, 2016

Choose a reason for hiding this comment

bevacqua commented Jun 10, 2016

bevacqua commented Jun 10, 2016 • edited Loading

ycombinator commented Jun 10, 2016

spalger commented Jun 10, 2016

bevacqua Jun 10, 2016

Choose a reason for hiding this comment

ycombinator Jun 10, 2016

Choose a reason for hiding this comment

ycombinator commented Jun 10, 2016

bevacqua commented Jun 10, 2016

ycombinator Jun 10, 2016 •

edited

Loading

bevacqua commented Jun 10, 2016 •

edited

Loading