If user settings cannot be found for installed version of Kibana, turn Kibana plugin red #7333

ycombinator · 2016-05-31T22:00:27Z

Otherwise the promise never settles and can block a response from the Kibana server.

Resolves #7318.
Resolves #7278.

Otherwise the promise never settles and can block a response from the Kibana server.

pickypg · 2016-05-31T22:01:25Z

Is that preferable to telling them there is some kind of error?

ycombinator · 2016-05-31T22:07:47Z

Is that preferable to telling them there is some kind of error?

Yeah, I'd think the error needs to be surfaced somewhere (either in the Kibana server log or in the UI) but I'm not too familiar with this code so I'll defer to @bevacqua on what the right thing to do here is. I'm choosing to return an empty object because I think this is the right default, per #7285 (comment).

bevacqua · 2016-06-01T14:33:33Z

src/ui/settings/index.js

@@ -29,7 +29,8 @@ export default function setupSettings(kbnServer, server, config) {
    return client
      .get({ ...clientSettings })
      .then(res => res._source)
-      .then(user => hydrateUserSettings(user));
+      .then(user => hydrateUserSettings(user))
+      .catch(e => { return {}; });


You can do .catch(e => ({})); instead. While .catch(e => {}) would result in an empty block function, ({}) is treated as an expression resolving to an empty object.

Oh, how interesting! I tend to prefer the more explicit syntax so its obvious to the reader what's going on but this is good to know!

Returning an object from an arrow function is a fairly common scenario, so I'd expect most people to be aware of what's going on (or be able to infer it from context at least).

By the way I would move the .catch call one level up, so that it doesn't muck with hydrateUserSettings, which has no expected reason for throwing even if an ES error is caught.

Good call on moving the .catch call one level up. Since we are also going to be logging here, I'm just going to create a small function to do that and return the {}, and pass that function to .catch. Hopefully this will help make it clearer why this bit of code exists.

bevacqua · 2016-06-01T14:34:57Z

While it's okay to suppress these errors we should still log them somewhere, as @pickypg points out.

ycombinator · 2016-06-01T14:36:45Z

While it's okay to suppress these errors we should still log them somewhere, as @pickypg points out.

Thanks @bevacqua. I will log them in the Kibana server log. I am unfamiliar with the intent of this code so can you clarify what it is trying to do, specifically, what does "user settings" mean here? This will help me come up with an appropriate error message.

bevacqua · 2016-06-01T14:39:25Z

"User settings" are the settings the user has modified, and we have stored in .kibana. Those are merged with system defaults.

ycombinator · 2016-06-01T19:19:26Z

Lets hold off on this PR for a bit. I believe there is an underlying issue that needs to be addressed first: #7278 (comment).

ycombinator · 2016-06-02T00:20:54Z

The discussion has progressed on #7278 (comment) and we've agreed to change the Kibana plugin status to red (with an appropriate error message) if user settings for the installed version of Kibana cannot be found. This will drop the user into the status page, which is better than timing out the request from the browser.

I've updated this PR to implement the above functionality and it is ready for review again. @pickypg, @bevacqua: mind taking another look at this?

This is because bind() is typically used to change the value of this. As we aren't actually changing the value of this here, using _.partial() explains the intent more clearly, IMO.

bevacqua · 2016-06-02T14:27:26Z

Love it! 🎉

pickypg · 2016-06-02T15:16:44Z

Ditto. LGTM

In elastic#7333 we needed the ability to set the server status from outside of a plugin, but statuses were implemented in a way that coupled them to plugins. This let to reaching in and setting the status of a plugin from the server. Rather than extending the undesirable coupling of status & plugin I've instead made the server status service support creating more generic status tracker objects, and extended it's API to include plugin-specific methods like `createForPluginId(pluginId)` and `getStateForPluginId(pluginId)`. With the new API the settings service will be able to create it's own status object with `kbnServer.status.create('settings')` rather than reaching into the kibana plugin and setting its status.

In elastic#7333 we needed the ability to set the server status from outside of a plugin, but statuses were implemented in a way that coupled them to plugins. This let to reaching in and setting the status of a plugin from the server. Rather than extending the undesirable coupling of status & plugin I've instead made the server status service support creating more generic status tracker objects, and extended it's API to include plugin-specific methods like `createForPluginId(pluginId)` and `getStateForPluginId(pluginId)`. With the new API the settings service will be able to create it's own status object with `kbnServer.status.create('settings')` rather than reaching into the kibana plugin and setting its status. Former-commit-id: e6a8da0

`v90.0.0`⏩`v91.0.0-backport.0` ⚠️ While this upgrade pings many teams and has a large code diff, **the majority of the changes are snapshots or tests-related** and do not touch source code, so should theoretically only need a code review and not dedicated QA. The changes in EUI that required a large swathe of these updates are: - **EuiPopover** removed an extra unnecessary `<div>` wrapper on its anchors, which affected many snapshots and a few CSS overrides, which should have been updated - **EuiButtonGroup** now renders `<button>` elements instead of `<input type="radio">` elements for single selection, which affected both snapshots and E2E tests - **EuiSuperDatePicker**'s absolute date input now requires an `Enter` keypress when parsing dates (affected E2E tests) - **EuiComboBox**, when rendered with `singleSelection={{ plainText: 'true' }}`, no longer renders a pill (i.e. text). This combobox type now behaves more like an `EuiFieldText`, where the selection is rendered via input `value` instead. This affected a high amount of E2E tests (both FTR and Cypress), both in terms of updating assertions and changing selections, but should **not** significantly affect user experience - see elastic/eui#7332 for more. --- ## [`v91.0.0-backport.0`](https://github.com/elastic/eui/tree/v91.0.0-backport.0) **This is a backport release only intended for use by Kibana.** - Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs. - `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 a bug with `EuiSelectable`s with custom `truncationProps`, where scrollbar widths were not being accounted for ([#7392](elastic/eui#7392)) ## [`91.0.0`](https://github.com/elastic/eui/tree/v91.0.0) - Updated the background color of `EuiPopover`s in dark mode to increase visibility & contrast against other page/panel backgrounds ([#7310](elastic/eui#7310)) - Memoized `EuiDataGrid` to prevent unneeded re-renders ([#7324](elastic/eui#7324)) - Added a configurable `role` prop to `EuiAccordion` ([#7326](elastic/eui#7326)) - Added a configurable `role` prop to `EuiGlobalToastList` ([#7328](elastic/eui#7328)) - For greater flexibility, `EuiSuperDatePicker` now allows users to paste ISO 8601, RFC 2822, and Unix timestamps in the `Absolute` tab input, in addition to timestamps in the `dateFormat` prop ([#7331](elastic/eui#7331)) - Plain text `EuiComboBox`es now behave more like a normal text field/input. Backspacing will no longer delete the entire value, and selected values can now be double clicked and copied. ([#7332](elastic/eui#7332)) - `EuiDataGrid`'s display settings popover now allows users to clear the "Lines per row" input before typing in a new number ([#7338](elastic/eui#7338)) - Improved the UX of `EuiSuperDatePicker`'s Absolute tab for users manually typing in timestamps ([#7341](elastic/eui#7341)) - Updated `EuiI18n`s with multiple `tokens` to accept dynamic `values` ([#7341](elastic/eui#7341)) **Bug fixes** - Fixed `EuiComboBox`'s `onSearchChange` callback to pass the correct `hasMatchingOptions` value ([#7334](elastic/eui#7334)) - Fixed an `EuiSelectableTemplateSitewide` bug where the `popoverButton` behavior would break if passed a non-DOM React wrapper ([#7339](elastic/eui#7339)) **Deprecations** - `EuiPopover`: deprecated `anchorClassName`. Use `className` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: deprecated `buttonRef`. Use `popoverRef` instead ([#7311](elastic/eui#7311)) - `EuiPopover`: removed extra `.euiPopover__anchor` div wrapper. Target `.euiPopover` instead if necessary ([#7311](elastic/eui#7311)) - Deprecated `EuiButtonGroup`'s `name` prop. This can safely be removed. ([#7325](elastic/eui#7325)) **Breaking changes** - Removed deprecated `euiPaletteComplimentary` - use `euiPaletteComplementary` Instead ([#7333](elastic/eui#7333)) **Accessibility** - Updated `type="single"` `EuiButtonGroup`s to render standard buttons instead of radio buttons under the hood, per recent a11y recommendations ([#7325](elastic/eui#7325)) - `EuiAccordion` now defaults to a less screenreader-noisy `group` role instead of `region`. If your accordion contains significant enough content to be a document landmark role, you may re-configure it back to `region`. ([#7326](elastic/eui#7326)) - Reduced screen reader noisiness when sorting `EuiDataGrid` columns via toolbar ([#7327](elastic/eui#7327)) - `EuiGlobalToastList` now defaults to a `log` role. If your toasts will always require immediate user action, consider (with caution) using the `alert` role instead. ([#7328](elastic/eui#7328)) **CSS-in-JS conversions** - Updated `$euiFontFamily` and `$euiCodeFontFamily` to match Emotion fonts ([#7332](elastic/eui#7332)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com> Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>

If there is an error, return an empty object for user settings

4902271

Otherwise the promise never settles and can block a response from the Kibana server.

ycombinator assigned bevacqua May 31, 2016

bevacqua reviewed Jun 1, 2016
View reviewed changes

bevacqua assigned ycombinator and unassigned bevacqua Jun 1, 2016

Turn Kibana plugin red if user settings cannot be found

357e391

ycombinator changed the title ~~If there is an error, return an empty object for user settings~~ If user settings cannot be found for installed version of Kibana, turn Kibana plugin red Jun 1, 2016

ycombinator mentioned this pull request Jun 1, 2016

Server doesn't respond until elasticsearch is live #7278

Closed

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

Using _.partial() instead of bind()

484e45f

This is because bind() is typically used to change the value of this. As we aren't actually changing the value of this here, using _.partial() explains the intent more clearly, IMO.

ycombinator assigned bevacqua and pickypg and unassigned ycombinator Jun 2, 2016

bevacqua removed their assignment Jun 2, 2016

ycombinator merged commit 6e46c0e into elastic:master Jun 2, 2016

ycombinator deleted the gh-7318 branch June 2, 2016 15:16

ycombinator added the v5.0.0 label Jun 2, 2016

ycombinator assigned ycombinator and unassigned pickypg Jun 2, 2016

spalger mentioned this pull request Jun 15, 2016

[server/status] implement generic status tracking #7459

Merged

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

If user settings cannot be found for installed version of Kibana, turn Kibana plugin red #7333

If user settings cannot be found for installed version of Kibana, turn Kibana plugin red #7333

ycombinator commented May 31, 2016 •

edited

Loading

pickypg commented May 31, 2016

ycombinator commented May 31, 2016

bevacqua Jun 1, 2016

ycombinator Jun 1, 2016

bevacqua Jun 1, 2016

ycombinator Jun 1, 2016 •

edited

Loading

bevacqua commented Jun 1, 2016

ycombinator commented Jun 1, 2016

bevacqua commented Jun 1, 2016

ycombinator commented Jun 1, 2016

ycombinator commented Jun 2, 2016

bevacqua commented Jun 2, 2016

pickypg commented Jun 2, 2016

If user settings cannot be found for installed version of Kibana, turn Kibana plugin red #7333

If user settings cannot be found for installed version of Kibana, turn Kibana plugin red #7333

Conversation

ycombinator commented May 31, 2016 • edited Loading

pickypg commented May 31, 2016

ycombinator commented May 31, 2016

bevacqua Jun 1, 2016

Choose a reason for hiding this comment

ycombinator Jun 1, 2016

Choose a reason for hiding this comment

bevacqua Jun 1, 2016

Choose a reason for hiding this comment

ycombinator Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

bevacqua commented Jun 1, 2016

ycombinator commented Jun 1, 2016

bevacqua commented Jun 1, 2016

ycombinator commented Jun 1, 2016

ycombinator commented Jun 2, 2016

bevacqua commented Jun 2, 2016

pickypg commented Jun 2, 2016

ycombinator commented May 31, 2016 •

edited

Loading

ycombinator Jun 1, 2016 •

edited

Loading