[uiSettings] support overriding uiSettings from the config file by spalger · Pull Request #21628 · elastic/kibana

spalger · 2018-08-03T02:05:55Z

This PR implements the uiSettings.overrides setting which [when stored in kibana.yml or passed as config args when starting Kibana] allows forcing some uiSettings to always have a specific value. This setting accepts a map of uiSetting keys to values that will always be used to override whatever is stored in the config saved object.

When users view the settings in the advanced settings UI they are disabled and describe why they can't be changed.

Attempting to change these values from the uiSettings client/service/api is also prevented, causing a 400 error to be thrown and/or sent as the response.

elasticmachine · 2018-08-03T03:15:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-03T05:57:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-03T09:16:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

LeeDr · 2018-08-03T13:20:53Z

@spalger I must be missing something. We already have 2 ways for tests to set whatever things they want in the .kibana config doc which I think is where all the advanced settings are stored.

Many tests load a .kibana index which already has some settings in the config doc including things like default index pattern.
await kibanaServer.uiSettings.disableToastAutohide();
await kibanaServer.uiSettings.replace({ 'dateFormat:tz': 'UTC', 'defaultIndex': 'logstash-*' });

These seems like a pretty large change (25 files) without a great explanation for what problem it solves over and above the other ways we already have to set things.

LeeDr · 2018-08-03T13:36:43Z

Aren't these uiSettings.overrides stored in the config doc? If they are, then each test that replaces the .kibana index would wipe them out anyway. If they're not, please explain.

…setting-overrides

spalger · 2018-08-03T14:56:55Z

I must be missing something. We already have 2 ways for tests to set whatever things they want in the .kibana config doc which I think is where all the advanced settings are stored.

Yeah all of those options are for mutating the uiSettings stored in elasticsearch, but this change doesn't not try to solve that problem. Instead it makes a uiSetting immutable, and forces the API to use a specific value for that setting. The value is not persisted to Elasticsearch, it is only found in the config file, and if you try to change that value from the API you will get an error because the value is overridden.

This way users of Kibana can choose to enforce certain uiSettings from the config file without having to interact with Kibana to change the setting.

spalger · 2018-08-03T14:59:01Z

Aren't these uiSettings.overrides stored in the config doc? If they are, then each test that replaces the .kibana index would wipe them out anyway. If they're not, please explain.

The overrides are not stored in the config document, they are stored in the kibana.yml file and merged with the values from the config document when it is read within the uiSettings service/API.

elasticmachine · 2018-08-03T15:49:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

LGTM! My only suggestions were about consistent terminology, either overridden or isControlledByServer.

And I do have an idea about this, though feel free to reject it if you're sick of the whole thing. What do you think of using the term isBakedIn instead? It conveys the idea to me that Kibana's been unalterably configured with certain settings. And we can use it to describe the feature in the release notes: "Administrators can now bake settings into Kibana for their users."

cjcenizal · 2018-08-06T18:27:38Z

src/core_plugins/kibana/public/management/sections/settings/components/field/field.js

+    if (setting.isControlledByServer) {
+      return (
+        <EuiText size="xs">
+          This setting is overriden by the Kibana server and can not be changed.


How about "An administrator has baked this setting into Kibana's configuration"? I don't think we need to explain that it can't be changed since the UI makes that clear.

I'm really not a huge fan of the "baked" terminology. It implies to me that the values are set by some "baking" process, like if they were baked into the build or something. I think if we wanted to get away from the "controlled" terminology I would prefer something like "forced", but then I feel like we come back around to "overridden"... I think I'm most comfortable with the idea of the UI/API representing these config values as "controlled by the server", and the server seeing them as overrides that it applies to the config values.

OK, works for me.

Someone reading it might think you're getting "baked" while coding ;-)

cjcenizal · 2018-08-06T22:25:37Z

src/core_plugins/kibana/public/management/sections/settings/advanced_settings.test.js

  set: () => {},
  remove: () => {},
  isCustom: (setting) => setting.isCustom,
+  isControlledByServer: (key) => key.startsWith('test:overridden'),


Maybe references to overridden should be replaced by isControlledByServer here?

cjcenizal · 2018-08-06T22:27:16Z

src/core_plugins/kibana/public/management/sections/settings/components/field/field.js

            checked={!!unsavedValue}
            onChange={this.onFieldChange}
-            disabled={loading}
+            disabled={loading || setting.isControlledByServer}


Maybe destructure isControlledByServer along with the other setting properties on line 322?

cjcenizal · 2018-08-06T22:43:44Z

src/ui/ui_settings/__tests__/ui_settings_mixin_integration.js

    const config = await Config.withDefaultSchema({
-      uiSettings: { enabled }
+      uiSettings: {
+        overrides: {


Do we still need the overrides "namespace"? If we still need a namespace, maybe "controlledByServer" would be more consistent?

I'd like to leave the option for custom defaults: down the road, and I feel like controlledByServer makes more sense from the UI perspective, but less sense in the server config itself.

cjcenizal · 2018-08-06T22:44:51Z

src/ui/ui_settings/public/ui_settings_client.js

+
+  assertUpdateAllowed(key) {
+    if (this.isControlledByServer(key)) {
+      throw new Error(`Unable to update "${key}" because it is overridden by the Kibana server`);


Maybe it's more consistent to say "because it is controlled by the Kibana server"?

…setting-overrides

elasticmachine · 2018-08-07T22:22:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…setting-overrides

elasticmachine · 2018-08-08T00:28:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-08T00:31:40Z

retest

elasticmachine · 2018-08-08T02:11:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

bmcconaghy

Code looks good to me, but the naming here still seems off. The variable is "inControlledByServer" but the word overridden is used all over the place. The relationship between the two concepts is murky. Maybe it could be "isLockedByServer" and we could just use locked everywhere. We need to settle on a single term to represent what is happening here.

bmcconaghy · 2018-08-08T15:10:46Z

I'm OK with overrides as the consistent term. Just want there to be a single term for what this is.

…setting-overrides

elasticmachine · 2018-08-14T23:26:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…tic#21628) This PR implements the `uiSettings.overrides` setting which [when stored in kibana.yml or passed as config args when starting Kibana] allows forcing some uiSettings to always have a specific value. This setting accepts a map of uiSetting keys to values that will always be used to override whatever is stored in the config saved object. ![image](https://user-images.githubusercontent.com/1329312/43619094-feded1ae-9680-11e8-9ec3-c12d4d949c46.png) When users view the settings in the advanced settings UI they are disabled and describe why they can't be changed. ![image](https://user-images.githubusercontent.com/1329312/43618938-2cdee0f4-9680-11e8-9ed6-f384d4ee78f6.png) Attempting to change these values from the uiSettings client/service/api is also prevented, causing a 400 error to be thrown and/or sent as the response.

…#21628) (#21983) Backports the following commits to 6.x: - [uiSettings] support overriding uiSettings from the config file (#21628)

spalger · 2018-08-15T23:22:19Z

6.5/6.x: c175c04

spalger added 2 commits August 2, 2018 17:44

[uiSettings] support overriding uiSettings from the config file

8271263

[uiSettings] show overridden settings as read only in management

413a0a4

spalger added review :Management v7.0.0 v6.5.0 labels Aug 3, 2018

spalger mentioned this pull request Aug 3, 2018

Implement no animation mode #21629

Merged

cjcenizal self-requested a review August 3, 2018 02:56

spalger added 4 commits August 2, 2018 20:16

remove a few left-over uiSettings.enabled uses

6bf8c2e

[uiSettings] include isOverriden flag in uiSetttings/get API response

6f515f3

[ftr/uiSettings] don't delete keys that are overridden by config

49b70a3

[uiSettings/public] use isOverridden flags rather than keys

7328d6a

spalger force-pushed the implement/ui-setting-overrides branch from 4446556 to 7328d6a Compare August 3, 2018 04:00

spalger added 2 commits August 2, 2018 21:24

whoops, didn't mean to remove that

28c2191

[uiSettings/mixin] test passing of overrides in mixin

976e3a9

[uiSettings] isOverridden -> isControlledByServer

1267833

LeeDr self-requested a review August 3, 2018 13:23

Merge branch 'master' of github.com:elastic/kibana into implement/ui-…

d130bc1

…setting-overrides

cjcenizal approved these changes Aug 6, 2018

View reviewed changes

spalger requested review from bmcconaghy and jen-huang August 7, 2018 20:36

Merge branch 'master' of github.com:elastic/kibana into implement/ui-…

c695f51

…setting-overrides

spalger added 3 commits August 7, 2018 13:40

[advancedSettings/tests] use isControlledByServer naming

a27d580

[advancedSettings/field] destructure setting.isControlledByServer

f97eea9

[uiSettings/client] use "controlled" verbiage

4a2664e

spalger added 2 commits August 7, 2018 15:35

Merge branch 'master' of github.com:elastic/kibana into implement/ui-…

dc5c82f

…setting-overrides

update snapshots

c476514

bmcconaghy reviewed Aug 8, 2018

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into implement/ui-…

95229b4

…setting-overrides

spalger force-pushed the implement/ui-setting-overrides branch from ba7e2fa to ae7f738 Compare August 14, 2018 20:40

[uiSettings] switch back to "overridden" naming for consistency

ef3c14f

spalger force-pushed the implement/ui-setting-overrides branch from ae7f738 to ef3c14f Compare August 14, 2018 20:43

This comment has been minimized.

Sign in to view

spalger merged commit e49d5f3 into elastic:master Aug 15, 2018

spalger mentioned this pull request Aug 15, 2018

[6.x] [uiSettings] support overriding uiSettings from the config file (#21628) #21983

Merged

spalger pushed a commit that referenced this pull request Aug 15, 2018

[6.x] [uiSettings] support overriding uiSettings from the config file (…

c175c04

…#21628) (#21983) Backports the following commits to 6.x: - [uiSettings] support overriding uiSettings from the config file (#21628)

spalger deleted the implement/ui-setting-overrides branch August 15, 2018 23:22

lukeelmers mentioned this pull request Apr 19, 2021

[uiSettings]Removes Infinity from notifications lifetimes #97384

Merged

4 tasks

kertal mentioned this pull request Jun 29, 2022

Add an example of uiSettings override #135385

Merged

Comments

Conversation

spalger commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 3, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Aug 3, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Aug 3, 2018

💔 Build Failed

Uh oh!

LeeDr commented Aug 3, 2018

Uh oh!

LeeDr commented Aug 3, 2018

Uh oh!

spalger commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spalger commented Aug 3, 2018

Uh oh!

elasticmachine commented Aug 3, 2018

💚 Build Succeeded

Uh oh!

cjcenizal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 7, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Aug 8, 2018

💚 Build Succeeded

Uh oh!

spalger commented Aug 8, 2018

Uh oh!

elasticmachine commented Aug 8, 2018

💚 Build Succeeded

Uh oh!

bmcconaghy left a comment

Choose a reason for hiding this comment

Uh oh!

bmcconaghy commented Aug 8, 2018

Uh oh!

This comment has been minimized.

elasticmachine commented Aug 14, 2018

💚 Build Succeeded

Uh oh!

spalger commented Aug 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spalger commented Aug 3, 2018 •

edited

Loading

spalger commented Aug 3, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading